search

JHS-Viking-Robotics / FRC-2022

Code for the 2022 FRC season, now written in Java
Other
2 stars 2 forks source link

Added enums for direction and intake. #21

Closed jalzeidi closed 3 years ago

derickson2402 commented 3 years ago

Hi Jaffar, thanks so much for your interest in this project, and for submitting a Pull Request!

When I opened issue #20, I figured that an enum would be the easiest way to keep track of the different positions/states for the Hopper Lift/Intake. In fact, it probably is the best way, and your code does an awesome job of implementing this solution :smile::thumbsup: .

Looking at this issue again though, I think that adding the positions/motor outputs into Constants.java instead would be best, as that is how the rest of the code is structured. For example, using Constants.Hopper.LIFT_UP or Constants.Hopper.INTAKE_IN instead (it may be even better to use another subclass of Hopper (check out #17), but I'd like to wait on this refactor for a bit until some of the current changes are tested).

If you are still interested in helping, I'd definitely approve a PR which did this. I do not have the position values on hand, so just use zeros or dummy values for now. Thanks so much for your interest and help!

derickson2402 commented 3 years ago

The other reason for using Constants.java is that the enum/static will need to be accessible from a separate Command class and/or from RobotContainer.java, so being able to import from Constants.java will be useful

derickson2402 commented 3 years ago

Ok yet another different idea, we will probably use NetworkTables values to store the actual values so that they can easily be overridden on the dashboard. Then on boot, those values will be set to default using the values from Constants.java.

This coincides with the current design like in 8db7e7d, where NetworkTables is used as the store for motor controller configuration options so they are easy to change during testing, then synced to the controllers in the code.

derickson2402 commented 3 years ago

Closing this PR and moving the discussion to #20