-
Notifications
You must be signed in to change notification settings - Fork 664
[wpimath] Add ChassisAccelerations and drivetrain accelerations classes and add forward and inverse kinematics for accelerations to the interface #8185
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
base: 2027
Are you sure you want to change the base?
Conversation
…r accelerations) and make both classes interpolatable Signed-off-by: Zach Harel <[email protected]>
Signed-off-by: Zach Harel <[email protected]>
Signed-off-by: Zach Harel <[email protected]>
…use it already had the protobuf) and fix toString format Signed-off-by: Zach Harel <[email protected]>
Signed-off-by: Zach Harel <[email protected]>
Signed-off-by: Zach Harel <[email protected]>
…celerations, and SwerveModuleAccelerations with relevant protobufs/structs Signed-off-by: Zach Harel <[email protected]>
…anumDriveWheelAccelerations, and SwerveModuleAccelerations with relevant protobufs/structs Signed-off-by: Zach Harel <[email protected]>
Signed-off-by: Zach Harel <[email protected]>
Signed-off-by: Zach Harel <[email protected]>
…classes to include new third generic parameter Signed-off-by: Zach Harel <[email protected]>
Signed-off-by: Zach Harel <[email protected]>
Signed-off-by: Zach Harel <[email protected]>
Signed-off-by: Zach Harel <[email protected]>
…them and also make gradle happy Signed-off-by: Zach Harel <[email protected]>
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.
Do we need to include adding interpolate()
to the wheel speeds classes (and wheel accelerations classes) in this PR? It's already pretty large and doesn't seem directly related.
wpimath/src/main/java/edu/wpi/first/math/kinematics/ChassisAccelerations.java
Outdated
Show resolved
Hide resolved
wpimath/src/main/java/edu/wpi/first/math/kinematics/ChassisAccelerations.java
Outdated
Show resolved
Hide resolved
wpimath/src/main/java/edu/wpi/first/math/kinematics/ChassisAccelerations.java
Outdated
Show resolved
Hide resolved
I included the interpolation here solely because it was also something I wanted to add and I didn't want to make another PR |
wpimath/src/main/java/edu/wpi/first/math/kinematics/ChassisAccelerations.java
Show resolved
Hide resolved
…celeration vs velocity Signed-off-by: Zach Harel <[email protected]>
…erations and related files Signed-off-by: Zach Harel <[email protected]>
…of angularAcceleration Signed-off-by: Zach Harel <[email protected]>
…rAcceleration and then Signed-off-by: Zach Harel <[email protected]>
…swerve (which includes angular velocity due to centripetal acceleration component) Signed-off-by: Zach Harel <[email protected]>
…y in module acceleration calculations Signed-off-by: Zach Harel <[email protected]>
wpimath/src/main/java/edu/wpi/first/math/kinematics/ChassisAccelerations.java
Outdated
Show resolved
Hide resolved
wpimath/src/main/java/edu/wpi/first/math/kinematics/SwerveDriveKinematics.java
Outdated
Show resolved
Hide resolved
wpimath/src/main/java/edu/wpi/first/math/kinematics/SwerveDriveKinematics.java
Outdated
Show resolved
Hide resolved
wpimath/src/main/java/edu/wpi/first/math/kinematics/SwerveDriveKinematics.java
Outdated
Show resolved
Hide resolved
wpimath/src/main/java/edu/wpi/first/math/kinematics/SwerveDriveKinematics.java
Outdated
Show resolved
Hide resolved
wpimath/src/main/java/edu/wpi/first/math/kinematics/SwerveDriveKinematics.java
Outdated
Show resolved
Hide resolved
…rivekinematics to its own method Signed-off-by: Zach Harel <[email protected]>
…ons for clarity Signed-off-by: Zach Harel <[email protected]>
wpimath/src/main/native/cpp/kinematics/MecanumDriveKinematics.cpp
Outdated
Show resolved
Hide resolved
wpimath/src/main/native/cpp/kinematics/MecanumDriveKinematics.cpp
Outdated
Show resolved
Hide resolved
wpimath/src/main/native/cpp/kinematics/MecanumDriveKinematics.cpp
Outdated
Show resolved
Hide resolved
wpimath/src/main/native/cpp/kinematics/MecanumDriveKinematics.cpp
Outdated
Show resolved
Hide resolved
…cs methods Signed-off-by: Zach Harel <[email protected]>
wpimath/src/main/native/cpp/kinematics/MecanumDriveKinematics.cpp
Outdated
Show resolved
Hide resolved
Signed-off-by: Zach Harel <[email protected]>
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.
Where's the derivation of the swerve drive kinematics math again?
wpimath/src/main/native/include/frc/kinematics/SwerveDriveKinematics.h
Outdated
Show resolved
Hide resolved
wpimath/src/main/native/include/frc/kinematics/SwerveDriveKinematics.h
Outdated
Show resolved
Hide resolved
wpimath/src/main/native/include/frc/kinematics/SwerveDriveKinematics.h
Outdated
Show resolved
Hide resolved
Signed-off-by: Zach Harel <[email protected]>
…erations method Signed-off-by: Zach Harel <[email protected]>
wpimath/src/main/java/edu/wpi/first/math/kinematics/SwerveDriveKinematics.java
Show resolved
Hide resolved
…iveKinematics Signed-off-by: Zach Harel <[email protected]>
wpimath/src/main/java/edu/wpi/first/math/kinematics/SwerveDriveKinematics.java
Outdated
Show resolved
Hide resolved
… javadoc Signed-off-by: Zach Harel <[email protected]>
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.
Great work! The first PRs always take the longest (and this one is already a really large change)- Thanks for sticking with it!
double linearAcceleration = Math.hypot(x, y); | ||
|
||
moduleAccelerations[i] = | ||
new SwerveModuleAccelerations(linearAcceleration, new Rotation2d(x, y)); |
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.
I just remembered that this will print a warning if x
and y
are both (sufficiently close to) zero:
allwpilib/wpimath/src/main/java/edu/wpi/first/math/geometry/Rotation2d.java
Lines 111 to 119 in 7a2a982
if (magnitude > 1e-6) { | |
m_cos = x / magnitude; | |
m_sin = y / magnitude; | |
} else { | |
m_cos = 1.0; | |
m_sin = 0.0; | |
MathSharedStore.reportError( | |
"x and y components of Rotation2d are zero\n", Thread.currentThread().getStackTrace()); | |
} |
(And yes, zero acceleration for a module is possible- For example, spinning in place around that module.)
In general, are polar coordinates really the best representation for a module's accelerations? From the SwerveDriveKinematics
side, it seems like Cartesian would be easier since we wouldn't need to worry about edge cases with zero acceleration, but I don't know if polar makes more sense on the generation side. (I think that the inconsistency with SwerveModulePosition
and SwerveModuleState
would be fine because those are directly related to encoder measurements while the acceleration is not.)
// We already know that our omega should be 2π from the previous test. Next, we need to | ||
// determine | ||
// the vx and vy of our chassis center. Because our COR is at a 45 degree angle from the center, |
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.
This isn't super important, but if we're going to end up making changes anyways, could you prevent this single awkward word? (One thing you could do is to briefly join the entire paragraph into one line and then format)
If this would be the only remaining change, it's probably fine to mark this as resolved and then someone can spin off another PR to fix this.
// FL (12, 12): radius angle = 135°, tangential = 45°, centripetal = -135° → total angle = | ||
// -144° | ||
// FR (12, -12): radius angle = 45°, tangential = -45°, centripetal = -135° → total angle = | ||
// 126° | ||
// BL (-12, 12): radius angle = 135°, tangential = 45°, centripetal = 45° → total angle = | ||
// -54° | ||
// BR (-12, -12): radius angle = -45°, tangential = 45°, centripetal = 135° → total angle = | ||
// 36° |
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.
Pity this didn't fit, it'd have been really nice if it did. If there's already changes to make that aren't just comment formatting, maybe try this?
// FL (12, 12): radius angle = 135°, tangential = 45°, centripetal = -135° → total angle = | |
// -144° | |
// FR (12, -12): radius angle = 45°, tangential = -45°, centripetal = -135° → total angle = | |
// 126° | |
// BL (-12, 12): radius angle = 135°, tangential = 45°, centripetal = 45° → total angle = | |
// -54° | |
// BR (-12, -12): radius angle = -45°, tangential = 45°, centripetal = 135° → total angle = | |
// 36° | |
// FL (12, 12): radius angle = 135°, tangential = 45°, centripetal = -135° | |
// → total angle = -144° | |
// FR (12, -12): radius angle = 45°, tangential = -45°, centripetal = -135° | |
// → total angle = 126° | |
// BL (-12, 12): radius angle = 135°, tangential = 45°, centripetal = 45° | |
// → total angle = -54° | |
// BR (-12, -12): radius angle = -45°, tangential = 45°, centripetal = 135° | |
// → total angle = 36° |
Up to you how you want to format it, this is just a suggestion.
.toArray(Rotation2d[]::new); | ||
|
||
assertAll( | ||
() -> assertEquals(totalAccel, moduleAccelerations[0].acceleration, 0.1), |
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.
Could the tolerances be changed from 0.1 to kEpsilon
? I'm not sure why some of the other tests use 0.1 instead of kEpsilon
, but it'd be good to specify a higher level of tolerance.
// FL (12, 12): radius angle = 135°, tangential = 45°, centripetal = | ||
// -135° → total angle = -144° FR (12, -12): radius angle = 45°, tangential = | ||
// -45°, centripetal = -135° → total angle = 126° BL (-12, 12): radius angle | ||
// = 135°, tangential = 45°, centripetal = 45° → total angle = -54° BR | ||
// (-12, -12): radius angle = -45°, tangential = 45°, centripetal = 135° → | ||
// total angle = 36° |
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.
Hm, not sure why the formatting got so messed up.
ChassisAccelerations and the drivetrain acceleration types are added in both Java and C++.
ChassisAccelerations
is basically justChassisSpeeds
but for accelerations!DifferentialDriveWheelAccelerations
,MecanumDriveWheelAccelerations
, andSwerveModuleAccelerations
are the acceleration equivalent of the drivetrain speeds types.In Java, the
Kinematics
interface now has an additional generic parameterA
which represents the accelerations, andtoChassisAccelerations
andtoWheelAccelerations
methods, which are implemented the same way astoChassisSpeeds
andtoWheelSpeeds
.Protobuf and struct classes were also added for all four classes in Java and C++.