rclpy icon indicating copy to clipboard operation
rclpy copied to clipboard

Insulate /clock callback on specific thread

Open fujitatomoya opened this issue 4 years ago • 5 comments

Feature request

Feature description

rclpy follow-up of https://github.com/ros2/rclcpp/issues/1542.

the current implementation of simulation time which listen to the /clock topic, this behavior breaks the symmetry between the usage of real time (system time internally accessed without the need of subscribing to a topic, and hence always updated) and simulation time (time received on the /clock topic and hence frozen in a callback). the symmetry should be maintain in order to not draw false conclusions about the behavior of a code developed in simulation VS real time, and thus independently from good/bad code practice.

Implementation considerations

N.A

fujitatomoya avatar May 10 '21 02:05 fujitatomoya

Reproducible Sample

  • How to build
> cd <colcon_workspace>/src
> git clone [email protected]:fujitatomoya/ros2_test_prover.git
> cd <colcon_workspace>
> colcon build --symlink-install --packages-select prover_rclpy prover_rclpy
> source install/local_setup.bash
  • use_sim_time is NOT enabled. (default, real time)
root@134f29e8f25f:~/ros2_ws/colcon_ws# ros2 run prover_rclpy rclpy_792
[INFO] [1620620313.030122595] [test_node]: Timer expired!
[INFO] [1620620313.030652940] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620313, nanosec=30261016)
[INFO] [1620620313.131537800] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620313, nanosec=130820724)
[INFO] [1620620313.233182128] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620313, nanosec=232245229)
[INFO] [1620620313.334565491] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620313, nanosec=333647523)
[INFO] [1620620313.436254238] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620313, nanosec=435334840)
[INFO] [1620620313.537922876] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620313, nanosec=536981457)
[INFO] [1620620313.639613674] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620313, nanosec=638694465)
[INFO] [1620620313.741326942] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620313, nanosec=740385533)
[INFO] [1620620313.842932769] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620313, nanosec=841742538)
[INFO] [1620620313.944124850] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620313, nanosec=943217412)
[INFO] [1620620314.045225243] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620314, nanosec=44351785)
[INFO] [1620620315.014339364] [test_node]: Timer expired!
[INFO] [1620620315.015234882] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620315, nanosec=14510385)
[INFO] [1620620315.116864219] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620315, nanosec=115837950)
[INFO] [1620620315.218562187] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620315, nanosec=217637459)
...<snip>
  • use_sim_time is enabled.
# start simulation clock publisher
root@134f29e8f25f:~/ros2_ws/colcon_ws# ros2 run prover_rclcpp sim_clock_publisher

# start node with timer
root@134f29e8f25f:~/ros2_ws/colcon_ws# ros2 run prover_rclpy rclpy_792
[INFO] [1620620428.799579528] [test_node]: Timer expired!
[INFO] [1620620428.800055133] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620428, nanosec=788452724)
[INFO] [1620620428.900954073] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620428, nanosec=788452724)
[INFO] [1620620429.001873214] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620428, nanosec=788452724)
[INFO] [1620620429.102788923] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620428, nanosec=788452724)
[INFO] [1620620429.203695413] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620428, nanosec=788452724)
[INFO] [1620620429.304654575] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620428, nanosec=788452724)
[INFO] [1620620429.405622946] [test_node]: Current time: builtin_interfaces.msg.Time(sec=1620620428, nanosec=788452724)
...<snip>


fujitatomoya avatar May 10 '21 04:05 fujitatomoya

CC: @wjwwood @clalancette

fujitatomoya avatar May 10 '21 04:05 fujitatomoya

This PR needs #850. Otherwise a new Node would be needed and this would result in a cyclic dependency (Node -> TimeSource -> Node) and is additionally very ugly.

Flova avatar Mar 03 '22 18:03 Flova

Adapting TimeSource itself should be quite easy.

Flova avatar Mar 03 '22 18:03 Flova

I hacked together a solution for #850 and #792 which seems to work. The clock is updated on the same Node without an explicit spin call. It is done via a thread with a seperate spin in the TimeSource. The said spin uses a patched Executor which also accepts callback_groups in addition to nodes. The implementation is quite ugly at the moment, but it seems to work. I will do a PR. Feel free to comment comment on my implementation so we can improve it.

Flova avatar Mar 04 '22 02:03 Flova