Add Setting the Absolute Encoder Offset #105
Conversation
Add Setting the Absolute Encoder Offset though code
|
Addresses #100 |
|
This needs to tie in from Great base work! |
|
So do you want this to replace the offset built into YAGSL? Or replace the functionality of it? I'm a bit confused by your last comment |
|
The use case for this is not universal so you would have to treat it as such. Your current implementation is fine except no one would use it bc its a pain to setup without modifying YAGSL. So a more generic solution would be "modes" for the offset in |
|
Ok so how do you feel about me putting it as optional in the JSON so if teams want to use it it's available and if they don't it's not? Like offset type and offset. If no type specified then use the default built-in one. If type specified of "encoder-level" or something then it uses the alternative method. |
|
I try to avoid adding to JSONs as much as possible to reduce confusion and keep a stable schema. I dont think this issue arises to a point where it seems reasonable to add to the schema, mainly because it is not something that affects swerve drives so much that it would significantly add to cycles. I like the idea but I think it would be unnecessary clutter in module JSONs. An easy way to trigger it programmatically would be |
… on the encoder Allows SwerveDrive to change the absolute encoder offsets Checks to see if successful before undoing the internal YAGSL offsets Has warnings for unsupported devices and errors for why
|
Okay I updated it based on your requests. Besides maybe the phrasing I think its really good. |
|
Not sure how to resolve the conflict, but it just missing } and /** |
There was a problem hiding this comment.
While I understand why you would want to save the offset from the encoder, it gets blown away at initialization of the encoder through factory resets anyways (supposedly, SparkMAX with throughbore needs investigation still). Sorry that i dont think so much of it is needed offends you, i am willing to debate whether or not it may be useful!
This is pretty great work and I appreciate all of your effort put into it!
|
|
||
| /** | ||
| * Get the current Absolute Encoder offset as reported by the Encoder. | ||
| */ | ||
| public abstract double getAbsoluteEncoderOffset(); |
There was a problem hiding this comment.
You do not need this function.
| /** | ||
| * Get the Absolute Encoder Offset inside of the SparkMax's Memory. | ||
| * | ||
| * @return The zero offset of the absolute encoder with the position conversion factor applied from the Sparkmax's Memory. | ||
| */ | ||
| @Override | ||
| public double getAbsoluteEncoderOffset() | ||
| { | ||
| return encoder.getZeroOffset(); | ||
| } |
| /** | ||
| * Get the Offset of the Encoder in the WPILib Encoder Library. | ||
| * | ||
| * @return Current Zero Point Offset. | ||
| */ | ||
| @Override | ||
| public double getAbsoluteEncoderOffset() | ||
| { | ||
| return encoder.getPositionOffset(); | ||
| } |
| * Cannot get or set the offset of the CanAndCoder. | ||
| * | ||
| * @return Will always return 0.0 | ||
| */ | ||
| @Override | ||
| public double getAbsoluteEncoderOffset() | ||
| { | ||
| //CanAndCoder does not support Absolute Offset Changing | ||
| return 0.0; | ||
| } |
| { | ||
| return true; | ||
| } | ||
| DriverStation.reportWarning("Failure to set Absolute Encoder Offset Error: "+error, false); |
There was a problem hiding this comment.
Include the ID and the encoder type in message.
There was a problem hiding this comment.
Idk where I can get those. It doesn't seem to be accessible for the variables that currently exist in either SwerveModule or the Encoder
| public void pushOffsetsToControllers(double offset) | ||
| { | ||
| oldAbsoluteEncoderOffset = absoluteEncoder.getAbsoluteEncoderOffset(); | ||
| if(absoluteEncoder.setAbsoluteEncoderOffset(offset) == true){ |
There was a problem hiding this comment.
Can be simplified, also remember absoluteEncoder could be null so a check is required. Also should use angleOffset
| * | ||
| * @param offset the offset the AbsoluteEncoder should use. | ||
| */ | ||
| public void pushOffsetsToControllers(double offset) |
There was a problem hiding this comment.
I disagree with passing the offset here, solely because it could introduce extreme confusion and questions on "why isnt the angle being set" by teams who switched programmers. Offset should be angleOffset
| * | ||
| * @param restoreMemory Whether or not the origional offset value should be restored. | ||
| */ | ||
| public void restoreInternalOffset(boolean restoreMemory) |
There was a problem hiding this comment.
This function should not use the memory from the motor controller, parameter isnt necessary.
| if(restoreMemory){ | ||
| absoluteEncoder.setAbsoluteEncoderOffset(oldAbsoluteEncoderOffset); | ||
| } |
| absoluteEncoder.setAbsoluteEncoderOffset(oldAbsoluteEncoderOffset); | ||
| } | ||
| else{ | ||
| absoluteEncoder.setAbsoluteEncoderOffset(0); |
There was a problem hiding this comment.
absoluteEncoder could be null and should be checked
I can resolve it later, its not a big deal rn |
|
Honestly I'm chilling I'm not an amazing programmer so I'm learning. It doesn't offend me at all. I completely understand your concerns. I think it's super difficult to convey tone over text so I apologize if I gave you that impression. |
|
If you mean my phrasing comment I meant it's hard to explain the difference between the YAGSL offsets and the offsets stored in the encoder/motor controller |
|
Not a problem. It is very hard to explain. I had to abstract alot to keep it generic enough for any configuration unfortunately in keeping with a paradigm where nothing from configuration classes gets changes (mostly, i broke it a few times and need to fix those later) i duplicated some variables 2-3 times over where only 1 is used, including the one burned to motor controllers. |
| * | ||
| * @param restoreMemory Whether the memory inside the encoder is restored or not. | ||
| */ | ||
| public void restoreInternalOffset(boolean restoreMemory) |
There was a problem hiding this comment.
Unnecessary parameter
src/main/java/swervelib/encoders/AnalogAbsoluteEncoderSwerve.java
Outdated
Show resolved
Hide resolved
| public boolean setAbsoluteEncoderOffset(double offset) | ||
| { | ||
| //CanAndCoder does not support Absolute Offset Changing | ||
| DriverStation.reportWarning("Cannot Set Absolute Encoder Offset of CanAndCoders", false); |
There was a problem hiding this comment.
Add PWM ID to this error message
| @Override | ||
| public boolean setAbsoluteEncoderOffset(double offset) | ||
| { | ||
| REVLibError error = encoder.setZeroOffset(offset); |
There was a problem hiding this comment.
Wrap this in a for loop from 0 to maximumRetries. SparkMAX's can be finnicky.
|
The CanAndCoder seems to actually allow setting the encoder offset they just dont have good documentation how. https://docs.reduxrobotics.com/canspec/Canandcoder#settings Also additionally if the intention is to reset the encoders to factory defaults including the zero offset that is currently disabled for CanAndCoder |
|
The CANandCoder is an exception bc it has the button to zero it which some people may want to take advantage of. |
Added Device Ports in the warnings. However Sparkmaxes cannot easily report the ID from the encoder object.
|
I will fix up the merge conflicts tomorrow hopefully. |
Signed-off-by: thenetworkgrinch <thenetworkgrinch@users.noreply.github.com>
|
A week late, but I think this change will go over really well! |
|
Yeah I think it should be helpful to the teams that need it |
Add Setting the Absolute Encoder Offset though code.
Supported Encoders
Unsupported Encoders
Analog Encoder could be supported If it was using the WPILib analog encoder methods , but currently it uses digital inputs