Skip to content
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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

abnv-goyal
Copy link

No description provided.

Copy link
Contributor

@huttongrabiel huttongrabiel left a 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
Copy link
Contributor

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");
Copy link
Contributor

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.

Copy link
Contributor

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();
Copy link
Contributor

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.

Copy link
Contributor

@huttongrabiel huttongrabiel left a 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants