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

Java units API rewrite #6958

Merged
merged 47 commits into from
Sep 7, 2024
Merged

Conversation

SamCarlberg
Copy link
Member

Java generics are too limited to do what we need. This refactors generic code previously in Unit and Measure into unit-specific classes that can have unit-safe math operations (notably, times and divide) that can return values in known units instead of a wildcarded Measure<?>

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

@SamCarlberg SamCarlberg added the component: wpiunits Java units library label Aug 13, 2024
Copy link
Contributor

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

@SamCarlberg SamCarlberg marked this pull request as ready for review August 13, 2024 21:50
@SamCarlberg SamCarlberg requested review from a team as code owners August 13, 2024 21:50
@spacey-sooty
Copy link
Contributor

Can't speak for the internals but the new API looks much nicer 🙂
Can't wait to use it for my team!

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);
Copy link
Member

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?

@narmstro2020
Copy link
Contributor

Java generics are too limited to do what we need. This refactors generic code previously in Unit and Measure into unit-specific classes that can have unit-safe math operations (notably, times and divide) that can return values in known units instead of a wildcarded Measure<?>

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

Never been happier to have been superseded. Good work

@PeterJohnson
Copy link
Member

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();
Copy link
Member Author

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>

@viggy96
Copy link
Contributor

viggy96 commented Aug 29, 2024

Measurands (which is what users often interact with, what variable types are, etc) is simply the raw name now (eg “Current”), so to disambiguate it makes sense that the actual unit types need to have suffixes (“CurrentUnit”). The other approach would be to add a suffix to the measurand, and be more verbose there (“CurrentMeasure”).

I see, so from a programmer perspective, we still get the raw name?

@SamCarlberg
Copy link
Member Author

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);

PeterJohnson
PeterJohnson previously approved these changes Sep 2, 2024
Copy link
Contributor

@KangarooKoala KangarooKoala left a 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.

Allows for wider compatibility with results of math operations
Allows for known `Per` object outputs instead of bounded wildcard `Measure<? extends PerUnit>`
PeterJohnson
PeterJohnson previously approved these changes Sep 3, 2024
Copy link
Contributor

@KangarooKoala KangarooKoala left a 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 handle asPeriod(), 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) {
Copy link
Contributor

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>>?

Copy link
Member Author

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

Copy link
Member Author

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

@SamCarlberg
Copy link
Member Author

Did a search for Measure< through the diff and found the following parameters that still use X instead of Measure<XUnit>

Thanks for checking it over.

I added the .ofNative method later to allow for a unit-specific object to be returned:

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 .of, means we have more information at compile time and won't be dealing with weird units. Which would mean wildcard bounds wouldn't be necessary. If it turns out we need generic parameters in the future, we can relax the types while still maintaining compatibility. But that won't be possible in the other direction (convert generic Measure to concrete types like Time or Voltage).

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)
@PeterJohnson PeterJohnson merged commit a9b8850 into wpilibsuite:main Sep 7, 2024
35 checks passed
@SamCarlberg SamCarlberg deleted the unit-flattening branch September 7, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: wpiunits Java units library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants