Fix array behavior #692

kenji-miyake · 2021-12-10T06:07:10Z

Public API Changes

uint8[] will be sent in binary format instead of list format.

Description

Split from #646.

Fixed the behavior around arrays.

How to test

Fixed Array

Before(upstream/ros2)

$ # Terminal 1
$ ros2 run rosbridge_server rosbridge_websocket
[ERROR 1641822931.215073518] [rosbridge_websocket]: [Client 0] [id: publish:/fixed_array:3] publish: invalid literal for int() with base 10: b'\x00\x01\x02\x03\x04\x05\x06\x07\x08\t\n\x0b\x0c\r\x0e\x0f'
[ERROR 1641822932.220379579] [rosbridge_websocket]: [Client 0] [id: publish:/fixed_array:4] publish: invalid literal for int() with base 10: b'\x00\x01\x02\x03\x04\x05\x06\x07\x08\t\n\x0b\x0c\r\x0e\x0f'

$ # Terminal 2
$ curl -sL https://github.com/tier4/rosbridge_suite/raw/add-test-scripts/test_scripts/int8_fixed_array.py | python
publish: {'uuid': [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]}
publish: {'uuid': [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]}

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/int8_fixed_array.py | python
publish: {'uuid': [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]}
subscribe: {'uuid': 'AAECAwQFBgcICQoLDA0ODw=='}
publish: {'uuid': [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]}
subscribe: {'uuid': 'AAECAwQFBgcICQoLDA0ODw=='}

Multi Array

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/int8_array.py | python
publish: {'data': [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]}
subscribe: {'layout': {'dim': [], 'data_offset': 0}, 'data': [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]}
publish: {'data': [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]}
subscribe: {'layout': {'dim': [], 'data_offset': 0}, 'data': [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]}

This means uint8[] such as in sensor_msgs/msg/Image won't be binary.

https://github.com/ros2/common_interfaces/blob/f41462f5dd36f5c3ae28866f18f356423ffb84e4/sensor_msgs/msg/Image.msg#L26
https://github.com/ros2/example_interfaces/blob/2977bbe4e5e30c74d3594d31a8212193f5c27761/msg/UInt8MultiArray.msg#L9

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/int8_array.py | python
publish: {'data': [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]}
subscribe: {'layout': {'dim': [], 'data_offset': 0}, 'data': 'AAECAwQFBgcICQoLDA0ODw=='}
{'layout': {'dim': [], 'data_offset': 0}, 'data': 'AAECAwQFBgcICQoLDA0ODw=='}
publish: {'data': [0, 1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15]}
subscribe: {'layout': {'dim': [], 'data_offset': 0}, 'data': 'AAECAwQFBgcICQoLDA0ODw=='}
{'layout': {'dim': [], 'data_offset': 0}, 'data': 'AAECAwQFBgcICQoLDA0ODw=='}

uint32[3] (#699)

Before(upstream/ros2)

$ # Terminal 1
$ ros2 run rosbridge_server rosbridge_websocket
[ERROR 1641822637.451501204] [rosbridge_websocket]: [Client 0] [id: publish:/uint32_fixed_array:3] publish: The 'vertex_indices' field must be a set or sequence with length 3 and each value of type 'int' and eac
h unsigned integer in [0, 4294967295]
[ERROR 1641822638.448902374] [rosbridge_websocket]: [Client 0] [id: publish:/uint32_fixed_array:4] publish: The 'vertex_indices' field must be a set or sequence with length 3 and each value of type 'int' and eac
h unsigned integer in [0, 4294967295]

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

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/mesh_triangle.py | python
publish: {'vertex_indices': [1, 2, 3]}
subscribe: {'vertex_indices': [1, 2, 3]}
publish: {'vertex_indices': [1, 2, 3]}
subscribe: {'vertex_indices': [1, 2, 3]}

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

Seeing the result, it looks a bit strange, so I'll re-check it.
We have to discuss the test specifications.

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

kenji-miyake · 2022-01-10T13:37:21Z

Although it's not directly related to this PR, I probably found a mistake. float types are in int types.
9a92221#diff-212be7c9fa67fbb1f8af9069876e393724e387108a685d2cc0a2f2375b561ddbR16

type_map = {
   "bool":    ["bool"],
   "int":     ["int8", "byte", "uint8", "char",
               "int16", "uint16", "int32", "uint32",
               "int64", "uint64", "float32", "float64"],
   "float":   ["float32", "float64"],
   "str":     ["string"],
   "unicode": ["string"],
   "long":    ["uint64"]
}

It remains until now.

rosbridge_suite/rosbridge_library/src/rosbridge_library/internal/message_conversion.py

Lines 64 to 65 in cf1beef

    
           "float32", 
        
           "float64",

@jtbandes Can I remove this here or will you check?

-> Removed in https://github.com/RobotWebTools/rosbridge_suite/pull/692/files#r781215355.

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

kenji-miyake · 2022-01-10T14:00:13Z

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

@@ -61,8 +61,6 @@
        "uint32",
        "int64",
        "uint64",
-        "float32",
-        "float64",


#692 (comment)

Since I changed to use type_map["int"], I removed them.
https://github.com/RobotWebTools/rosbridge_suite/pull/692/files#diff-212be7c9fa67fbb1f8af9069876e393724e387108a685d2cc0a2f2375b561ddbR253-R254

kenji-miyake · 2022-01-10T14:03:12Z

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

@@ -91,7 +89,7 @@
    "string",
 ]
 ros_header_types = ["Header", "std_msgs/Header", "roslib/Header"]
-ros_binary_types = ["uint8[]", "char[]"]
+ros_binary_types = ["uint8[]", "char[]", "sequence<uint8>", "sequence<char>"]


Since uint8[] will be sequence<uint8>, I marked it as binary.
https://design.ros2.org/articles/idl_interface_definition.html

I think I can drop this, but then sensor_msgs/msg/Image won't be binary as well.
https://github.com/ros2/common_interfaces/blob/f41462f5dd36f5c3ae28866f18f356423ffb84e4/sensor_msgs/msg/Image.msg#L26

kenji-miyake · 2022-01-10T14:06:54Z

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

+            return inst.tolist()
+
+        if rostype not in type_map.get("float"):
+            return list(inst)


Originally, uint32[3] was converted by list(inst), which causes an error here.

It can be checked by the following code.

In [1]: import json ...: import numpy as np ...: ...: json.dumps(list(np.array([1,2,3], dtype=np.uint32))) ...: --------------------------------------------------------------------------- TypeError Traceback (most recent call last) <ipython-input-1-097c4709dbd2> in <module> 2 import numpy as np 3 ----> 4 json.dumps(list(np.array([1,2,3], dtype=np.uint32))) /usr/lib/python3.8/json/__init__.py in dumps(obj, skipkeys, ensure_ascii, check_circular, allow_nan, cls, indent, separators, default, sort_keys, **kw) 229 cls is None and indent is None and separators is None and 230 default is None and not sort_keys and not kw): --> 231 return _default_encoder.encode(obj) 232 if cls is None: 233 cls = JSONEncoder /usr/lib/python3.8/json/encoder.py in encode(self, o) 197 # exceptions aren't as detailed. The list call should be roughly 198 # equivalent to the PySequence_Fast that ''.join() would do. --> 199 chunks = self.iterencode(o, _one_shot=True) 200 if not isinstance(chunks, (list, tuple)): 201 chunks = list(chunks) /usr/lib/python3.8/json/encoder.py in iterencode(self, o, _one_shot) 255 self.key_separator, self.item_separator, self.sort_keys, 256 self.skipkeys, _one_shot) --> 257 return _iterencode(o, 0) 258 259 def _make_iterencode(markers, _default, _encoder, _indent, _floatstr, /usr/lib/python3.8/json/encoder.py in default(self, o) 177 178 """ --> 179 raise TypeError(f'Object of type {o.__class__.__name__} ' 180 f'is not JSON serializable') 181 TypeError: Object of type uint32 is not JSON serializable

