Clalancette/revamp time #5
Conversation
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
clalancette
force-pushed
the
clalancette/revamp-time
branch
from
c2c1462 to
8826dc9
Compare
|
Rebased onto the latest now that #3 has been merged. |
| return new Time(0, nanos, this.clockType); | ||
| } | ||
|
|
||
| public boolean getRosTimeIsActive() { |
There was a problem hiding this comment.
nit: I find isRosTimeActive() more readable
There was a problem hiding this comment.
I did this to be more consistent with the rest of the rcljava code, particularly the get and set methods in the generated message files. But I don't feel strongly about it either way.
| return nativeRosTimeOverrideEnabled(this.handle); | ||
| } | ||
|
|
||
| public void setRosTimeIsActive(boolean enabled) { |
There was a problem hiding this comment.
nit: I find setRosTime more readable
There was a problem hiding this comment.
Same as above.
| if (!this.node.hasParameter("use_sim_time")) { | ||
| this.node.declareParameter(new ParameterVariant("use_sim_time", false)); | ||
| } |
There was a problem hiding this comment.
There is a TOCTTOU race condition here.
The problem exists in other clients (e.g. rclcpp), so I don't care much.
There was a problem hiding this comment.
Yeah, this is a race. It happens early in the process of creating a node, so it is fairly unlikely to happen. I guess we could avoid the race by unconditionally calling declareParameter, and handling the exception if it fails. Thoughts?
There was a problem hiding this comment.
I guess we could avoid the race by unconditionally calling declareParameter, and handling the exception if it fails.
SGTM
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
Signed-off-by: Chris Lalancette <clalancette@openrobotics.org>
|
As discussed in person, closing this PR and reopening from a branch on the osrf repository. |
| if (!this.node.hasParameter("use_sim_time")) { | ||
| this.node.declareParameter(new ParameterVariant("use_sim_time", false)); | ||
| } |
There was a problem hiding this comment.
I guess we could avoid the race by unconditionally calling declareParameter, and handling the exception if it fails.
SGTM
2 participants
This PR does the bare minimum to implement 'use_sim_time' in rcljava. By 'bare minimum', I mean:
Timeclass to be more like the rclcpp/rclpy equivalents.Clockclass to be more like the rclcpp/rclpy equivalents.TimeSourceclass, which is responsible for setting up the 'use_sim_time' parameter and subscribing to the '/clock' topic.Timeclass were added.ClockorTimeSourcewere added.Nevertheless, I think this is a good starting point to finish implementing 'use_sim_time'. The next steps here are to write tests, fix the TODOs, and then test that 'use_sim_time' actually works.
FYI, this builds on top of #3 , so some of those commits are in here as well. Once #3 is reviewed/merged, this one should be rebased.
@jacobperron @ivanpauno FYI.