Adding a message type for Log messages that will be sent out over the…

[nburek]

Adding a message type for Log messages that will be sent out over the rosout topic.

Issue for this request: #50

[tfoote]

For reference this is a migration of the rosgraph_msgs/Log in ROS1: http://docs.ros.org/melodic/api/rosgraph_msgs/html/msg/Log.html

##
## Severity level constants
##
byte DEBUG=1 #debug level
byte INFO=2  #general level
byte WARN=4  #warning level
byte ERROR=8 #error level
byte FATAL=16 #fatal/critical level
##
## Fields
##
Header header
byte level
string name # name of the node
string msg # message 
string file # file the message came from
string function # function the message came from
uint32 line # line the message came from
string[] topics # topic names that the node publishes

The primary changes are changing the constant values and dropping the topics.
I agree that dropping the topics makes sense.

The enum values have changed from a bitmask like approach to a sequential approach with padding. This is matching the values in rcutils: https://github.com/ros2/rcutils/blob/master/include/rcutils/logging.h#L156-L165
They were set in ros2/rcutils#57 to follow python's conventions: ros2/rcutils#57 (review)

Since we're using an enum and I don't believe that we're planning to support custom logging levels I'd suggest that we restore the old enum values for consistency adding 0 for UNSET.

[dirk-thomas]

name of the node

This comment is only partially true. The name might be a node name but it can also be any other user defined string.

Since we're using an enum and I don't believe that we're planning to support custom logging levels I'd suggest that we restore the old enum values.

The logging system already does support arbitrary numbers in between. So the constants should match the implementation exactly. The type should be changed from byte to uint8 though since the data is actually numeric with its implicit order and not an opaque octet.

for consistency adding 0 for UNSET.

Since there is no semantic associated to 0 atm I think there shouldn't be a constant defined in the message for it.

[nburek]

name of the node

This comment is only partially true. The name might be a node name but it can also be any other user defined string.

I'll update the comment to mention that the field is meant to represent the name of the logger used to log this message. The logger name won't exactly match the node name anyways

Since we're using an enum and I don't believe that we're planning to support custom logging levels I'd suggest that we restore the old enum values.

The logging system already does support arbitrary numbers in between. So the constants should match the implementation exactly. The type should be changed from byte to uint8 though since the data is actually numeric with its implicit order and not an opaque octet.

I'll change the type from byte to uint8.

[mlautman]

Don't forget to add msg/Log.msg to the rosidl_generate_interfaces call

[mlautman]

Header -> std_msgs/Header

[nburek]

Thanks for the feedback @mlautman. I've added the Log.msg to the rosidl_generate_interface call in CMake. I've also removed the header from the Log message for now since it would add a dependency on std_msgs from common_interfaces. In the conversation on issue #50, there was a desire to not have any new dependencies added in front of rcl.

[mlautman]

Seems strange to not be able to depend on a core library like std_msgs. The header field is really useful

[dirk-thomas]

Seems strange to not be able to depend on a core library like std_msgs.

That statement is taken out of context. It is undesired to add a dependency to the common_interfaces package into the rcl packages. Since the log message should continue to use a header field the std_msgs package (or maybe just that specific message) could be moved into the rcl_interfaces repo...

[nburek]

It may make sense for the Header message definition to be moved into rcl_interfaces. If that's the case then the Log message could add it back in as a field. However, should that be done for this pull request or should we move forward with the Log message as defined in this request and then create a new issue for the follow up work of moving either std_msgs or Header so they are part of the core dependencies?

[dirk-thomas]

The Header also contains a frame id which isn't needed / used here. Therefore I would suggest to add a timestamp field instead.

[nburek]

The Header also contains a frame id which isn't needed / used here. Therefore I would suggest to add a timestamp field instead.

That may beg the question of if frame id belongs inside of Header or not. Judging by the comment in the .msg file, the frame id pertains to transformation data. Does it make sense to remove the frame id to make Header apply more generally or should header potentially be moved/renamed so that it's more apparent the Header is meant to be used with Transform messages.

[wjwwood]

That may beg the question of if frame id belongs inside of Header or not. Judging by the comment in the .msg file, the frame id pertains to transformation data. Does it make sense to remove the frame id to make Header apply more generally or should header potentially be moved/renamed so that it's more apparent the Header is meant to be used with Transform messages.

I would say no... The header is used mostly in the sensor messages where data both has a creation time and a physical origin location, in which case the frame id makes sense.

I think it's just convenient for people to grab the header when they needed a timestamp and/or a sequence number (sequence number is no longer in ROS 2's header message). Personally, I think this is a case of "Header abuse".

[tfoote]

The std_msgs/Header has significant meaning for physically connected data which is almost all data at the robotic developer level or robot application level. What we're doing here is much more internal to the middleware. We could define a new datatype but we've already deprecated the sequence number in the header and if you remove the frame_id it's just a timestamp. So just using a raw timestamp is actually really all that there is to a std_msgs/Header if we don't want the frame_id.

There's probably a slightly better name for std_msgs/Header but since it's used throughout the ROS ecosystem in it's current name, changing it is much more of a hassle than it's worth.

[nburek]

Alright, good to know. I'll go ahead and add timestamp as a Time type field

[mlautman]

Add find_package(builtin_interfaces REQUIRED) after find_package(ament_cmake REQUIRED) and DEPENDENCIES builtin_interfaces at the end of rosidl_generate_interfaces

[nburek]

Add find_package(builtin_interfaces REQUIRED) after find_package(ament_cmake REQUIRED) and DEPENDENCIES builtin_interfaces at the end of rosidl_generate_interfaces

Thanks Mike. I'm actually in the middle of doing that now and verifying it builds locally. Will update pull request in a minute.

[tfoote]

• Linux
• Linux-aarch64
• macOS
• Windows

[tfoote]

This lgtm and it's passing CI.