Skip to content

Instantly share code, notes, and snippets.

@calcmogul
Created November 19, 2021 18:57
Show Gist options
  • Star 0 You must be signed in to star a gist
  • Fork 0 You must be signed in to fork a gist
  • Save calcmogul/cbc13a635526a7a960ae757ecb4792c9 to your computer and use it in GitHub Desktop.
Save calcmogul/cbc13a635526a7a960ae757ecb4792c9 to your computer and use it in GitHub Desktop.

Point2d is a duplicate of Translation2d, but with fewer features.

The following functions should just take a Trajectory, not the internal vector of states type. https://github.com/VexWPILib/vpi/blob/main/vpi/include/vpi/controller/SimpleTrajectoryFollower.h#L21-L25

The following functions feel bolted on and don't really follow the original purpose of "basic inverse kinematics for teleop driving". It also provides multiple ways to do the same thing, and we like to follow the Zen of Python's "There should be one-- and preferably only one --obvious way to do it.". https://github.com/VexWPILib/vpi/blob/main/vpi/include/vpi/drive/DifferentialDrive.h#L121-L161

The "IK()" functions were deliberately made public so the user could insert their own operations between the inverse kinematic operation and the motor setting. The non-IK functions are wrappers around the IK functions for those who don't feel like calling the motor setters themselves.

What was the reasoning for switching from the original units library to OkapiLib? The user-defined literals are similar, but there's several cases where usage is really verbose. https://github.com/VexWPILib/vpi/blob/main/vpi/include/vpi/filter/SlewRateLimiter.h#L26-L30

The library author has informed me this isn't in the spirit of the library, although I've never used it so I don't know what correct usage looks like.

Taking constructor arguments by pointer is frowned upon. Prefer references as it keeps the user's syntax clean. https://github.com/VexWPILib/vpi/blob/main/vpi/include/vpi/controller/SimpleWaypointFollower.h#L16

PIDController was designed to be run synchronously instead of asynchronously with built-in thread safety (the C++ design maxim of "don't pay for what you don't use"). We've found it's better for the caller to handle synchronization, if they need to at all. https://github.com/VexWPILib/vpi/blob/main/vpi/include/vpi/pid/PIDController.h WPILib uses TimedRobot::AddPeriodic() to schedule higher rate tasks in a cooperative multitasking manner, so no thread synchronization is needed. Besides, the roboRIO only has two cores, so it's not like many threads can run truly in parallel anyway.

Also, feedforward was explicitly excluded from PIDController since PIDController is a feedback controller. It's much cleaner for composability and customizability reasons to have separate feedforward classes, which we do.

Since you have your own versions of std::clamp here and std::make_unique here, I'm assuming you only have access to C++11. That's rather unfortunate. Structured bindings from C++17 makes user code for multi-return-value functions like kinematics cleaner.

@dcieslak19973
Copy link

Good points; I'm thinking thru still the best way to address Point2d/Translation2d; I think users may not intuitively make the connection that a Translation can be a Point.

I compromised a bit on including the DriveTo/TurnTo methods; these are present in other VRC drivetrains and felt giving the user a familiar starting point was worth it.

I'll play around some with the WPI units, I was more familiar with OkapiLib's and like that they will give compile time errors if one starts making unit mistakes. It wasn't obvious if the WPI ones did the same, and, admittedly, I didn't really dig into them.

I appreciate the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment