Skip to content

add timestamp in the header of publish msg - #147

Closed
yechun1 wants to merge 1 commit into
ros2:masterfrom
yechun1:ros2topic_pub
Closed

yechun1 wants to merge 1 commit into
ros2:masterfrom
yechun1:ros2topic_pub

Conversation

@yechun1

@yechun1 yechun1 commented Sep 7, 2018

Copy link
Copy Markdown
Contributor

publish msg with timestamp could be used to verify ros2opic delay command porting

Signed-off-by: Chris Ye chris.ye@intel.com

@tfoote tfoote added the in review Waiting for review (Kanban column) label Sep 7, 2018

@dirk-thomas dirk-thomas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please also try you patch with a message which doesn't have a header field. I would expect it to fail atm.

Comment thread ros2topic/ros2topic/verb/pub.py Outdated
# TODO(dhood): use sim time if parameter has been set on the node.
_clock = ROSClock()
_time_source = TimeSource(node=node)
_time_source.attach_clock(_clock)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of creating a new clock please use the clock already available in the node: clock = node.get_clock()

Comment thread ros2topic/ros2topic/verb/pub.py Outdated
print('publishing #%d: %r\n' % (count, msg))
curr_rostime = _clock.now().nanoseconds
msg.header.stamp.sec = int(curr_rostime * 1e-9)
msg.header.stamp.nanosec = int(curr_rostime % 1e9)

@dirk-thomas dirk-thomas Sep 7, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid doing this "calculation" I added a new method seconds_nanoseconds on the Time class in ros2/rclpy#235. Please use that one:

msg.header.stamp.sec, msg.header.stamp.nanosec = clock.now().seconds_nanoseconds

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think you'd use now().to_msg() here instead

@dirk-thomas dirk-thomas added enhancement New feature or request in progress Actively being worked on (Kanban column) and removed in review Waiting for review (Kanban column) labels Sep 7, 2018
@yechun1
yechun1 force-pushed the ros2topic_pub branch 2 times, most recently from 82b8df1 to 118fcbb Compare September 10, 2018 06:33
@yechun1

yechun1 commented Sep 10, 2018

Copy link
Copy Markdown
Contributor Author

@dirk-thomas thanks for review, updated the code. the patch depends on ros2/rclpy#235.

Pass verified on msg with header(sensor_msgs/Image) and without header(std_msgs/String)
$ ros2 topic pub /msg sensor_msgs/Image

publisher: beginning loop
publishing #1: sensor_msgs.msg.Image(header=std_msgs.msg.Header(stamp=builtin_interfaces.msg.Time(sec=0, nanosec=0), frame_id=''), height=0, width=0, encoding='', is_bigendian=0, step=0, data=[])
publishing #2: sensor_msgs.msg.Image(header=std_msgs.msg.Header(stamp=builtin_interfaces.msg.Time(sec=1536561756, nanosec=761655296), frame_id=''), height=0, width=0, encoding='', is_bigendian=0, step=0, data=[])
publishing #3: sensor_msgs.msg.Image(header=std_msgs.msg.Header(stamp=builtin_interfaces.msg.Time(sec=1536561757, nanosec=761586944), frame_id=''), height=0, width=0, encoding='', is_bigendian=0, step=0, data=[])

$ ros2 topic pub /msg std_msgs/String

publisher: beginning loop
publishing #1: std_msgs.msg.String(data='')
publishing #2: std_msgs.msg.String(data='')
publishing #3: std_msgs.msg.String(data='')

@dirk-thomas

Copy link
Copy Markdown
Member

Having the option to set the header timestamp to the current is great. If the user wan't to publish a specific message with a specific timestamp though this isn't possible anymore. I would recommend to add an option to conditionally control this "magic" step. Opt out is likely a good choice.

@yechun1

yechun1 commented Sep 11, 2018

Copy link
Copy Markdown
Contributor Author

when user set specific message, the time basically will not set to zero. Below is changes to fill timestamp if msg has header but the timestamp is not zero. is this ok?


--- a/ros2topic/ros2topic/verb/pub.py
+++ b/ros2topic/ros2topic/verb/pub.py
@@ -103,14 +103,24 @@ def publisher(
     print('publisher: beginning loop')
     count = 0
 
+    # check if the msg has header, and the header has specific timestamp
+    timestamp_auto_fill = False
+    if hasattr(msg, 'header'):
+       if (msg.header.stamp.sec == 0) and (msg.header.stamp.nanosec == 0):
+           timestamp_auto_fill = True
+       else:
+           timestamp_auto_fill = False
+
     def timer_callback():
         nonlocal clock
         nonlocal count
+        nonlocal timestamp_auto_fill
         count += 1
         if print_nth and count % print_nth == 0:
             print('publishing #%d: %r\n' % (count, msg))
 
-        if hasattr(msg, 'header'):
+        # add timestamp as ROS time if msg with header has no specific timestamp
+        if timestamp_auto_fill:
             msg.header.stamp.sec, msg.header.stamp.nanosec = clock.now().seconds_nanoseconds()
         pub.publish(msg)

@dirk-thomas

Copy link
Copy Markdown
Member

What if the user wants to intentionally publish a message with the timestamp being zero? I don't think tools should prohibit users from doing whatever they want.

@yechun1

yechun1 commented Sep 12, 2018

Copy link
Copy Markdown
Contributor Author

Oh. Do you mean to add optional argument like "--timestamp" "-t" ?

@dirk-thomas

Copy link
Copy Markdown
Member

Do you mean to add optional argument like "--timestamp" "-t" ?

Yes, the ROS 1 tool rostopic pub offer the option -s / --substitute-keywords:

When publishing with a rate, performs keyword ('now' or 'auto') substitution for each message

Since that seems to work fine it might be best to follow this approach.

@dirk-thomas

dirk-thomas commented Sep 12, 2018

Copy link
Copy Markdown
Member

I would suggest to rather implement the same option as in ROS 1 as it is strictly more flexible / capable.

@yechun1

yechun1 commented Sep 17, 2018

Copy link
Copy Markdown
Contributor Author

the -s implementation depended on genpy library which used genpy.message.fill_message_args(). Is there any replacement api on ROS2 now? Do we have plan in the future to port ROS messages and services generators (genpy/gencpp/genlisp ...)?

@dirk-thomas

Copy link
Copy Markdown
Member

Message generation is very different between the ROS versions. The packages in ROS 2 are called e.g. rosidl_generator_cpp|py. If some of the logic from these ROS 1 packages is needed in ROS 2 then it can be ported to the existing ROS 2 packages. Please consider to create a pull request for those of they could be used for this ticket

@yechun1
yechun1 force-pushed the ros2topic_pub branch 3 times, most recently from a1ad5df to 416e9a2 Compare September 20, 2018 06:06
When publishing with a rate, performs keyword ('now' or 'auto') substitution for each message. Implemented the same option as in ROS1.

Signed-off-by: Chris Ye <chris.ye@intel.com>
@yechun1 yechun1 closed this Jan 10, 2019
@yechun1
yechun1 deleted the ros2topic_pub branch January 10, 2019 07:29
@tfoote tfoote removed the in progress Actively being worked on (Kanban column) label Jan 10, 2019
@yechun1

yechun1 commented Jan 10, 2019

Copy link
Copy Markdown
Contributor Author

This patch is not necessary. closed it.

esteve pushed a commit to esteve/ros2cli that referenced this pull request Dec 16, 2022
* Fix ros2 launch on Windows

argcomplete it doesn't support Windows. Moved the `argcomplete` outside the try catch

Signed-off-by: ahcorde <ahcorde@gmail.com>

* Fixed comment on ros2launch

Signed-off-by: ahcorde <ahcorde@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants