-
Notifications
You must be signed in to change notification settings - Fork 611
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
Java units API rewrite #6958
Java units API rewrite #6958
Conversation
This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR. |
Can't speak for the internals but the new API looks much nicer 🙂 |
wpimath/src/test/java/edu/wpi/first/math/kinematics/ChassisSpeedsTest.java
Show resolved
Hide resolved
Consumer<State> recordState) { | ||
m_rampRate = rampRate != null ? rampRate : Volts.of(1).per(Seconds.of(1)); | ||
m_rampRate = rampRate != null ? rampRate : Volts.of(1).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.
Does per() no longer take a quantity but rather a unit type?
Never been happier to have been superseded. Good work |
Needs conflicts resolved. |
Java's type system is incapable of handling these with generics, so we need to write all the discrete cases ourselves
Generic arguments are 100% breaking from the v1 API Fix some missing imports Add`Unit.of`, `Unit.ofBaseUnits`, and `Unit.mutable` methods returning wildcarded generic types. Subclasses should override these to sharpen the signatures to the unit-specific measurement implementations like `Angle` and `Distance`.
Allows immutables to be records and easily converted to value types if valhalla ever lands
Reduces clutter in the root `units` package
Instead of separate discrete constructors for different unit types
Add code generation for the measure implementations (eg Distance, ImmutableDistance, and MutDistance) due to the sheer volume of manual work that would have been required to write (or copy/paste) ten thousand lines of code Reconsolidate the measure implementations into a single package Rename `negate` to `unaryMinus` and some variants of `per` to `divide` for consistent naming. Overloads of `divide` may be renamed to `per`; TBD Rename `Ratio` measure type back to `Per` to ease code generation. May be renamed back to `Ratio` or `Quotient`; TBD
Checkstyle and PMD are disabled for generated files
Add `per(TimeUnit)` abstract method to Unit for subclasses to implement. Often returns a velocity
Can't overload `times()` due to type erasure
Not as easy to combine into energy as torque, and it's a less common usecase Rename existing function to `multAsTorque` for clarity Add equivalent torque multiplication function to the Force class
return (Measure<AccelerationUnit<D>>) super.zero(); | ||
@SuppressWarnings({"unchecked", "rawtypes"}) | ||
public Acceleration<D> zero() { | ||
return (Acceleration<D>) (Acceleration) super.zero(); |
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.
A raw cast is required here due to conflicts with the wildcarded return supertype:
wpiunits/src/main/java/edu/wpi/first/units/AccelerationUnit.java:59: error: incompatible types: Measure<CAP#1> cannot be converted to Acceleration<D>
return (Acceleration<D>) super.zero();
^
where D is a type-variable:
D extends Unit declared in class AccelerationUnit
where CAP#1 is a fresh type-variable:
CAP#1 extends PerUnit<VelocityUnit<D>,TimeUnit> from capture of ? extends PerUnit<VelocityUnit<D>,TimeUnit>
I see, so from a programmer perspective, we still get the raw name? |
Yes, you would do something like this with the new API: Voltage appliedVoltage = motor.getAppliedVoltage();
Current draw = motor.getCurrent();
Power totalPower = appliedVoltage.times(draw); Instead of this (with the original units API): Measure<Voltage> appliedVoltage = motor.getAppliedVoltage();
Measure<Current> draw = motor.getCurrent();
Measure<Power> totalPower = (Measure<Power>) appliedVoltage.times(draw); Or this (if you want to use generic types for some reason). Note that this approach still loses out on type-safe math operations Measure<VoltageUnit> appliedVoltage = motor.getAppliedVoltage();
Measure<CurrentUnit> draw = motor.getCurrent();
Measure<PowerUnit> totalPower = (Measure<PowerUnit>) appliedVoltage.times(draw); |
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.
Nothing blocking, just a few wonderings.
wpilibjExamples/src/main/java/edu/wpi/first/wpilibj/examples/armbot/Constants.java
Outdated
Show resolved
Hide resolved
Allows for wider compatibility with results of math operations
Allows for known `Per` object outputs instead of bounded wildcard `Measure<? extends PerUnit>`
87790b5
to
7365901
Compare
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.
Did a search for Measure<
through the diff and found the following parameters that still use X
instead of Measure<XUnit>
on 7365901:
Command.withTimeout()
Commands.waitTime()
new WaitCommand()
new SysIdRoutine.Config()
(2 overloads)new SysIdRoutine.Mechanism()
(2 overloads)LEDPattern.scrollAtRelativeSpeed()
(Up to you how to handleasPeriod()
, since that's the only usage of it in the current code, but it might be worth it to keep it even without immediate uses)LEDPattern.scrollAtAbsoluteSpeed()
LEDPattern.blink()
(2 overloads)LEDPattern.breathe()
LEDPattern.atBrightness()
TimedRobot.addPeriodic()
(2 overloads)new ChassisSpeeds()
ChassisSpeeds.discretize()
ChassisSpeeds.fromFieldRelativeSpeeds()
ChassisSpeeds.fromRobotRelativeSpeeds()
new DifferentialDriveOdometry()
DifferentialDriveOdometry.resetPosition()
new DifferentialDriveWheelSpeeds()
DifferentialDriveWheelSpeeds.desaturate()
new MecanumDriveWheelPositions()
new MecanumDriveWheelSpeeds()
SwerveDriveKinematics.desaturateWheelSpeeds()
new SwerveDriveKinematics()
new TrajectoryConfig()
TrajectoryConfig.setStartVelocity()
TrajectoryConfig.setEndVelocity()
new TrapezoidProfile.State()
public SwerveModuleState(Measure<Velocity<Distance>> speed, Rotation2d angle) { | ||
this(speed.in(MetersPerSecond), angle); | ||
public SwerveModuleState( | ||
Measure<? extends PerUnit<DistanceUnit, TimeUnit>> speed, Rotation2d angle) { |
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.
Just out of curiosity, is there some reason this can't be Measure<LinearVelocityUnit>
or maybe Measure<VelocityUnit<DistanceUnit>>
?
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.
Because of Java generic interoperability. This constructor accepts a measure of any unit that extends from PerUnit<DistanceUnit, TimeUnit
- which means LinearVelocity
, Per<DistanceUnit, TimeUnit>
, or Velocity<DistanceUnit>
would all be acceptable input types
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.
The downside to wildcard bounds is that you can't use in
, since the compiler doesn't know what specific subtype will be stored in the measure
Thanks for checking it over. I added the PerUnit<VoltageUnit, DistanceUnit> VoltsPerMeter = Volts.per(Meter);
var measure = VoltsPerMeter.ofNative(1.0); // => returns Per<VoltageUnit, DistanceUnit>
measure.in(VoltsPerMeter); // No cast is required Which, if used instead of I believe it'd actually be better to use the unit-specific types at first, and relax later if necessary. |
Allows for more obvious types for readability and access to type-specific methods that would otherwise be unusable (eg `Frequency.asPeriod()`) Some places must still be generic (eg trapezoid profile constraints and states)
Java generics are too limited to do what we need. This refactors generic code previously in
Unit
andMeasure
into unit-specific classes that can have unit-safe math operations (notably,times
anddivide
) that can return values in known units instead of a wildcardedMeasure<?>
Unit-specific measure implementations are automatically generated by
./wpiunits/generate_units.py
, which generates generic interfaces and mutable and immutable implementations of those interfaces. These make up the bulk of the diff of this PR (approximately 9300 LOC)This also adds units for angular and linear velocities, accelerations, and momenta; moment of inertia; and torque
Supersedes #6684, #6696, #6710, #6778