-
Notifications
You must be signed in to change notification settings - Fork 337
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
Resolve premature controller initialization #213
Resolve premature controller initialization #213
Conversation
I reverted the original commit in this PR and replaced it with a different solution to the issue. It basically avoids calling Feel free to squash merge if you're happy with the final result. |
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.
one nitpick, otherwise this looks good to me.
Works great for me. |
Ping @gavanderhoorn, @ipa-nhg. Any chance you can review this? Not sure why the tests are marked yellow here, since they show as passed in Travis CI. |
I've restarted the build. Travis can be a bit strange sometimes. |
@miguelprada: this seems like it should fix running controllers without knowing the current state of the robot, but may I suggest we change it for readability to check So something like: bool ROSController::update()
{
// don't run controllers if we haven't received robot state yet
if (!state_initialized_)
return;
...
} I'm assuming the compiler will in-line the call to |
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 would suggest using robot_state_received_
as the name for the variable. state_initialized_
is too generic and doesn't convey the fact that the variable really captures whether the driver has received the initial state from the robot.
- Renames state_initialized_ as robot_state_received_ - Relocates the check into ROSController::update
I believe all requests have been fulfilled. |
Thanks @miguelprada for the fix and for iterating on it with me. 👍 |
In my (admittedly limited) tests this resolves #206.
It partially reverts the changes in Zagitta#6, in particular the code that ensures the controllers are updated at a minimum rate of 125Hz.