Generate MISRA compatible constants

[mjcarroll]

Feature request

Feature description

It has been brought to our attention that the message constants that are generated for ROS2 are not compatible with writing MISRA-compliant code.

Let's consider a ROS2 message file PointBlock.msg that apart from other things containing a constant:

...
uint32 CAPACITY=256
...

One of resulting generated files is PointBlock.msg point_block__struct.hpp:

...
  // constants
  enum { CAPACITY = 256u };
...

Now because of CAPACITY is a class-less enumeration and is not signed, we can't use it in assignments to and comparisons with uint32_t variables.

implicit conversion from 'unsigned int' to 
'enum apex_app_msgs::msg::PointBlock_<std::allocator<void> >::(anonymous at point_block__struct.hpp:113:3)'
changes signedness of underlying type

[mjcarroll]

@dirk-thomas Do you recall the reason why rosidl wraps constants in an enum?

[mjcarroll]

I think that the issue stems from the fact that the underlying type of the enum, which I believe is at least a signed type (I think width is implementation specific). Because of this, it makes the implicit conversion have comparison between different signs.

A few alternatives would be:

• Explicitly type the enums (available in C++11 and up): enum: uint32_t { CAPACITY = 256u; }
• Use static const for integral constants, the way we do for everything else. I noticed that we don't consistently use one way or another.

  // constants
  static const bool BOOL_CONST;
  enum { BYTE_CONST = 50 };
  enum { CHAR_CONST = 100 };
  static const float FLOAT32_CONST;
  static const double FLOAT64_CONST;
  enum { INT8_CONST = -50 };
  enum { UINT8_CONST = 200u };
  enum { INT16_CONST = -1000 };
  enum { UINT16_CONST = 2000u };
  enum { INT32_CONST = -30000 };
  enum { UINT32_CONST = 60000u };
  enum { INT64_CONST = -40000000 };
  enum { UINT64_CONST = 50000000u };
  static const std::basic_string<char, std::char_traits<char>, typename ContainerAllocator::template rebind<char>::other> STRING_CONST;

[dirk-thomas]

Do you recall the reason why rosidl wraps constants in an enum?

This is how it is in ROS 1.

[gbiggs]

Since ROS 2 uses C++11, I think that explicitly typing the enums seems like the best option if MISRA accepts that. (I haven't read MISRA in a few years, and not the version updated for C++11, so I don't know if it does.)

[dejanpan]

MISRA C++2008 still only supports C++2003. New MISRA C++2018 standard will support C++2014 and we (have a reason to) believe that it will largely overlap with Autosar's document. They in section 6.7.2 suggest to use "explicitly type the enums" which at least to me feels also quite convenient while still safe.

[serge-nikulin]

Why not explicit integral const? The pros are:

• const will be consistent for integers, floats, and strings.
• const solves the conversion problem.
• MISRA C++:2008 recommends const before anything else.

What are the cons to const?

@dejanpan, can you please ask MISRA committee whether the upcoming standard will allow silent conversions between enum classes and integers of the same "signess"? If I remember correctly Ada language does not allow such conversions so MISRA might prohibit them in C++ too.

[mjcarroll]

@serge-nikulin Typing the enums should not make them an enum class, they will retain the properties of a normal (pre-C++11) enum, with the underlying type explicitly stated: http://en.cppreference.com/w/cpp/language/enum

The enum class construct is a new addition in C++11, and could also be used, but changes the scope of the enumerated values.

[wjwwood]

So, what is the answer to @serge-nikulin's question? Why not const everywhere?

Is there any reason not to move away from enums everywhere and use const globals instead?

[gbiggs]

I checked Scott Meyer's C++11 book but it didn't have any specific advice. My impression of best practice is that if it's a single value, which these seem to be, using global const values is probably better. enum classes are better if you have a set of identities that you want to have the same type (e.g. enum class Colour {red, green, blue}).

[mjcarroll]

I think that const probably makes the most sense, as long as we are okay with moving away from the enum, which is the "legacy solution" at this point.

[mjcarroll]

@serge-nikulin Do you know how this affects the MISRA C compliance? It appears that I am unable to use the same fix for C as I did C++, because static variables in C must be initialized with literal constants or enum members, so it makes code like this https://github.com/ros2/rcl/blob/master/rcl_lifecycle/src/default_state_machine.c#L40 invalid.

[serge-nikulin]

@mjcarroll, I am not sure I understand the question.
MISRA C:2012 is OK about static initializers, assuming they are not partial and use correct literals (e.g. unsigned values have capital U suffix).

Also, your quoted example looks good to me (I have not tried to build it, tho)

rcl_lifecycle_state_t rcl_state_unknown = {
  "unknown", lifecycle_msgs__msg__State__PRIMARY_STATE_UNKNOWN, NULL, NULL, 0
};

[mjcarroll]

The issue is that if we change the definition of lifecycle_msgs__msg__State__PRIMARY_STATE_UNKNOWN from an enum (what it originally was) to a static const TYPE (what we had to do in C++), it makes that quoted example not build, because the struct initializer must take literal constants or enum values.

The question is, is the fix we did for C++ (enum -> static const) necessary in C to fulfill MISRA compliance, because it appears to break other things downstream.

[serge-nikulin]

Thanks, I see it now.

The question is, is the fix we did for C++ (...) necessary in C to fulfill MISRA compliance...

No, it is not necessary if the generated C code is not a part of a safe app.

I think it's OK to leave C code generation as is because the generated C code is not a part of a potential safe app but (AFAIK) is a part of Python app (unsafe by default).

[mikaelarguedas]

As the C structures can be used in critical apps (e.g. pure C implementations). Surprisingly the C MISRA checkers didn't catch that violation that's why we were wondering if that was a requirement for MISRA C compliance.

@serge-nikulin

The question is, is the fix we did for C++ (...) necessary in C to fulfill MISRA compliance...

No, it is not necessary if the generated C code is not a part of a safe app.

I read this as : "It is necessary to achieve MISRA C compliance if the generated C structures are used in a safe app". Is that correct ?

[serge-nikulin]

Surprisingly the C MISRA checkers didn't catch that violation that's why we were wondering if that was a requirement for MISRA C compliance.

I found that MISRA C:2012 (four years after MISRA C++:2008) is much easier to live with. I often drop down to C99 to implement a feature just because it's much easier to be MISRA compliant in C99. Hopefully, MISRA C++:2018 will fix that mindless self-conflicting churn of the current '2008 revision.

"It is necessary to achieve MISRA C compliance if the generated C structures are used in a safe app". Is that correct?

Yes, that's correct. Or provide a way to MISRA compliant code. The currently active C++ implementation has this second provision but the way painful: a developer has to employ static cast on every use of a message constant.