Resolve startup race condition for sim time #608
Conversation
first step to resolve #595
Second fix for #595 This was causing it to require /clock topic data or a parameter change becaue the clock was attached after the node was attached. Doing it in the other order would have worked. Also homogenizing the behavior to use the last recieved value otherwise zero time when enabling sim time.
|
tfoote
added
in review
tfoote
changed the title
rclcpp/include/rclcpp/node_interfaces/node_time_source_interface.hpp
Outdated
Show resolved
Hide resolved
Co-Authored-By: tfoote <tfoote@osrfoundation.org>
…ce.hpp Co-Authored-By: tfoote <tfoote@osrfoundation.org>
|
Resolved comments and rerunning CI for mac and windows. The windows tests looked to be a bunch from other issues.
@ros-pull-request-builder retest this please |
|
The original sim time test in |
|
After debugging a bit with @tfoote, it looks that my problem may be coming from having I'll make a PR changing the sim time test to work with this and we can run CI with the combination. |
|
This PR has been tested as working against the new navigation stack by @crdelsey #595 (comment) |
| @@ -0,0 +1,50 @@ | |||
| // Copyright 2017 Open Source Robotics Foundation, Inc. | |||
There was a problem hiding this comment.
this could probably still be fixed
| { | ||
|
|
||
| /// Pure virtual interface class for the NodeTimeSource part of the Node API. | ||
| class NodeTimeSourceInterface |
There was a problem hiding this comment.
I think this might need a virtual destructor to make sure ~NodeTimeSource() gets called when the std::shared_ptr<NodeTimeSourceInterface> on the node is destructed.
There was a problem hiding this comment.
Yeah, this is probably true as well for all the other interface classes too. I went over it with @wjwwood and it looks like virtual default constructors are the best approch. We're clearly not using it right now but in the future not having them will cause warnings or errors.
|
Added virtual destructors for all interfaces in a7de819
|
|
|
||
| RCLCPP_PUBLIC | ||
| virtual | ||
| ~NodeBaseInterface() = default; |
There was a problem hiding this comment.
The destructor is conventionally at the top of the class just after any constructors.
* move destructors to top of class
|
Retrying windows after merging refactor
|
| @@ -0,0 +1,50 @@ | |||
| // Copyright 2017 Open Source Robotics Foundation, Inc. | |||
There was a problem hiding this comment.
this could probably still be fixed
|
@ros-pull-request-builder retest this please |
Resolves ros2#595 * Separate the Node Time Source from the Node Clock * Implement initial value checking of use_sim_time parameter parameter * Be sure to update all newly attached clocks * Homogenizing the behavior to use the last received value otherwise zero time when enabling sim time. * Add virtual destructors to interface classes
Signed-off-by: Mikael Arguedas <mikael.arguedas@gmail.com>
ros2#608) Signed-off-by: Emerson Knapp <eknapp@amazon.com>
Fixes #595
A race condition at startup was identified in #595 where the clock and parameter interfaces needed each other to initialize and since they could not rely on both being initialized the loop was closed through sending and receiving ROS messages. This however meant that initialization was dependent on messages being passed through ROS topics before time would be initialized correctly. The solution to this is to break apart the clock dependency into the time source and clock separately, like we already do for the underlying data and this breaks the cycle.
When reviewing this there's an extension which would be a good improvement to not rely on parameter events being published but use the internal node interface API to get the events in a callback. However as we're late in the release cycle I'm going to hold off on fixing that is this is functional just not optimal and would require a noteably large API change. Along with that change if there was a more fully featured API for registering callbacks for parameter changes locally, the parameter event publishing could also be split out into a separate component that could obviate the need for a parameter to turn that functionality on and off in the constructor of the parameter, and avoid the dependencies on the clock and topics interface for the core parameter API.