Fix time behavior #691

kenji-miyake · 2021-12-10T06:05:26Z

Public API Changes

To be discussed.

Description

Split from #646.

Since the current code doesn't pass the tests, I fixed it.
However, this deletes a lot of code, so should be discussed carefully.

How to test

Time Array

Before(upstream/ros2)

$ # Terminal 1
$ ros2 run rosbridge_server rosbridge_websocket
[ERROR 1639116972.608771088] [rosbridge_websocket]: [Client 0] [id: publish:/time:3] publish: The 'times' field must be a set or sequence and each value of type 'Time'
[ERROR 1639116973.613656065] [rosbridge_websocket]: [Client 0] [id: publish:/time:4] publish: The 'times' field must be a set or sequence and each value of type 'Time

$ # Terminal 2
$ curl -sL https://github.com/tier4/rosbridge_suite/raw/add-test-scripts/test_scripts/time_array.py | python
publish: {'times': [{'sec': 1, 'nanosec': 2}, {'sec': 3, 'nanosec': 4}]}
publish: {'times': [{'sec': 1, 'nanosec': 2}, {'sec': 3, 'nanosec': 4}]}

After(this PR)

$ # Terminal 1
$ ros2 run rosbridge_server rosbridge_websocket
// No error

$ # Terminal 2
$ curl -sL https://github.com/tier4/rosbridge_suite/raw/add-test-scripts/test_scripts/time_array.py | python
publish: {'times': [{'sec': 1, 'nanosec': 2}, {'sec': 3, 'nanosec': 4}]}
subscribe: {'times': [{'sec': 1, 'nanosec': 2}, {'sec': 3, 'nanosec': 4}]}
publish: {'times': [{'sec': 1, 'nanosec': 2}, {'sec': 3, 'nanosec': 4}]}
subscribe: {'times': [{'sec': 1, 'nanosec': 2}, {'sec': 3, 'nanosec': 4}]}

Time (new format of `sec/nanosec`)

Before(upstream/ros2)

$ # Terminal 1
$ ros2 run rosbridge_server rosbridge_websocket
// No error

$ # Terminal 2
$ curl -sL https://github.com/tier4/rosbridge_suite/raw/add-test-scripts/test_scripts/time_new_format.py | python
publish: {'sec': 1, 'nanosec': 2}
subscribe: {'sec': 1, 'nanosec': 2}
publish: {'sec': 1, 'nanosec': 2}
subscribe: {'sec': 1, 'nanosec': 2}

After(this PR)

$ # Terminal 1
$ ros2 run rosbridge_server rosbridge_websocket
// No error

$ # Terminal 2
$ curl -sL https://github.com/tier4/rosbridge_suite/raw/add-test-scripts/test_scripts/time_new_format.py | python
publish: {'sec': 1, 'nanosec': 2}
subscribe: {'sec': 1, 'nanosec': 2}
publish: {'sec': 1, 'nanosec': 2}
subscribe: {'sec': 1, 'nanosec': 2}

Time (old format of `secs/nsecs`)

Before(upstream/ros2)

$ # Terminal 1
$ ros2 run rosbridge_server rosbridge_websocket
[ERROR 1641824688.064981748] [rosbridge_websocket]: [Client 2] [id: publish:/time:3] publish: 'Time' object has no attribute 'secs'
[ERROR 1641824689.066817317] [rosbridge_websocket]: [Client 2] [id: publish:/time:4] publish: 'Time' object has no attribute 'secs'

$ # Terminal 2
$ curl -sL https://github.com/tier4/rosbridge_suite/raw/add-test-scripts/test_scripts/time_old_format.py | python
publish: {'secs': 1, 'nsecs': 2}
publish: {'secs': 1, 'nsecs': 2}

After(this PR)

$ # Terminal 1
$ ros2 run rosbridge_server rosbridge_websocket
[ERROR 1641824706.049035256] [rosbridge_websocket]: [Client 0] [id: publish:/time:3] publish: Message type builtin_interfaces/Time does not have a field secs
[ERROR 1641824707.049698653] [rosbridge_websocket]: [Client 0] [id: publish:/time:4] publish: Message type builtin_interfaces/Time does not have a field secs

