Review imported messages from ROS 1 about common_interfaces HOT 23 CLOSED

Another question, @ros2/owners should we drop seq from the Header msg?

After doing some archeology I've found some references in the now defunct ros-trac which seem to indicate that removing seq was something we have thought about doing for a long time now. Including this from Josh:

#2933 Unable to insert Sequence Number into header

Currently any value inserted into the seq field of a message header is ignored and replaced with the automatically incrementing value.
This may be as designed, the documentation doesn't make the it clear, but all other message classes explicitly avoid altering the seq value in their serialize method, provided it is non-zero.
I suspect the problem here is that the field is called 'seq', as is an argument to the serialize method, and the tools that generate the headers are just blindly inserting the field name.
If this is as designed, is there any chance it can be changed? Being able to set our own sequence numbers would be very useful for testing, as we'd like to be able to use the sequence number to link a frame to related ground truth data.

jfaust> 5 years ago
This is as-designed, and won't be changed. The seq member will only be set in the header of the message, as the header is considered special in some ways (though that's the only way that's left I think).

Header will likely be disappearing as a special member at some point (hopefully for 2.0). If you want a sequence number of your own you'll have to provide your own in your message.

jfaust> 5 years ago
Hmm, I guess that really makes the answer "yes, it is likely to be changed", but the seq member may also be removed from the Header message

And this sparse ticket from Brian:

#1965 Investiage removal seq from Header
reporter: gerkey

Determine needs and implications, and file new tickets.

gerkey> 6 years ago
Jack's using it, so we won't turn off auto-filling of sequence numbers.
status: wontfix

@tfoote also referenced Brian's ticket from ROS answers: http://answers.ros.org/question/55126/why-does-ros-overwrite-my-sequence-number/?answer=55132#post-id-55132

Jack also seems to mention it here, with backup from @tfoote again: http://answers.ros.org/question/38856/ros-header-increasing-seq-in-service-calls/?answer=38933#post-id-38933

from common_interfaces.

