WIP: Allow to pass existing TimeSource via NodeOptions
#2673
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roncapat
force-pushed
the
feat/shared_time_source_1
branch
3 times, most recently
from
e88dff2
to
a7d891f
Compare
Signed-off-by: Patrick Roncagliolo <[email protected]>
Signed-off-by: Patrick Roncagliolo <[email protected]>
roncapat
force-pushed
the
feat/shared_time_source_1
branch
from
edd5b50
to
31f87f6
Compare
|
@roncapat is this still in draft / WIP status? |
|
@fujitatomoya works nicely for me locally, but I'd like at least a review to understand what are your opinion on this / if I need to change something. |
There was a problem hiding this comment.
@roncapat thanks for the PR and feedback.
a several comments for now, i think we need to add more unit tests to verify this feature.
- if the
TimeSourceis shared withuse_clock_thread_disabled, this main Node must be spinning to update the clock for everybody else, otherwise other nodes sharingTimeSourcecannot get the update for the simulation clock information. user application must understand this condition. - i think if the
TimeSourceis shared, we cannot reset this in the subordinate Nodes during destruction. only main Node can do the reset once theTimeSourceis no longer required to keep? - The subordinate Nodes will not have
<nodename>/use_sim_timeparameter neither clock subscription QoS parameters, but only main Node. i think this is also important for user to know, i would suggest doc section in the code and ros2_documentation. - unit test required to make sure
TimeSourceis shared within multiple Nodes instances w/ and w/o simulation time. (currently only copy assignment test is added.) LifecycleNodesupport is missing? or is this on purpose for any constraints?
| @@ -235,6 +237,7 @@ Node::Node( | |||
| sub_namespace_(""), | |||
| effective_namespace_(create_effective_namespace(this->get_namespace(), sub_namespace_)) | |||
| { | |||
| node_time_source_->attachClock(node_clock_); | |||
There was a problem hiding this comment.
this actually tries to insert the same element node_cloeck_ if TimeSource is not specified in NodeOption, but that is just fine since std::unordered_set checks the hash to avoid duplication.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Addresses #2659 (task 1).
This allows to put an existing
TimeSourceinNodeOptionsso that a Node in the same process space can avoid re-creating aTimeSource.For example, the
TimeSourcemay be extracted viarclcpp::Node::get_node_time_source_interface()from an existing node, and passed viaNodeOptionsto all the others that share the process space.This is, for example, how I tested it locally in an application of mine.
Main benefit: a single subscriber can be shared when using
use_sim_time: better scaling for large rclcpp-composed simulations.