-
Notifications
You must be signed in to change notification settings - Fork 0
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
Converts the teeth amount of a gear to the pitch diameter. #72
base: main
Are you sure you want to change the base?
Conversation
Can we add units to all of the methods please? |
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.
First pass feedback
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.
We need to consistently name these functions. I propose renaming the functions to follow the format pitchDiameterFromMechanismName
since that seems intuitive
For example:
pitchDiameterFrom3mmPulley
or pitchDiameterFrom25ChainSprocket
package coppercore.math; | ||
|
||
public class GearConversionFunctions { | ||
public static double pitchPulley3mm(int teeth) { |
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.
Per #52 all these functions should return a Distance, which is a unit from wpilib. So that needs to be added to all functions.
Also you'll need to include the following in your build.gradle in the dependencies
block
def wpilibVersion = "2025.1.1-beta-1"
implementation "edu.wpi.first.wpiunits:wpiunits-java:$wpilibVersion"
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.
Comparing floating point numbers with equality is generally considered bad practice. assertEquals
should have an option to specify an epsilon, and I recommend setting that to 1e-10.
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.
To use it just provide a third parameter which it the delta
No description provided.