• The current parser uses relaxed rules in order to handle ROS 1 messages (https://github.com/ros2/rosidl/blob/7fbc60bc0c041796a603a4e301108d0e5887e75e/rosidl_parser/rosidl_parser/__init__.py#L51-L56). When you switch the regular expressions to the strict versions they should still pass.

• Update the rmw implementation to handle the new package name of builtin messages.

• Relative message references are fine, see https://github.com/ros2/design/blob/gh-pages/articles/110_interface_definition.md:

  The *base type* can be one of the following:
  * a primitive type from the list above: e.g. `int32`
  * a string with an upper boundary: `string<=N` to limit the length of the string to `N` characters
  * a complex type referencing another message:
    * an *absolute* reference of a message: e.g. `some_package/SomeMessage`
    * a *relative* reference of a message within the same package: e.g. `OtherMessage`
  

from common_interfaces.

Please make sure to enable the linters for the message packages.

from common_interfaces.

The current parser uses relaxed rules in order to handle ROS 1 messages ...

Ok, I'll make a PR with the fixes that those stricter rules require, so everyone can see it.

Update the rmw implementation to handle the new package name of builtin messages.

I'll do that when I propose a pr to remove builtin_msgs from examples. Just trying to get these setup before making that change.

Please make sure to enable the linters for the message packages.

Will do. Is it just the ament_lint_auto and ament_lint_common dependencies that I need?

from common_interfaces.

RTPS/DDS provides a 64 bit sequence number, so if we do want to expose
this, we could reach into the dds implementation. Although hmm I guess that
wouldn't cover intra-process messaging.
On Jun 16, 2015 6:47 PM, "William Woodall" [email protected] wrote:

Another question, @ros2/owners https://github.com/orgs/ros2/teams/owners
should we drop seq from the Header msg?

After doing some archeology I've found some references in the now defunct
ros-trac which seem to indicate that removing seq was something we have
thought about doing for a long time now. Including this from Josh:

#2933 Unable to insert Sequence Number into header

Currently any value inserted into the seq field of a message header is
ignored and replaced with the automatically incrementing value.
This may be as designed, the documentation doesn't make the it clear, but
all other message classes explicitly avoid altering the seq value in their
serialize method, provided it is non-zero.
I suspect the problem here is that the field is called 'seq', as is an
argument to the serialize method, and the tools that generate the headers
are just blindly inserting the field name.
If this is as designed, is there any chance it can be changed? Being able
to set our own sequence numbers would be very useful for testing, as we'd
like to be able to use the sequence number to link a frame to related
ground truth data.

jfaust> 5 years ago
This is as-designed, and won't be changed. The seq member will only be set
in the header of the message, as the header is considered special in some
ways (though that's the only way that's left I think).

Header will likely be disappearing as a special member at some point
(hopefully for 2.0). If you want a sequence number of your own you'll have
to provide your own in your message.

jfaust> 5 years ago
Hmm, I guess that really makes the answer "yes, it is likely to be
changed", but the seq member may also be removed from the Header message

And this sparse ticket from Brian:

#1965 Investiage removal seq from Header
reporter: gerkey

Determine needs and implications, and file new tickets.

gerkey> 6 years ago
Jack's using it, so we won't turn off auto-filling of sequence numbers.
status: wontfix

@tfoote https://github.com/tfoote also referenced Brian's ticket from
ROS answers:
http://answers.ros.org/question/55126/why-does-ros-overwrite-my-sequence-number/?answer=55132#post-id-55132

Jack also seems to mention it here, with backup from @tfoote
https://github.com/tfoote again:
http://answers.ros.org/question/38856/ros-header-increasing-seq-in-service-calls/?answer=38933#post-id-38933

—
Reply to this email directly or view it on GitHub
#1 (comment)
.

from common_interfaces.

Well, I think exposing the sequence number might be ok, but it should be an immutable part of the middleware interaction, rather than a user settable element in only some of the messages. I think the fact that it was inconsistently set by publish was confusing to users, which is evident in the questions about Python auto incrementing for pub/sub and the seq not getting set for services.

I'd personally vote to get rid of it, and optionally expose the DDS sequence number through our middleware API.

It depends on how we implement intra-process comms, but either way having a sequence number in intra-process comms is probably trivial to implement.

from common_interfaces.

If we alter the message definitions imported from ROS 1 what should the strategy then be for the bridge?

• Consider the messages not "equal" anymore and not allow exchanging them?
• Ignore fields which are only on one side / set the value to a default on the other side?

I remember a similar discussion about the frame_id in the header. While it is a plain string field it should be a semantic type like FrameId in order to allow automatic transformations.

But we should be careful about making changes since every modification will make the bridge more complicated.

from common_interfaces.

+1 for removing seq.

I think typing frame_id would be very valuable especially as we consider multi-robot system coordination. And on top of making the bridge more complicated, it will make porting code forward just that much harder.

from common_interfaces.

Consider the messages not "equal" anymore and not allow exchanging them?

I can't imagine a scenario where not exchanging std_msgs/Header with std_interface/Header would be acceptable. Even if messages cannot be automatically exchanged (and even if they can be), it should always be possible for developers to optionally insert some kind of transfer function to help convert between messages. Pending that dynamic capability, we should do everything we can for the core messages in our first static bridge. If we have a message that needs to change so drastically, maybe we reconsider the idea of not allowing it through the bridge, but that's a pretty extreme reaction.

Ignore fields which are only on one side / set the value to a default on the other side?

I think it depends on the case by case basis. For sequence number, we can do a few different things. If the sequence number is coming from ROS 1, then we'll have to ignore it and use what ever sequence number is provided by DDS (assuming we can't set the sequence number in ROS 2), but for messages originating from ROS 2 we can stuff the ROS 1 seq with the DDS sequence number modulo 2^32 (since the ROS sequence number is uint32 and the DDS one is uint64). I think we should make any necessary custom effort like this to bridge the gap, documenting thoroughly with situations like the sequence number in the header.

I'm not up to speed on why we want a FrameId msg, but that seems like a straight forward transfer function.

My feeling is that keeping the migration simple is important, but we shouldn't shy away from changes like this because this is really the only opportunity we have to make these adjustments. I agree that we shouldn't make arbitrary changes though, like changing field names unless there is a good rational for doing so. We can also summarize these proposed changes to the base messages and share them with the ros-ng-sig and get feedback.

from common_interfaces.

In 6b792c4 I've converted time to Time in Header.msg, as well as cleaned up the comments, removing a reference to numeric frame id's.

I'll make a pr for removing seq and we can make a decision on that there.

from common_interfaces.

I'm starting to migrate other interfaces from ROS 1 packages in common_msgs, but I'm going to skip a few for now:

• actionlib_msgs, defer until we look at actions in ROS 2
• diagnostic_msgs, defer until we look into diagnostics in ROS 2
• nav_msgs, just the GetMap action in that package, I'll port everything else.

Does that sound ok to everyone?

from common_interfaces.

Sounds good to me.

from common_interfaces.

Agreed

from common_interfaces.

+1

from common_interfaces.

+1

from common_interfaces.

Ok, I went ahead an ported everything, except the GetMap.action in nav_msgs, incase we want to use them with the ROS 1 equivalents in the demo. However, this adds a lot of unused messages which take a long time to compile. So after making sure they compile correctly, I've added AMENT_IGNORE files in these packages:

• actionlib_interfaces
• diagnostics_interfaces
• shape_interfaces
• trajectory_interfaces
• visualization_interfaces

With the idea that we can enable them again when and if we need them.

I'll test again for others, but the sensor_interfaces for sure has trouble with ros2/rosidl#25.

I've also refactored the examples repository and made changes necessary to work with the renaming of builtin_msgs, simple_msgs, and userland_msgs.

I also put a tag on the examples repository called pre_refactor in case we need to go back and get stuff I pruned from files or files I removed all together. I tried to limit the examples to just pub/sub, services, and parameters for now, and I simplified a few of them so they're simpler and easier to parse for non c++ veterans.

I'll be opening more pr's soon and testing them on the farm. Hopefully all of this will be ready for review by tomorrow and it will be minimally disruptive to other branches people are starting.

from common_interfaces.

Please feel free make a last pass over the current state of this repository and comment here, but I'm going to close this for now.

from common_interfaces.

While disabling most of the messages is a viable short term fix (to reduce the compile time) this seems to be an important thing to address.

from common_interfaces.

While disabling most of the messages is a viable short term fix (to reduce the compile time) this seems to be an important thing to address.

What should be addressed? The compile time? To be clear I'm not disabling them due to ros2/rosidl#25.

from common_interfaces.

Yes, if the compile time is "so bad" we should consider looking into it to come up with ways to speed it up.

from common_interfaces.

Short of having less messages or building with less rmw implementations by default, I'm not sure what we can do. It's not that the compile time is unbearable, but I just thought that as long as no one is using those messages that we might as well save some compile time.

from common_interfaces.

On frame_id, mentioned above (#1 (comment)). I see two conflicting aspects:

1. If frame_id stays, it makes forward porting easier.
2. There are many cases in which frame_id simply makes no sense.

I'm not sure I understood the multi-robot comment cited above (#1 (comment)). Is the intention to use the frame_id field to designate the robot the data is coming from? (not really a frame, but just some identifier).

from common_interfaces.

@adolfo-rt We can't remove the frame_id. The Header is primarily a pair of frame_id and timestamp. If you don't need the frame_id you can just use a timestamp in the datatype. There are some tools which look for the Header as a subdatatype. Such as the message syncronizers. They could be generalized by the introduction of a standard name for a timestamp field where it could be made to work with any timestamped or Header containing message.

The question about frame_ids is whether to leave it as a string datatype or to make it a strongly typed datatype FrameId or something similar. When data transfers between robot systems with different tf trees all data will need to be rewritten to be consistent with the target systems frame_id tree. left_gripper from two robots must be disambiguated when you have two robots with left hands. Both the transform tree and the message with the frame_id embedded must be converted. If the frame_id is strongly typed introspecting for the conversion would be a lot simpler. While at the moment we cannot apply a frame_id remapping to any string argument, they need to be manually whitelisted.

from common_interfaces.