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

Node unable to avoid using sim time if /clock published #515

Closed
dhood opened this issue Jul 17, 2018 · 4 comments
Closed

Node unable to avoid using sim time if /clock published #515

dhood opened this issue Jul 17, 2018 · 4 comments
Assignees
Labels
question Further information is requested

Comments

@dhood
Copy link
Member

dhood commented Jul 17, 2018

Even if a node has its use_sim_time parameter set to false, if a message is received by the TimeSource on the /clock topic, the node's clock will start using sim time.

This branch has added a test that fails but I would expect to pass: https://github.com/ros2/rclcpp/blob/07567dfbcd56803d0cb5ff8afe23e6b3171d02b6/rclcpp/test/test_time_source.cpp#L339..L341

The presence of a publisher on the clock topic currently causes nodes with unset use_sim_time parameter value to start using sim time (see #514 if you want to discuss that), but I don't think it should happen for those that have explicitly set the parameter to false.

If others agree we can label this as a bug and then only enable ROS time upon receipt of /clock msg if the parameter has not explicitly been set. That would keep the rest of the existing behaviour the same (again, if you want to discuss the behaviour on unset use_sim_time parameter, see #514).

@dhood dhood added the question Further information is requested label Jul 17, 2018
@dhood dhood self-assigned this Jul 17, 2018
@wjwwood
Copy link
Member

wjwwood commented Jul 17, 2018

In my opinion, it should definitely be opt-in (presence of a publisher on /clock is not sufficient), though maybe a one time warning is appropriate if use_sim_time is unset but there is a publisher (it might be an indication that there is a configuration issue). Explicitly setting use_sim_time to False could disable the warning.

@sloretz
Copy link
Contributor

sloretz commented Jul 17, 2018

+1 to opt-in

Is a /clock subscription necessary when use_sim_time is false? Not having it would prevent a warning like @wjwwood described. It also would add a delay if the subscription had to be created later when use_sim_time is dynamically enabled, but I'm not sure if that's a problem.

@tfoote
Copy link
Contributor

tfoote commented Jul 25, 2018

The behavior in ROS1 is to not even subscribe unless the parameter is set. It's actually required that the parameteter is set before startup as the subscription is only created during the startup phase of a ROS1 node. I implemented this off of memory from the first implementation of ROS1 time but that evolved since that generation. As I mentioned in #514 we might want to take advantage of the parameter callbacks and support switching based on the parameter instead of requiring it at startup. Code can be robust to the change in time by using the TimeJump callbacks.

@dhood
Copy link
Member Author

dhood commented Sep 25, 2018

#559 made it so time source's use of sim time only depends on the use_sim_time parameter, not the presence of a clock publisher. This test was added to check that sim time doesn't activate even if a clock publisher is present if the use_sim_time parameter is set to false.

@dhood dhood closed this as completed Sep 25, 2018
nnmm pushed a commit to ApexAI/rclcpp that referenced this issue Jul 9, 2022
Signed-off-by: Ivan Santiago Paunovic <ivanpauno@ekumenlabs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants