Allow legacy field names in rosidl_generate_interfaces [humble]

[Ryanf55]

Purpose

Propose an implementation of relaxing ROSIDL constraints on field names per this ROS Discourse proposal from a while back.

https://discourse.ros.org/t/relaxing-ros2-topic-service-field-name-restrictions/6371/8

Use Case

Happy Halloween! Do you deal with scary 👻 large ROS1 code bases that use cameCase for message field names? I do!

I have a rather large codebase that I need to migrate from ROS 1 to ROS 2. It is very much "legacy" code, so it's largely untested and unmaintained. All of the message fields names use camelCase, and sed operations don't work because variables names are reused to non-ROS messages. I don't want to cause regressions porting to ROS 2, so minimizing the code change is crucial. I am very risk averse and also in a resource bind because ROS 1 is EOL soon. Having to deal with variable naming changes is the last thing I want to bother with when porting to ROS 2. It's at least 2000 LOC of potential copy-paste typos, with many similarly named fields, such as lat and alt that are easily mixed up.

I am proposing this on humble because that's what I tested this on. If the maintainers like the implementation, I can forward port to rolling (clean up the code, finish the implementation, get CI happy, etc).

I think yellow CMake warnings about legacy field names at compile time are a good thing - open to removing those.

Demo

Follow this guide, with some changes: https://docs.ros.org/en/humble/Tutorials/Beginner-Client-Libraries/Single-Package-Define-And-Use-Interface.html#create-a-package

CMakeLists

change to this, noticing the new ALLOW_LEGACY_FIELD_NAMES option.

rosidl_generate_interfaces(${PROJECT_NAME} ALLOW_LEGACY_FIELD_NAMES 
 ${msg_files}
)

Add these so we can test the new versions
package.xml

  <depend>rosidl_cmake</depend>
  <depend>rosidl_adapter</depend>

AddressBook.msg

Change phone_type to phoneType

uint8 phoneType

If needed, set up a rolling environment:

docker run --net host -v $(pwd):/ws -w /ws -it ros:rolling bash
apt-get update
rosdep install --from-paths . --ignore-src -ry
colcon build --packages-up-to more_interfaces --event-handlers=console_cohesion+

Finally, build and test:

# ROS 2 Rolling only
colcon build --packages-up-to more_interfaces --event-handlers=console_cohesion+

# ROS 2 humble ONLY
colcon build --packages-up-to more_interfaces --allow-overriding rosidl_adapter rosidl_cmake --event-handlers=console_cohesion+

[clalancette]

We discussed this in one of the recent ROS PMC meetings. In short, we are OK with the idea of this, particularly because it is "opt-in". There were a few notes on this PR:

1. This should target rolling first, then we can consider backporting it.
2. We want to make sure that the CMake warning message explains what this option is allowing, and that it is only recommended during porting.
3. There are some checks in here that are language-specific checks. That is, some of the checks are to ensure we don't generate field names that are reserved (for instance, in Python we can't generate a field called any). We have to be sure that this relaxing of restriction doesn't allow those things.

[Ryanf55]

Thanks Chris! I'll forward port to rolling, update the CMake warnings to be more specific, and reopen with some more tests to cover that last concern.

[gavanderhoorn]

Just to add to what @clalancette wrote: I noticed the IDL spec seems to state identifiers are case insensitive.

That might also be something to warn users who'd like to opt-in to this relaxation about, as it could lead to problems later down the pipeline and/or at runtime.

[Ryanf55]

Just to add to what @clalancette wrote: I noticed the IDL spec seems to state identifiers are case insensitive.

That might also be something to warn users who'd like to opt-in to this relaxation about, as it could lead to problems later down the pipeline and/or at runtime.

That's a good point. Many companies/teams I've worked with who use FastDDS without ROS 2 frequently use camelCase for their field names. It's not legacy. It's fully supported by DDS. ROS 2 is just more strict.

Is there a better name for the flag that is more representative?

[gavanderhoorn]

That's a good point.

just to link to it: section 7.2.3 of IDL 4.2 states:

Identifiers

An identifier is an arbitrarily long sequence of ASCII alphabetic, digit and underscore ( _ ) characters. The first character must be an ASCII alphabetic character. All characters are significant.

IDL identifiers are case insensitive. However, all references to a definition must use the same case as the defining occurrence. This allows natural mappings to case-sensitive languages.

I'm not sure how / if this would affect anything here, but I thought I'd mention it.

Many companies/teams I've worked with who use FastDDS without ROS 2 frequently use camelCase for their field names. It's not legacy. It's fully supported by DDS. ROS 2 is just more strict.

Ok.

I'm not sure I understand what you're saying here.

Is there a better name for the flag that is more representative?

I'm not sure the flag name is necessarily wrong. I was just wondering whether the infrastructure/scripts should be vocal about the type of potential problems that could occur.

This was also discussed in the ROS PMC meeting, but I don't remember exactly what the outcome of that (part of the) discussion was.

[Ryanf55]

I'm not sure I understand what you're saying here.

My team uses ROS 2 for our autonomy. We integrate with groups who don't use ROS 2, but use FastDDS instead, or other DDS vendors. These other groups have extensive code based on IDL using camelCase naming. When I tell these groups that in order to communicate with our software, they need to go make intrusive changes in their code tochange naming conventions, their developers are not happy. They usually say no. Some of these groups are suppliers of software outside my company, so I have no business to dictate their API's. These groups are often aware of the limitations of ROS IDL (no enums, no unions are popular ones from what I've seen).

To resolve this conflict, my team usually ends up writing some sort of shim node in the middle to conver the API's that reduces performance and increases development time. This has nothing to with ROS 1 or legacy behavior, so that's why I'm thinking the name I selected for the flag in rosidl of ALLOW_LEGACY_FIELD_NAMES is not a very good name. I am open to alternatives.

I was just wondering whether the infrastructure/scripts should be vocal about the type of potential problems that could occur.

If I find any problems, I'd be happy to document or provide runtime warnings as appropriate. So far, I haven't ran into anything.

[Ryanf55]

I've created a new branch on rolling with a cleaned up version of this branch: #839
Let's move the discussion there, and I'll keep iterating till it's ready.