kenji-miyake · 2022-01-10T14:07:59Z

rosbridge_library/src/rosbridge_library/internal/message_conversion.py

-    except Exception:
+    if isinstance(msg, str):
+        return list(standard_b64decode(msg))
+    if isinstance(msg, list):


If we accept list input like this, we need this if-condition.

kenji-miyake · 2022-01-10T14:10:17Z

rosbridge_library/src/rosbridge_library/protocol.py

-                # Only bother sending the log message if there's an id
-                self.log("error", "Unable to serialize %s message to client" % msg["op"], cid)
+        except Exception as e:
+            self.log("error", f"Unable to serialize message '{msg}': {e}")


Without this, I think we can't notice the errors. This time it took a lot of time to find the cause. 🥺

interesting - why would cid be None?

Mm, sorry I'm not sure about that. 😢

kenji-miyake · 2022-01-10T14:11:39Z

@jtbandes I'm done! Could you review this, please? 🙏

jtbandes

Sorry for the delay – Thanks for fixing this! It would be nice to add some integration tests too, to prove it's working as expected end to end.

jtbandes · 2022-01-20T00:21:47Z

rosbridge_library/src/rosbridge_library/protocol.py

-                # Only bother sending the log message if there's an id
-                self.log("error", "Unable to serialize %s message to client" % msg["op"], cid)
+        except Exception as e:
+            self.log("error", f"Unable to serialize message '{msg}': {e}")


interesting - why would cid be None?

kenji-miyake mentioned this pull request Dec 10, 2021

Fix message conversion #646

Closed

kenji-miyake marked this pull request as draft December 10, 2021 06:21

jtbandes mentioned this pull request Jan 3, 2022

Some message types fail to be forwarded by rosbridge_server to subscribed clients #699

Closed

Kenji Miyake added 5 commits January 10, 2022 19:41

Enable test_int8array in test_message_conversion.py

bd4d696

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

Treat dynamic uint8 array as binary

ea7170e

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

Allow receiving list data for binary variables

b1dfdfa

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

Always show an error message of serialization failure

c44adc1

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

Fix special mappings for numeric types

4993755

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

Kenji Miyake added 2 commits January 10, 2022 22:37

Remove incorrect types

d7956e9

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

Fix JSON dump of integer types

06fe1ea

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

kenji-miyake force-pushed the fix-array-behavior branch from 7964eb0 to 06fe1ea Compare January 10, 2022 13:47

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

kenji-miyake commented Jan 10, 2022

View reviewed changes

jtbandes approved these changes Jan 20, 2022

View reviewed changes

jtbandes merged commit 5398588 into RobotWebTools:ros2 Jan 20, 2022

jtbandes mentioned this pull request Jan 20, 2022

Fix time behavior #691

Merged

kenji-miyake deleted the fix-array-behavior branch January 20, 2022 04:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix array behavior #692

Fix array behavior #692

kenji-miyake commented Dec 10, 2021 •

edited

Loading

kenji-miyake commented Dec 10, 2021

kenji-miyake commented Jan 10, 2022 •

edited

Loading

kenji-miyake Jan 10, 2022

kenji-miyake Jan 10, 2022

kenji-miyake Jan 10, 2022

kenji-miyake Jan 10, 2022

kenji-miyake Jan 10, 2022

jtbandes Jan 20, 2022

kenji-miyake Jan 20, 2022

kenji-miyake commented Jan 10, 2022

jtbandes left a comment •

edited

Loading

jtbandes Jan 20, 2022

Fix array behavior #692

Fix array behavior #692

Conversation

kenji-miyake commented Dec 10, 2021 • edited Loading

Fixed Array

Multi Array

uint32[3] (#699)

kenji-miyake commented Dec 10, 2021

kenji-miyake commented Jan 10, 2022 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenji-miyake commented Jan 10, 2022

jtbandes left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kenji-miyake commented Dec 10, 2021 •

edited

Loading

kenji-miyake commented Jan 10, 2022 •

edited

Loading

jtbandes left a comment •

edited

Loading