$ # Terminal 2
$ curl -sL https://github.com/tier4/rosbridge_suite/raw/add-test-scripts/test_scripts/time_old_format.py | python
publish: {'secs': 1, 'nsecs': 2}
publish: {'secs': 1, 'nsecs': 2}

kenji-miyake · 2022-01-10T14:28:40Z

@jtbandes Should I change the code to support the old format?

My concern is that the output from rosbridge will be the new format, so anyway the clients should upgrade the interfaces if they not only publish but subscribe Time data.
Am I right?

jtbandes

Thanks and sorry for the delay. From your examples in the description, it looks like the main change is to fix the handling of time[] arrays. Do you think it would be better to leave the other features as-is and focus on only fixing time arrays?

jtbandes · 2022-01-20T00:30:30Z

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

-    if rostype == "time" and msg == "now":
-        return ROSClock().now().to_msg()


Looks like this is removing a feature where the string "now" will be translated to the current time. I guess this feature was not documented, so it's ok to remove? Is that what you're thinking too?

Ah, I didn't care about that, sorry. Will confirm the reason why this was added!

Introduced here: 8e3eb96

jtbandes · 2022-01-20T00:32:38Z

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

-    # Copy across the fields, try ROS1 and ROS2 fieldnames
-    for field in ["secs", "nsecs", "sec", "nanosec"]:
-        try:
-            if field in msg:
-                setattr(inst, field, msg[field])
-        except TypeError:
-            continue


What's the impact of removing this? Does it mean a client publisher might need to change their code if they are still using the old values?

It's not necessarily bad to remove, but technically we should bump the package version to 2.0.0 if we remove features.

On the other hand, since some of the changes from #692 are significant to clients, I wonder if we should bump the version to 2.x anyway.

Does it mean a client publisher might need to change their code if they are still using the old values?

Yes, if this part is removed, they should change their code that uses secs/nsecs.

But I believe they should do so because they must update their subscriber-side code in any case, since rosbridge outputs Time data in the sec/nanosec format.
What do you think about it?

It's not necessarily bad to remove, but technically we should bump the package version to 2.0.0 if we remove features.

I see, I agree with you!

On the other hand, since some of the changes from #692 are significant to clients, I wonder if we should bump the version to 2.x anyway.

Yes, I think it's a good idea.

kenji-miyake · 2022-01-21T01:14:37Z

Do you think it would be better to leave the other features as-is and focus on only fixing time arrays?

@jtbandes Yes, I agree. I changed this to draft and will update the code at a later date.

Signed-off-by: Kenji Miyake <[email protected]>

kenji-miyake · 2022-02-12T09:12:04Z

@jtbandes I'm sorry to be late. Since I've fixed the code, could you review this again, please?

FYI, Related PRs/commits:

kenji-miyake · 2022-02-12T09:13:23Z

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

-            return {"sec": inst.sec, "nanosec": inst.nanosec}
-        except AttributeError:
-            return {"secs": inst.secs, "nsecs": inst.nsecs}
+        return {"sec": inst.sec, "nanosec": inst.nanosec}


Since rostype is ROS 2 type, I guess there is no need to use the secs/nsecs format.

kenji-miyake · 2022-02-12T09:14:26Z

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

 from rcl_interfaces.msg import Parameter
 from rclpy.clock import ROSClock
+from rclpy.time import Duration, Time


By import rclpy, I couldn't access to rclpy.time.

In [1]: import rclpy In [2]: rclpy.time --------------------------------------------------------------------------- AttributeError Traceback (most recent call last) Input In [2], in <module> ----> 1 rclpy.time AttributeError: module 'rclpy' has no attribute 'time'

kenji-miyake · 2022-02-12T09:16:06Z

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

