-
Notifications
You must be signed in to change notification settings - Fork 4
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
Change 300ms time delay to a while loop #341
base: master
Are you sure you want to change the base?
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.
Really good! Just a couple minor things
src/TunePID.cpp
Outdated
std::this_thread::sleep_for(300ms); // wait for encoder position data to arrive | ||
int32_t starting_angle = can::motor::getMotorPosition(serial); | ||
time_point<steady_clock> start = steady_clock::now(); | ||
int timeout = 300; // in milliseconds |
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.
Quick couple of things, since this int
is constant it should really be constexpr int timeout = 300ms
. Note the ms at the end so you don't have to specify it in comments.
if (starting_angle_data_point.isValid()) { | ||
starting_angle = starting_angle_data_point.getData(); | ||
} else { | ||
LOG_F(WARNING, "STARTING ANGLE DATA NOT FOUND"); |
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'm realizing that I said to log here but you can probably just exit with an error. This is the TunePID executable which isn't mission critical.
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.
but still log
src/TunePID.cpp
Outdated
|
||
int32_t starting_angle; | ||
if (starting_angle_data_point.isValid()) { | ||
starting_angle = starting_angle_data_point.getData(); |
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 just do int32_t starting_angle = ...
here instead of declaring it above. It's ok in this case since we will fail if we don't enter this block.
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! Just a few things to fix
No description provided.