-
Notifications
You must be signed in to change notification settings - Fork 0
21 add drive states and linear drive #25
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
base: main
Are you sure you want to change the base?
Conversation
- Change team number in wpilib_preferences.json - Update drivetrain constants for max speeds and accelerations - Modify feature flags to disable turret operation - Refactor drive command bindings to use DriveCoordinator - Introduce DriveCoordinator and associated states for improved drive management - Enhance logging for drive goal speeds and state transitions
…ove redundant unit registration in JsonConstants
- Introduced DriveGainsTuning mode in TestMode enum for tuning drive parameters. - Added PIDGains class to encapsulate tuning gains for steering and driving. - Implemented DriveTestModeState to manage tuning of drive and steer gains during operation. - Enhanced DriveWithJoysticksState to support entry and exit actions for drive commands. - Created LoggedTunablePIDGains class for managing and logging PID gains dynamically.
…nstants and DrivetrainConstants classes with updated configurations (WIP)
…functionality with additional methods for configuration application (WIP)
…s and remove TunerConstants references
…ity; update PIDGains to include kG parameter and adjust related classes accordingly.
…onstants configuration file
… implementations; update PIDGains methods to return modified configurations
|
@avidraccoon how's this coming along? |
…update LoggedTunablePIDGains to adjust naming conventions and enhance readability.
… classes; update ModuleIOSim to adjust DRIVE_KV based on PIDGains.
…ssary import in Module class.
…o 21-add-drive-states-and-linear-drive
…ment, and streamline DriveTestModeState logic
aidnem
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is just my first pass of the constants stuff, I'll look into the states and DriveCoordinator tomorrow (I don't want to code review when I'm super tired, as I'll likely miss stuff or make silly mistakes).
| "maxAngularVelocity": { | ||
| "value": 6.2, | ||
| "unit": "Radian per Second" | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are there fields for max(Linear|Angular)Speed AND velocity here? Shouldn't there just be one?
| "kV": 0.75722, | ||
| "kG": 0.0, | ||
| "kA": 0.0 | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should steergains exist here too? Or will they once someone calls saveObject again? if that's the case, I don't mind not having them in here for just this merge, we can deal with it later.
| @@ -1,12 +1,123 @@ | |||
| { | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we have "DriveConstants" and "DrivetrainConstants"? Should this be called TunerConstants?
| import edu.wpi.first.units.measure.Voltage; | ||
| import frc.robot.constants.JsonConstants; | ||
|
|
||
| public class DrivetrainConstants { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we get a javadoc explaining the distinction between DriveConstants and DrivetrainConstants?
| new ModuleConfig() | ||
| .withDriveMotorId(7) | ||
| .withSteerMotorId(2) | ||
| .withEncoderId(9) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this refactor. I assume it will still be decently portable if we have to regenerate TunerConstants?
|
|
||
| // Temporary manual calls to load fields after JSON load | ||
| robotInfo.loadFieldsFromJSON(); | ||
| drivetrainConstants.finishLoadingConstants(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coppercore is now upgraded to support afterJsonLoad so you can use that here.
|
|
||
| // Fields loaded after JSON | ||
|
|
||
| public CANBus kCANBus; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be JSONExcluded?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I prefer not to use the k notation before constants that aren't gains. Could we maybe name it canivoreBus or something similar to make it clear to which bus we are referring?
|
|
||
| // @AfterJSONLoad | ||
| public void loadFieldsFromJSON() { | ||
| kCANBus = new CANBus(canivoreBusName, logFilePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason you didn't use the constructor that just takes a bus name here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was from tuner constants
| import edu.wpi.first.units.measure.*; | ||
|
|
||
| // Generated by the Tuner X Swerve Project Generator | ||
| // 2025 B BOT CONSTANTS!!!!! (dont remove put somewhere else please) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we get these immortalized in their own environment for JSON? this way, even if we do destroy this java file (which we may have to do if we regenerate tuner constants), we don't lose B bot?
No description provided.