Expand auto to the current time when passed to a Header field

esteve commented 2 years ago

This PR enables expansion of auto to a generated Header whose stamp field is set to the current time. This also expands now to the current time if the field is of type builtin_interfaces.msg.Time

These changes match the same behavior that rostopic has:

http://wiki.ros.org/ROS/YAMLCommandLine#Headers.2Ftimestamps

Closes #700

fujitatomoya commented 2 years ago

do we want this as option via command line like ros2 topic pub --with-current-time? if message does not have the field, probably fallback to ignore that option 1st. this way we can avoid the overhead for current users? (it seems that populating field every single time via timer callback would not be cost effective?) what do you think?

@clalancette @ivanpauno could you share your thought?

esteve commented 2 years ago

@fujitatomoya Thanks for the feedback.

@esteve do we want this as option via command line like ros2 topic pub --with-current-time? if message does not have the field, probably fallback to ignore that option 1st. this way we can avoid the overhead for current users? (it seems that populating field every single time via timer callback would not be cost effective?) what do you think?

I thought about that, but it always felt to me that --substitute-keywords in ROS1's rostopic (which would be the equivalent of --with-current-time) was a workaround for https://github.com/ros/ros_comm/issues/55

In any case, I agree with you that for large messages updating the message very time might not be the best. I'll look into optimizing the code so that only timestamps are updated, and the rest of the message is left intact. However, I do think that the default behavior for ros2 topic pub should be to always update timestamps with current time if now is passed as a value, with no extra flags.

fujitatomoya commented 2 years ago

@esteve thanks for sharing information.

basically, as user interface, i think

ros2 topic pub option --with-current-time is easy to use, but this overrides all fields stamp and builtin_interfaces.msg.Time. If user wants to insert the current time in specific field of the message, it cannot support.
auto and now statements in message field can allow user to manage the message field, more flexible.

i believe auto and now statements are okay.

large messages updating the message very time might not be the best. I'll look into optimizing the code so that only timestamps are updated, and the rest of the message is left intact.

as implementation, i do not have any better way to do this at this moment, sorry.

personally, if it requires performance, i will built the binary instead of using script or shell. i think this can be acceptable, but just thought about the user who is not interested in this feature would want to avoid this overhead.

esteve commented 2 years ago

@fujitatomoya thanks for approving this PR, but I'll work today on optimizing it and adding a test

esteve commented 2 years ago

@fujitatomoya I've added a bunch of tests, the rest of the logic in the set_message_fields_expanded function is already tested in rosidl_runtime_py.set_message_fields

I've optimized the logic for expanding now and auto so that only the appropriate builtin_interfaces.msg.Time fields are updated, and the the rest of the message is left intact, thus avoiding any extra overhead when continuously publishing messages.

Additionally, with this PR, you can pass now to any builtin_interfaces.msg.Time field and it'll be expanded to the current time, not only the stamp in the header.

esteve commented 2 years ago

@fujitatomoya I need to make some more changes, I just realized that set_message_fields_expanded can be simplified a bit more, it doesn't really need a Node argument if the timestamps are updated elsewhere.

esteve commented 2 years ago

@fujitatomoya @ivanpauno can you have another look? I'm ok with merging the logic in set_message_fields_expanded with rosidl_runtime_py.set_message_fields, I'll submit a PR to rosidl_runtime_py, but I want to make sure that the rest of the code here is ok.

esteve commented 2 years ago

@fujitatomoya @ivanpauno here's the PR for the changes in rosidl_runtime_py https://github.com/ros2/rosidl_runtime_py/pull/19

fujitatomoya commented 2 years ago

@esteve is this still draft?

esteve commented 2 years ago

@fujitatomoya thanks for the reminder. This is now ready, but I asked @ivanpauno to expand on his feedback.