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

[wpimath] Remove units from trapezoid profile classes #7276

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

SamCarlberg
Copy link
Member

They were assuming inputs were always going to be in SI units, which is not the case

@SamCarlberg SamCarlberg added the component: wpimath Math library label Oct 23, 2024
@SamCarlberg SamCarlberg requested a review from a team as a code owner October 23, 2024 02:17
@PeterJohnson PeterJohnson changed the title Remove units from trapezoid profile classes [wpimath] Remove units from trapezoid profile classes Oct 23, 2024
@calcmogul
Copy link
Member

calcmogul commented Oct 23, 2024

Does TrapezoidProfile give incorrect answers if you pass in a non-SI unit? The profile should still work as long as the units are consistent.

@SamCarlberg
Copy link
Member Author

Does TrapezoidProfile give incorrect answers if you pass in a non-SI unit? The profile should still work as long as the units are consistent.

It works if measurement inputs are provided in terms of the SI base units. But people setting everything in terms of the same non-SI unit (such as degrees or inches or native sensor ticks) would have the math break

@calcmogul
Copy link
Member

But people setting everything in terms of the same non-SI unit (such as degrees or inches or native sensor ticks) would have the math break

That should work, actually. The equations in TrapezoidProfile don't care what the length unit is.

@WispySparks
Copy link
Contributor

WispySparks commented Oct 24, 2024

Can confirm we ran into this issue ourselves, we were using the profiled pid classes with trapezoid profiles and thought using units would be good but quickly found out everything was wonky when we didn't use what the class internally expected

@SamCarlberg
Copy link
Member Author

That should work, actually. The equations in TrapezoidProfile don't care what the length unit is.

Right. They just care that units are consistent. If you create constraints with the unit-taking constructor new Constraints(DegreesPerSecond.of(...), ...) and then run the profile with states with raw numbers in non-SI units (eg gyro.getRotation2d().getDegrees(), then units aren't consistent and things break even though the naming of the things used in the code is implies otherwise; the unit-taking constructor makes the constraints in terms of SI units, but the measurement inputs are in terms of non-SI units.

@Daniel1464
Copy link

Units-safe trapezoid profiles should arguably be a separate class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants