-
Notifications
You must be signed in to change notification settings - Fork 110
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
Refactor pulse width calculation for kick into its own function #3351
base: master
Are you sure you want to change the base?
Refactor pulse width calculation for kick into its own function #3351
Conversation
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.
Nice work! A couple comments
* @param speed Speed of the desired kick in m/s | ||
* @return Width of pulse | ||
*/ | ||
uint32_t calculateChickerPulseWidth(int kick_constant, double kick_coefficient, |
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.
You'll need to mark this function as inline
. Functions defined in header files need to be marked as inline
to avoid violating the One Definition Rule
* @param kick_constant | ||
* @param kick_coefficient |
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.
These params should have descriptions. e.g. "the constant to use in the kick speed to pulse width conversion"
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.
Also, if you take a look at other functions where calculateChickerPulseWidth
is called, you'll see that kick_constant
, kick_coefficient
, chip_constant
, etc. are undocumented in the function docs. This is outside the scope of this PR, but it might be worth updating those function docs with descriptions for those params
…alculateChickerPulseWidth to be inline
…into duplicate_power_kick
7717838
to
7eaf4ce
Compare
The last software test (//software/embedded/ansible:requirements_test) timed out after 60 seconds, all others passed. I'm not sure why the auto-fixes failed. |
You can just rerun them if they fail for those reasons. //software/embedded/ansible:requirements_test is flaky on CI for some reason and the auto fixes check often fails after formatting. |
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.
⚡️
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.
Looks good to merge! You should pull master so that the other two checks pass on CI.
Please fill out the following before requesting review on this PR
Description
Combines duplicate calculations for the chipper's kick pulse width
Testing Done
Built and tested file using bazel
Resolved Issues
resolves #3193
Length Justification and Key Files to Review
Software/src/proto/message_translation/power_frame_msg.hpp
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.h
file) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom
. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO
(or similar) statements should either be completed or associated with a github issue