-
Notifications
You must be signed in to change notification settings - Fork 2
Query Node Parameters #26
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
Conversation
Signed-off-by: Troy Gibb <[email protected]>
Signed-off-by: Troy Gibb <[email protected]>
Signed-off-by: Troy Gibb <[email protected]>
Signed-off-by: Troy Gibb <[email protected]>
Signed-off-by: Troy Gibb <[email protected]>
Signed-off-by: Troy Gibb <[email protected]>
Signed-off-by: Troy Gibb <[email protected]>
Signed-off-by: Troy Gibb <[email protected]>
Signed-off-by: Troy Gibb <[email protected]>
Signed-off-by: Troy Gibb <[email protected]>
Signed-off-by: Troy Gibb <[email protected]>
Signed-off-by: Troy Gibb <[email protected]>
Signed-off-by: Troy Gibb <[email protected]>
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.
Pretty minor comments, mostly nonblocking style comments, just want to resolve one or two API questions before we move forward
Signed-off-by: Troy Gibb <[email protected]>
Signed-off-by: Troy Gibb <[email protected]>
Signed-off-by: Troy Gibb <[email protected]>
Signed-off-by: Troy Gibb <[email protected]>
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.
Definitely some followups i want us to make, but we can merge this and work forward. I can take a pass on some of it, and it sounds like your ParameterValue PR addresses some of the rest
Description
Closes #28
Populate the
/rosgraph
with parameter names for each node.This change had to be done carefully as to not block the main loop tracking node updates, instead preferring to launch on a separate thread (
std::launch::async
) and holding onto the futures in a separate data structure (params_futures
) so the promises didn't go out of scope as our node introspection logic looped. Currently the parameters querying logic loops indefinitely until the parameters are received (or the node is brought down)Regarding query retries as mentioned here, I opted instead for a long wait time for the service client and service call to resolve (5 seconds), which felt functionally equivalent. Open to alternatives here.
Out of scope for this change:
/parameter_events
per node. I could see the case for including them here, but I thought the change large enough already, and the parameter events are only really useful when the values are updated.Test Plan
I've added the basic happy path tests here, expecting the basic params to be populated in subsequent graph message updates.
Acquiring the correct published rosgraph message was tricky in assertions that incrementally requested the latest
/rosgraph
message and popped the next from queue. I discovered that incremental requests of the message resulted in flakiness across ~100 test reruns (--gtest_repeat=100
). Instead, I opted to add some conditional await logic (await_graphmon_msg_until
).