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

Rewrite DutyCycleEncoder and AnalogEncoder #6398

Merged
merged 33 commits into from
May 24, 2024

Conversation

ThadHouse
Copy link
Member

@ThadHouse ThadHouse commented Feb 25, 2024

The existing AnalogEncoder and DutyCycleEncoder classes are massive footguns. They automatically include rollover support, which is often not what people want. Additionally, DutyCycle's rollover support is completely broken and cannot be trusted. It also cannot be easily fixed even at the FPGA level.

@ThadHouse ThadHouse requested a review from a team as a code owner February 25, 2024 21:51
@ThadHouse
Copy link
Member Author

Still needs C++, Tests and Sim support. Very WIP.

@rzblue
Copy link
Member

rzblue commented Feb 25, 2024

For consistency's sake it may also be better to make the offset value "the voltage/position measurement where you would like 0 to be" and subtract the offset from the measured value - this is typically more intuitive to find (even though it's the same number multiplied by -1) and would match the convention used by DutyCycleEncoder currently, as well as many swerve templates/libraries

@ThadHouse ThadHouse changed the title Deprecate AbsoluteEncoder classes, Replace DCE with DutyCyclePotentiometer Rewrite DutyCycleEncoder and AnalogEncoder Feb 29, 2024
@ThadHouse ThadHouse requested a review from a team as a code owner February 29, 2024 06:10
@ThadHouse
Copy link
Member Author

/format

@PeterJohnson
Copy link
Member

Please retarget to the main branch.

@Starlight220
Copy link
Member

Clarification: What's the difference between AnalogPotentiometer and this version of AnalogEncoder?

It took me some time to realize that expectedZero is the offset. I understand that this naming is unambiguous whether the offset is done before or after scaling, but I think the word "offset" should still be there in the parameter description and maybe other places.

@ThadHouse
Copy link
Member Author

What's the difference between AnalogPotentiometer and this version of AnalogEncoder

They're basically the same thing at this point. We'd probably deprecate AnalogPotentiometer since actual potentiometers are rarely used anymore.

As for the Zero, Naming things is hard. Some people think offset, some think zero. I've seen it about 50/50 split.

@Starlight220
Copy link
Member

Starlight220 commented Apr 29, 2024

As for the Zero, Naming things is hard. Some people think offset, some think zero. I've seen it about 50/50 split.

So whatever the parameter name is, the description should include the other name as well.


Maybe extract a base AnalogSensor<U> class that accepts a U(volt) conversion function and handles offset+scaling (likely by using the above function) and then it can be used for unit-aware classes of any analog sensor? We can add intuitively-named subclasses for common use cases (encoders, potentiometers, ultrasonics -- #4894, pressure sensors, and so on). This can be a great step for what was desired in #5033.

@ThadHouse ThadHouse requested review from PeterJohnson and a team as code owners April 29, 2024 18:56
@ThadHouse ThadHouse changed the base branch from development to main April 29, 2024 18:56
@ThadHouse
Copy link
Member Author

Maybe extract a base AnalogSensor<U> class that accepts a U(volt) conversion function and handles offset+scaling (likely by using the above function) and then it can be used for unit-aware classes of any analog sensor? We can add intuitively-named subclasses for common use cases (encoders, potentiometers, ultrasonics -- #4894, pressure sensors, and so on). This can be a great step for what was desired in #5033.

This should all be done later. All this PR should be is fixing up the low level API.

@PeterJohnson PeterJohnson merged commit d05c7c1 into wpilibsuite:main May 24, 2024
27 checks passed
chauser pushed a commit to chauser/allwpilib that referenced this pull request May 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DutyCycleEncoder .get() method randomly jumps up 1 revolution
5 participants