Add support to Talon SRX for an integrated absolute encoder #208
Conversation
|
This shouldnt work, you didnt create the wrapper for a psuedo absolute encoder like we discussed. It is a good attempt but I would not accept it in its current state. The other issue is you seem to forget that the absolute encoder is ALWAYS right, the absolute encoder can change the position, NOT the motor controller. An acceptable trade off which i may consider is implementing both abstractions for motor controllers and absolute encoders in one class but i dont think that will work out well. |
| @@ -188,7 +190,9 @@ public SwerveMotor createMotor(boolean isDriveMotor) | |||
| case "talonfx": | |||
| return new TalonFXSwerve(id, canbus != null ? canbus : "", isDriveMotor); | |||
| case "talonsrx": | |||
| return new TalonSRXSwerve(id, isDriveMotor); | |||
| return new TalonSRXSwerve(id, isDriveMotor, FeedbackDevice.QuadEncoder); | |||
| case "talonsrx_pwm": | |||
There was a problem hiding this comment.
I disagree with this name, it implies that the SRX is controlled over PWM
There was a problem hiding this comment.
is this an alright name for the encoder section?
|
You can rely on the TalonSRX sensor which is an attached absolute encoder. You just need to create a wrapper which stores the same motor object (NOT CREATE A NEW ONE) and exclusively polls the encoder position in the correct units. |
|
You're getting closer!! Good job! |
|
Last thing is, since this is a larger change than normal, please test this on an actual robot and let us know when you do! Once it is tested it can be merged. (Note: This does NOT need to be tested by you, just an actual robot) |
|
Okay it might be a while until this is able to be tested but I'll let you know |
First time working around the codebase, I went this route because I don't think you can really have a reference to a talon's sensor like you can with revlib. Not sure if this all looks right