Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial support for encoders in simulation #36

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

LoadingPleaseWait
Copy link
Contributor

The EncoderJNI class has been added in patches so that encoders can be used in simulation.

@JaciBrunning
Copy link
Member

I can only assume this is in reference to #35

I'm going to leave this open for now, but not merged. I won't merge it until some further support is added (e.g. "working"), or until a discussion is made on it.

I think most users would rather Crash in simulation, knowing that the Encoders aren't supported in simulation, than to be reading false values of 0 and ripping their hair out wondering why their PID loops aren't working.

@LoadingPleaseWait
Copy link
Contributor Author

You are correct; this is for #35. Waiting for further support sounds fair. I'll add a way for the user to change the encoder values through the simulation GUI in another commit for this pull request.

@JaciBrunning
Copy link
Member

I'd recommend the GUI for the rate of encoder change as well as value. That would allow for Drive Train PID tuning, as well.

@LoadingPleaseWait
Copy link
Contributor Author

Alright, I'll have both the rate and the value in the GUI.

@LoadingPleaseWait
Copy link
Contributor Author

In my most recent commit, I added GuiNumberSpinners for the rate and value of encoders. I also added more GuiNumberSpinners for analog input. I know it's not very pretty since there isn't much space in the window but it gets the job done.

@floogulinc
Copy link
Collaborator

@LoadingPleaseWait can you post a screenshot of the GUI changes?

@LoadingPleaseWait
Copy link
Contributor Author

I know it looks a little different because I'm in Ubuntu.
screenshot from 2016-03-11 23-14-33

@floogulinc
Copy link
Collaborator

Looks like we still need to fix the margins on the little buttons...

@LoadingPleaseWait does that only show up if you've created an encoder object? Also, how does it handle multiple encoders (and ones of different types)?

Maybe they should be next to the DIO or analog in pins they were created using.

@LoadingPleaseWait
Copy link
Contributor Author

For now, the encoder label and buttons are there all the time and all encoders have those values.

@floogulinc
Copy link
Collaborator

I think we should avoid cluttering up the interface like this and have it work like everything else (DIO, analog IO, talons, PWM) and have encoders only show up when they have been instantiated. Also, users should be able to set individual encoder values instead of setting every encoder.

I also think that we should come up with a plan for how to support the many different sensors wpilib supports. We should also look at support for sensors plugged into Talon SRXs.

@JaciBrunning
Copy link
Member

I would recommend putting encoders under a different window, like how SRXs and Pneumatics. Sensors are the hardest thing to simulate, as it relies a lot of user intuition as to what values to put in

@LoadingPleaseWait
Copy link
Contributor Author

I'll put the encoder count and rate in a window just like the Talon SRXs and Pneumatics. I'll also make it so that you can control each encoder individually.

@LoadingPleaseWait
Copy link
Contributor Author

I've added a commit which puts the encoders in a different window and you can now control all the encoders individually.
screenshot from 2016-03-14 18-20-44

@JaciBrunning
Copy link
Member

Looks good. I'll check out the commit and give it a test after Week 3 events (I fly out in about 12 hours). @floogulinc could you give this a test if you get a free moment?

@Samcfuchs
Copy link

Hey what's the status on this PR now?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants