ROS2 message format should support enums

[dirk-thomas]

From @calvertdw on January 8, 2018 16:53

You guys know what this means.

Constants aren't good enough. They don't constrain you to the range of values and you are able to set the field to any number you want.

// Current way

int16 FOO=0
int16 BAR=1
int16 value

// Proposed way

enum foo_bar { FOO, BAR }

Copied from original issue: ros2/ros2#447

[dirk-thomas]

We are aware of this. Currently the .msg doesn't provide the necessary expressability to implement this. Therefore on the roadmap we have an items to consider other formats (see https://github.com/ros2/ros2/wiki/Roadmap#design--concept):

Revise IDL file format, consider desired features (grouping, comments, etc.), suitability of existing formats like IDL 4.2

[dirk-thomas]

With the IDL changes merged this becomes more feasible. Potential follow up of #346.

[allenh1]

@dirk-thomas / @calvertdw is anyone working on this, or could I start attempting something on this one?

On that note, I had one concern I'd like to address before I dive in on this (assuming I can). Suppose that one host has an outdated copy of the .msg file where the enum has contents enum foo_bar { FOO, BAZ }. Further, suppose that the copy of the .msg file on the second host is up to date and has this field as enum foo_bar { FOO, BAR, BAZ }. The standard dictates that FOO = 0, BAZ = 1 on host 1, but also that FOO = 0 and BAZ = 2 on the second host.

While this is definitely not good practice to do, such a mistake occurs quite easily and is not so simple to track down. So I'm not quite sure how to store such a type internally to detect such an incompatibility. @dirk-thomas maybe you have some idea?

[dirk-thomas]

is anyone working on this, or could I start attempting something on this one?

Nobody is currently working on this or planning to in the near future.

First, any change should happen for .idl files. The format of .msg files is not intended to be extended with additional features anymore.

Adding an enum value in the middle is only compatible if you explicitly specify the enum values (so that they don't change). Also I think the IDL spec already defines enums - so that part would likely be covered by the spec already.

[davidhodo]

@allenh1 Have you started work on this? This is a feature we need as well and would be willing to contribute towards. I'm not able to open the design concept link Dirk posted above, but is there any initial thoughts on how this should be approached?

[mjcarroll]

@davidhodo This the corrected link: https://github.com/ros2/ros2_documentation/blob/master/source/Roadmap.rst#design--concept

[oysstu]

Is the addition of bit masks also being considered along with this (IDL v4.2 section 7.4.13.4.3.3)?

[dirk-thomas]

Is the addition of bit masks also being considered along with this

No, bit masks are orthogonal to enums. Also nobody is currently working on enum support either.

[RFRIEDM-Trimble]

Is this on the roadmap?

If not, can anyone give some recommendations on how to deal with checking enums are valid before publishing or after receiving, handling conversions from the ROS integer back to enums, and how to avoid maintaining a list of constants in the message file in addition to an enum in other source code?

[clalancette]

Is this on the roadmap?

The roadmap is published here. In short, nobody is currently working on adding enum support. That said, if you are interested in working on it, we'd be happy to review pull requests.

[r7vme]

Hello, I've started to work on this task. After few days looking thru rosidl_parser code, I'm not quite sure how enum support should look like in .idl file. Especially I see that in ROSIDL one should place constants under special module <name>_Constants. I especially looking into this commit where suffix _Constants was added

Do we want the similar suffix for enums? Here is the example how it would look like.

module foo {
  module msg {
    module VehicleState_Constants {
      const uint16 CONSTANT1 = 1;
    };

    module VehicleState_Enums {
      enum GearState {
        DRIVE,
        NEUTRAL,
        REVERSE
      };
    };

    struct VehicleState {
      GearState gear_state;
    };
  };
};

For example in DDS IDL there are no such restrictions and one can have smth like (tested with Cyclone idlc generator)

module foo {
  module msg {
    enum GearState {
      DRIVE,
      NEUTRAL,
      REVERSE
    };

    struct VehicleState {
      GearState gear_state;
    };
  };
};

[r7vme]

@dirk-thomas @clalancette do you know if my assumption above makes sense?

cc @allenh1

[clalancette]

@dirk-thomas @clalancette do you know if my assumption above makes sense?

I'm not entirely sure why we added the additional <module>_Constants for constants. It may be just to ensure that there is no overlap between constants from different files when they are composed together. From that perspective, then, it might make sense to put the enums in a separate module as well.

[r7vme]

Hello, just want to give a small update. We at Apex.AI already have internal implementation of ROSIDL enum support and going to upstream it soon (2-3 weeks).

cc @lyletbjohnson

[jasonbeach]

That's fantastic!

[apockill]

Hi @r7vme I'm just curious if there's any progress? I'd love to know if this feature might make it into Humble.

[r7vme]

@apockill Unfortunately, I believe, it's won't make into Humble.

[r7vme]

@clalancette Hello, I created initial set of PRs (the main one). Have few questions.

1. Do you have suggestion who can be reviewer for those PRs? First one in the queue is Add ROSIDL enum support test_interface_files#20
2. Who should be making rmw typesupport related changes? I already added support in FastDDS and planning to do for CycloneDDS. I was planning to create issues in other rmw's.
3. Any other generic suggestions as this is quite a big change?

cc @allenh1 @kylemarcey

[ros-discourse]

This issue has been mentioned on ROS Discourse. There might be relevant details there:

https://discourse.ros.org/t/need-reviewers-for-rosidl-related-change/26285/1

[sloretz]

I'm not quite sure how enum support should look like in .idl file. Especially I see that in ROSIDL one should place constants under special module _Constants

The constants were put into a special module because the OMG IDL 4.2 spec (see 7.4.1.4.4.4.1) doesn't allow them to be put in structs. The can exist globally, but we wanted constants to be specific to the section that defined them in the case of services. The solution implemented was to make a module suffixed with _Constants so the code generators could match those constants with a corresponding struct.

As an example, the LoadMap service has constants defined on the result. See the _Constants module associated with the LoadMap_Response struct in IDL:

module nav_msgs {
  module srv {
    struct LoadMap_Request {
      string map_url;
    };
    module LoadMap_Response_Constants {
      const uint8 RESULT_SUCCESS = 0;
      const uint8 RESULT_MAP_DOES_NOT_EXIST = 1;
      const uint8 RESULT_INVALID_MAP_DATA = 2;
      const uint8 RESULT_INVALID_MAP_METADATA = 3;
      const uint8 RESULT_UNDEFINED_FAILURE = 255;
    };
    struct LoadMap_Response {
      nav_msgs::msg::OccupancyGrid map;
      uint8 result;
    };
  };
};

See the C++ Generator combines the constants onto the generated struct:

// message struct
template<class ContainerAllocator>
struct LoadMap_Response_
{
  // [...]

  // field types and members
  using _map_type =
    nav_msgs::msg::OccupancyGrid_<ContainerAllocator>;
  _map_type map;
  using _result_type =
    uint8_t;
  _result_type result;

  // [...]

  // constant declarations
  static constexpr uint8_t RESULT_SUCCESS =
    0u;
  static constexpr uint8_t RESULT_MAP_DOES_NOT_EXIST =
    1u;
  static constexpr uint8_t RESULT_INVALID_MAP_DATA =
    2u;
  static constexpr uint8_t RESULT_INVALID_MAP_METADATA =
    3u;
  static constexpr uint8_t RESULT_UNDEFINED_FAILURE =
    255u;
  // [...]
};  // struct LoadMap_Response_

All that to say, it looks like the IDL doesn't allow enum's in structs either, so the solution in ros2/test_interface_files@38fc073 of having a _Enums module looks fine to me.

[r7vme]

Small update here. Open PRs (#685) were paused from our side (initially mostly because of internal load). In the mean time, we've received a lot of internal and external feedback about current design / implementation.

On the positive side: this design and implementation is MVP and allows to "check the box" that enums are supported.

On the constructive side: People who already worked with Protobuf / FrancaIDL / OMG ODL find experience very frustrating due to many missing features in ROSIDL. Just one example, when we claim that enums are supported, expectations are that one can define enum once and use across all interfaces. This is not the case. See example below.

Lessons learned:

1. ROSIDL by design has a lot of ROS 1 legacy (single interface = single file). There are a lot of naming conventions (e.g. static module name hierarcy "<pkg.>::<msg|srv|action>"). It feels like, moving towards full features set of OMG IDL is the right way to go (multiple definitions, enums, unions, etc.), but keep supporting legacy naming conventions and ROS 1 interfaces like .msg, .srv, .action.
2. ROSIDL is very hard to extend. Mostly due to:
   1. ad-hoc parser implementation. I guess it was enough at the time and due to very narrow feature set. Possible improvement use Lark Transformer to generate ROSIDL intermediate model.ROSIDL intermediate model also will benefit having more AST features (e.g. abstract tree node and visitors), e.g. this will allow to do more advanced semantic analysis (e.g. type resolving)
   2. EmPy templates. This is really hard to extend those templates, mostly because they contain a lot of code / business logic. One suggestion can be to simplify templates by moving all code into a so called "view model", which will have all template specific values precompiled specifically for this template (from ROSIDL intermediate model). Another suggestion is may be to switch to Jinja2 templates, which is just very popular powerful template engine. EmPy seems not really actively developed (last commit in 2013).

I propose to pause existing solution (#685) and instead focus on improving ROSIDL tooling first. If high-level proposal (see lessons learned) looks ok, please let me know what can be next steps?

Example mentioned above. Not supported currently, but very common use case, reuse enums in other idl files

// GearBoxState.idl
module foo_msgs {
  module msg {
    enum GearBoxState {
      NEUTRAL,
      DRIVE,
      REVERSE,
      PARKING
    };
  };
};

// VehicleState.idl
#include "foo_msgs/msg/GearBoxState.idl"

module foo_msgs {
  module msg {
    struct FooMessage {
      foo_msgs::msg::GearBoxState enum_value;
    };
  };
};

cc @wjwwood @kylemarcey

[clalancette]

1. ROSIDL by design has a lot of ROS 1 legacy (single interface = single file). There are a lot of naming conventions (e.g. static module name hierarcy "<pkg.>::<msg|srv|action>"). It feels like, moving towards full features set of OMG IDL is the right way to go (multiple definitions, enums, unions, etc.), but keep supporting legacy naming conventions and ROS 1 interfaces like .msg, .srv, .action.

My biggest concern here (which has been a concern ever since we introduced support for straight .idl files) is fracturing the community. There are a lot of .msg files out there in the world, and I think one of the things people really like about ROS is the simplicity of those .msg files. If we switch some things over to .idl files, how do you combine .msg and .idl files? Also, the IDL file format isn't nearly as simple as .msg, so we are adding even more burden for people to learn ROS 2. Despite earlier comments, I think I'd actually be more onboard for introducing enum support (for instance) to .msg files, rather than wholesale switching over to .idl. That would keep the community on the same standard, and keep it easier to use ROS 2.

• ROSIDL is very hard to extend. Mostly due to:

Yes, you are definitely not the first one to complain about this. I'd be all for making ROSIDL easier to reason about and extend.

1. ad-hoc parser implementation. I guess it was enough at the time and due to very narrow feature set. Possible improvement use Lark Transformer to generate ROSIDL intermediate model.ROSIDL intermediate model also will benefit having more AST features (e.g. abstract tree node and visitors), e.g. this will allow to do more advanced semantic analysis (e.g. type resolving)

This seems worth exploring, though I think we need to keep performance in mind here. It is already really slow to generate the code, so I don't want to slow it down further. It isn't clear to me whether using something else will be faster or slower, but we definitely need to evaluate that.

• EmPy templates. This is really hard to extend those templates, mostly because they contain a lot of code / business logic. One suggestion can be to simplify templates by moving all code into a so called "view model", which will have all template specific values precompiled specifically for this template (from ROSIDL intermediate model). Another suggestion is may be to switch to Jinja2 templates, which is just very popular powerful template engine. EmPy seems not really actively developed (last commit in 2013).

For what it is worth, that is not the version of empy that we use. That was a fork at some point. The latest release of empy was in 2019: https://pypi.org/project/empy/#history

That said, I'd be much more interested in improvements to the current code rather than wholesale replacement. The fact of the matter is that our business logic is complicated, so I'm skeptical that a different templating engine (on its own) is going to yield significant readability benefits[1]. So I think we should see how much we can improve the current empy-based code rather than switching to something else.

[1] I definitely could be wrong here, but I think that would have to be demonstrated before we moved forward with a replacement.

[sloretz]

ad-hoc parser implementation. I guess it was enough at the time and due to very narrow feature set.

@r7vme A small improvement here would be docstrings on the functions. I find it's hard to review changes because it takes a while to figure out what a lot of them do.

EmPy templates. This is really hard to extend those templates, mostly because they contain a lot of code / business logic.

@r7vme +1 to the EmPy templates being very hard to read. It's very hard to see the difference between template logic and the static code - especially in rosidl_generator_py.

One suggestion can be to simplify templates by moving all code into a so called "view model", which will have all template specific values precompiled specifically for this template (from ROSIDL intermediate model)

@r7vme What do you mean by "view model"? Is this a common design pattern, and if so, got a page I can read about it?

Another suggestion is may be to switch to Jinja2 templates, which is just very
popular powerful template engine.

@r7vme If we end up rewriting the templates, I'd be all for switching to a more popular template engine

The fact of the matter is that our business logic is complicated, so I'm skeptical that a different templating engine (on its own) is going to yield significant readability benefits[1]

@clalancette It's hard to measure readability, but a more popular engine will have more tools to help with readability. There aren't a lot of tools or plugins for things like syntax highlighting (@dirk-thomas wrote a plugin for VSCode, but that's the only one I know of).

If we switch some things over to .idl files, how do you combine .msg and .idl files?

@clalancette Do you mean nesting types, like how would builtin_interfaces/Time stamp in std_msgs/msg/Header work if builtin_interfaces/msg/Time was an .idl file instead of an .msg file? The answer is it works now because both of them are already .idl files. Everything is converted to .idl before it's fed to the rosidl pipeline.

Despite earlier comments, I think I'd actually be more onboard for introducing enum support (for instance) to .msg files, rather than wholesale switching over to .idl. That would keep the community on the same standard, and keep it easier to use ROS 2.

@clalancette rosidl already wholesale switched over to IDL. Enums can't be added to .msg files until we support them in .idl files, because the .msg files get converted to .idl files before any generator sees them.

[wjwwood]

Personally I think the readability of our empy templates is due to how we use them, not the template engine itself.

If I'm not wrong the "view model" being referenced implies a template engine like jinja where it's essentially just substring replacement (with a few conditionals and stuff like that), part of the MVC (model-view-controller) pattern most likely, where the view is data driven and doesn't contain logic, but I believe that our use case is quite different from those use cases. And while we could use something like jinja, I'm not convinced it would be strict win over just making an effort to improve the readability of our existing templates using empy.

Also, to the idea that empy is a weird one-off template engine, I think it isn't as popular but there are many other engines that work in a very similar way, e.g. erb (embedded ruby). I think the reason that they are less popular is that the use cases for which they excel are less common, at least compared to use cases where engines like jinja or genshi are good, e.g. web development.

A better reason (in my opinion) to consider switching template engines would be for something like performance (as these templates do significantly impact our build times), rather than using something that's popular. While it might be nice to have an engine that more people are immediately familiar with, the reality is that very few people need to extend the typesupport pipeline and optimizing for developer familiarity it probably not the right focus (that doesn't mean we can't try to work on that too, just want to make sure we're spending our effort on the right things). Performance and suitability for the task seem like better things to optimize. So if we do switch template engines (a time consuming and potentially bug producing exercise) I would rather see us decide to do it for those reasons as opposed to personal preference or popularity.

Personally I would prefer to stay with empy, but either way that decision isn't going to make the templates magically more easy to read. In both cases we'll need to just spend time ensuring they are easy to read.

--

With respect to extending rosidl (by which I mean the "ROS interface description language", e.g. .msg files, and not the entire type support pipeline in ROS 2) vs just embracing OMGIDL (i.e. .idl files), I think that a pragmatic approach is best. I don't think we should be adding every feature to ROSIDL and I'm less concerned with fragmentation (some people using ROSIDL and some using OMGIDL) in the community than @clalancette, though I would strive to stick with ROSIDL in the core, just because it's simpler and keeps things consistent. But I would prefer people to "fragment" and use OMGIDL sometimes rather than spend lots of effort extending ROSIDL.

However, in the spirit of ROSIDL being easy for simple things, I think it would make sense to add enums to ROSIDL, assuming we can find someone to spend the time to do it. This is as opposed to something like adding bit fields or annotations or something like that to ROSIDL, in which cases I would personally prefer to just enable their use via OMGIDL and not try to add them to ROSIDL. So I think we should consider each use case on a case by case basis and decide if adding it to ROSIDL makes sense. And personally I would lean towards "do not extend ROSIDL" in most cases, but for enums in particular I would say we should add those to ROSIDL.

[wjwwood]

To the point of changing template engines and differentiating between using a more popular template engine vs using one that is appropriate for the task, I would be fine with trying to find a better supported engine (more popular) but would like to see us make sure it is more like empy (which was selected for its function more than its popularity), for example we could consider mako https://www.makotemplates.org/, which is more on the "logic in templates" side of things but also more popular than empy. Other engines like Jinja and Genshi are kind of in the middle with limited logic in the templates and Python's builtin string.Tempalte being kind of on the other end of the spectrum.

[r7vme]

What do you mean by "view model"? Is this a common design pattern, and if so, got a page I can read about it?

@sloretz Correct, in context of Jinja2 it's described also here. Quote from the page: View takes data from a Model and presents it (typically to a user)

[jasonbeach]

As a user who isn't familiar with the underlying implementation details, my only comment is +100 for the comment about build time performance. Generating the messages is by far the slowest part of our build. It would be kind of nice if it were easier to select which languages the messages were generated for... i.e for the most part, I don't care about the python or c messages...only c++, but I haven't found any examples on how to turn those off. Other than enums, the only other feature that would be really nice is something like protobuf's oneof type or some kind of union/variant.

…

On Fri, Nov 11, 2022, 1:42 AM Roman ***@***.***> wrote: What do you mean by "view model"? Is this a common design pattern, and if so, got a page I can read about it? @sloretz <https://github.com/sloretz> Correct, in context of Jinja2 it's described also here <https://flask-diamond.readthedocs.io/en/stable/developer/writing_views_with_jinja_and_blueprints/>. Quote from the page: *View takes data from a Model and presents it (typically to a user)* — Reply to this email directly, view it on GitHub <#260 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAUKSZ6ZKHQ7R5KLJOERWDLWHXTHBANCNFSM4EK2WMKA> . You are receiving this because you commented.Message ID: ***@***.***>

[bijoua29]

As another user who isn't familiar with the implementation details, I would agree with @wjwwood that supporting both ROSIDL and OMGIDL would be preferable. For simple use cases, you could continue to use the msg interface and ROSIDL. But for more complex use cases, OMGIDL might be required. A novice user can use the simpler format of ROSIDL without having to burden themselves to learn the OMGIDL. And I would agree with @wjwwood that it would be nice to at least add enums to ROSIDL. I will say that if you are a programmer and are used to creating nested structures and enums and unions in C++, creating OMGIDL-based idl is not very burdensome.

An added benefit, which is very relevant for us, is that you can easily communicate with native DDS applications by using OMGIDL. Currently, we are having to jump through hoops converting msg files to IDL that can be used by our native DDS applications. Obviously supporting OMGIDL will require the rest of the tooling e.g. 'ros2 topic', to also support OMGIDL, which presumably is a lot of work. But I think the benefit of more richer and readable types in OMGIDL is crucial to extending ROS2 to more applications and users.

[aditya2592]

I stumbled across this discussion while looking for enum support in ROS2. Thank you for starting this discussion! After reading through some of it, it is understandable that adding enums isnt very straightforward but I had an additional use case for enums in messages to point out nevertheless.

We have recently started using ROS2 with Rust in some of our projects and the increasing community support from libraries like r2r and rclrs for this is helping a lot. With Rust, there is an additional advantage to support enums in messages because once that is done, the Rust compiler warns about missing cases when using match statements and such. Being able to do this has made the code very easy to write by covering all the various branches it could have. We are manually creating enums for all constants to support this because the benefit it offers is immense. This enum use-case is indeed small in context of discussions surrounding ROSIDL but it has made code much more easier to write when working with ROS messages.

[oysstu]

(...) With Rust, there is an additional advantage to support enums in messages because once that is done, the Rust compiler warns about missing cases when using match statements and such. Being able to do this has made the code very easy to write by covering all the various branches it could have. We are manually creating enums for all constants to support this because the benefit it offers is immense. (...)

I've been doing the same in C++ with strongly typed enumerations. In that case, linters also warn about unhandled cases of switch statements. The compiler also keeps me from mixing up enumerations, e.g., lifecycle transitions and states, and generally leads to more readable code. For these reasons, I would favor enum class EnumType : uintX_t { ... } as the user-facing C++ interface.

[pefribeiro]

Hello, has this discussion made any progress? Apologies for commenting on such an old thread, but it seems to have been a while since an update was provided.