+        if rostype == "builtin_interfaces/Time":
+            inst = Time().to_msg()
+        elif rostype == "builtin_interfaces/Duration":
+            inst = Duration().to_msg()


I think these ("time/duration") are probably ROS 2 porting mistakes.

kenji-miyake · 2022-02-12T09:17:05Z

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

+            setattr(inst, "sec", msg[field])
+    for field in ["nanosec", "nsecs"]:
+        if field in msg:
+            setattr(inst, "nanosec", msg[field])


Since inst is ROS 2 type, I believe we must force converting to ROS 2 fields here.

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

jtbandes

Thank you!

jtbandes · 2022-02-17T19:25:37Z

I'm not sure about the CI failure on Rolling, I'll investigate it separately. It looks like the same failure is happening on the ros2 branch right now.

…rsion.py Co-authored-by: Jacob Bandes-Storch <[email protected]>

kenji-miyake mentioned this pull request Dec 10, 2021

Fix message conversion #646

Closed

kenji-miyake marked this pull request as ready for review January 10, 2022 14:28

jtbandes reviewed Jan 20, 2022

View reviewed changes

kenji-miyake marked this pull request as draft January 21, 2022 01:12

Kenji Miyake added 4 commits February 12, 2022 17:32

Enable array tests for Time and Duration in test_message_conversion.py

b83a855

Signed-off-by: Kenji Miyake <[email protected]>

Add tests for "now"

bb30572

Signed-off-by: Kenji Miyake <[email protected]>

Add tests for secs/nsecs

21a2a17

Signed-off-by: Kenji Miyake <[email protected]>

Fix time behavior

f53d547

Signed-off-by: Kenji Miyake <[email protected]>

kenji-miyake force-pushed the fix-time-behavior branch from afdeafb to f53d547 Compare February 12, 2022 09:08

kenji-miyake commented Feb 12, 2022

View reviewed changes

kenji-miyake marked this pull request as ready for review February 12, 2022 09:17

jtbandes reviewed Feb 17, 2022

View reviewed changes

rosbridge_library/src/rosbridge_library/internal/message_conversion.py Outdated Show resolved Hide resolved

jtbandes approved these changes Feb 17, 2022

View reviewed changes

Update rosbridge_library/src/rosbridge_library/internal/message_conve…

3eeaa05

…rsion.py Co-authored-by: Jacob Bandes-Storch <[email protected]>

jtbandes merged commit cdaa0d9 into RobotWebTools:ros2 Feb 18, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix time behavior #691

Fix time behavior #691

kenji-miyake commented Dec 10, 2021 •

edited

Loading

kenji-miyake commented Jan 10, 2022

jtbandes left a comment

jtbandes Jan 20, 2022

kenji-miyake Jan 21, 2022

kenji-miyake Feb 12, 2022

jtbandes Jan 20, 2022 •

edited

Loading

kenji-miyake Jan 21, 2022

kenji-miyake commented Jan 21, 2022

kenji-miyake commented Feb 12, 2022

kenji-miyake Feb 12, 2022

kenji-miyake Feb 12, 2022

kenji-miyake Feb 12, 2022

kenji-miyake Feb 12, 2022

jtbandes left a comment

jtbandes commented Feb 17, 2022

		if rostype == "time" and msg == "now":
		return ROSClock().now().to_msg()

Fix time behavior #691

Fix time behavior #691

Conversation

kenji-miyake commented Dec 10, 2021 • edited Loading

Time Array

Time (new format of sec/nanosec)

Time (old format of secs/nsecs)

kenji-miyake commented Jan 10, 2022

jtbandes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtbandes Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenji-miyake commented Jan 21, 2022

kenji-miyake commented Feb 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jtbandes left a comment

Choose a reason for hiding this comment

jtbandes commented Feb 17, 2022

kenji-miyake commented Dec 10, 2021 •

edited

Loading

Time (new format of `sec/nanosec`)

Time (old format of `secs/nsecs`)

jtbandes Jan 20, 2022 •

edited

Loading