ros2 / rcl_interfaces

A repository for messages and services used by the ROS client libraries
Apache License 2.0
40 stars 43 forks source link

rcl_interfaces Log message level field does not match type of constants #158

Closed gstorer closed 1 year ago

gstorer commented 1 year ago

The rcl_interfaces Log message level field does not match the type of constants. The constants are of type byte and the field is of type uint8.

This means the following Python code fails with an exception:

from rcl_interfaces.msg import Log
msg = Log()
msg.level = Log.INFO

To get this to work, something like this needs to be done which is a bit ridiculous:

from rcl_interfaces.msg import Log
msg = Log()
msg.level = int.from_bytes(Log.INFO, byteorder='little')

Either the level field needs to be changed to a byte type or the constants should be changed to uint8. I don't think rclpy should be expected to do a silent conversion between types to make this work correctly.

emersonknapp commented 1 year ago

Can you specify which distro/OS you saw this behavior on, including a copy-paste of the exact error output? I've just tried this on the latest Rolling and it worked fine:

from rcl_interfaces.msg import Log
msg = Log()
msg.level = Log.INFO
gstorer commented 1 year ago

Its running on Ubuntu 20.04 with Humble built from source. I will investigate a bit more on Monday and get the exact stack trace. It didn't occur to me that it would be fixed in a future rclpy version. It still seems wrong that the constants would have a different type to the field.

clalancette commented 1 year ago

It didn't occur to me that it would be fixed in a future rclpy version.

So this is an interesting case. It isn't actually "fixed", though we no longer see it due to some changes in more recent code.

Sometime back in March or April, we merged a change where we disabled-by-default the run-time type checking in ROS Python message code. We ended up doing that because that type checking was pretty expensive and actually showed up in flame graphs.

But you can see that the original problem pointed out here is still a problem in Rolling and Iron by doing the following:

$ export ROS_PYTHON_CHECK_FIELDS=1
$ python3
Python 3.10.12 (main, Jun 11 2023, 05:26:28) [GCC 11.4.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from rcl_interfaces.msg import Log
>>> msg = Log()
>>> msg.level = Log.INFO
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/ubuntu/new_ros2_ws/install/rcl_interfaces/lib/python3.10/site-packages/rcl_interfaces/msg/_log.py", line 236, in level
    assert \
AssertionError: The 'level' field must be of type 'int'

In this case, I agree that we should make the Level constants the same type as the level field, which will fix this issue for good. This isn't something we'd be able to backport, but at least it would be fixed for future versions.

gstorer commented 1 year ago

Great you got the bottom of it. I apologise for not following up. I have had the flu and have not been at work the past few days.