Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CycloneDDS subscriber fails to deserialize a XTypes appendable enumeration #1926

Open
jeffpg144 opened this issue Jan 30, 2024 · 2 comments
Open

Comments

@jeffpg144
Copy link

Summary

CycloneDDS subscriber fails to deserialize a XTypes appendable enumeration from a publisher sending a new enumerated value.
[localhost build]$ idlc -v
idlc (Eclipse Cyclone DDS) 0.11.0

Hello World, #1

  1. Start with examples/helloworld.
  2. Make existing types Appendable.
  3. Add enumeration.
  4. Update publisher and subscriber to set and print new enumeration.

Hello World, #2

  1. Make a copy of examples/helloworld as helloworld2.
  2. Update IDL by appending a “long newID” to the topic data type.
  3. Update IDL by appending a new color to the enumeration. (CA_RED ordinal of 3)
  4. Update publisher to send values for the newID and new enumeration.

Common Changes to Hello World Example

  1. Subscriber calls dds_take instead of dss_read so samples are only printed once.
  2. Subscriber continues to read additional samples and does not exit after the first sample.

Test Results

Pass
o HW #1 pub to HW #1 sub
o HW #1 pub to HW #2 sub
o HW #2 pub to HW #1 sub when sending existing enumerated color.
o HW #2 pub to HW #2 sub
Failed
o HW #2 pub to HW #1 subs when sending new enumerated value/color.

The follpwing is printed to the console.
“1706620617.096516 [0] recvMC: data(application, vendor 1.16): 110fd27:cd3514d3:a0cdd270:202 #2: deserialization HelloWorldData_Msg/HelloWorldData::Msg failed (for reasons unknown)”

Expected Behavior
• Expect HW #1 subscriber to deserialize published data and assigned the enumeration with a default value of 0. This is what happens for the newID field, HW #2 subscriber has a value of 0 for published samples from HW #1.
• Other OMG-DDS vendors code generate an additional value for all enumerations that indicate an “UNKNOWN VALUE” or “BAD VALUE”. Looking at the generated c code, I do not see an additional enumerated value.
• After reviewing X-Types Release Notes https://github.com/eclipse-cyclonedds/cyclonedds/blob/master/docs/dev/xtypes_relnotes.md this is no mention of appendable enumerations not being supported.

Things I’ve Already Tried

• Verify the IDL syntax for extensibility is correct. Using “@appendable” before the enumeration and structure
• Make sure the structure containing the enumeration is also appendable.
• Updated the idlc command line to include -Werror -x appendable -f cdrstream-desc

Source code for both Hello World #1 and #2.

examples_helloworld.zip

I have ~5 years experience working with other OMG-DDS vendor software. I have recently started to test interoperability with CycloneDDS focusing on XTypes.

@eboasson
Copy link
Contributor

eboasson commented Feb 5, 2024

Hi @jeffpg144, you raise an interesting issue. I would say that what Cyclone does is in line with the specs:

XTypes 1.3, 7.2.4.4.7 "Enumerated Types", table 18 gives for ENUMERATION_TYPE assignability:

ENUMERATION_TYPE if and only if:

  • T1.extensibility == T2.extensibility
  • Any literals that have the same name in T1 and T2 also have the same value, and any literals that have the same value in T1 and T2 also have the same name. This behavior may be modified with the @ignore_literal_names annotation, see 7.3.1.2.1.11.
  • If extensibility is final T1 and T2 have the same literals.

and for "Object construction":

Choose the corresponding T1 literal if it exists.

If the name or value of the T2 object does not exist in T1, the object cannot construct any object of type T1.

I read that "the object cannot construct any object of type T1" that the deserialised must reject the sample. I suppose this is debatable because makes the concept of @appendable on an enumerated type somewhat doubtful.

But then, this:

Other OMG-DDS vendors code generate an additional value for all enumerations that indicate an “UNKNOWN VALUE” or “BAD VALUE”. Looking at the generated c code, I do not see an additional enumerated value.

Is a tad problematic at least formally on the grounds of IDL 4.2, 7.4.1.4.4.4.3 "Enumerations", which says

The list of the possible values (enumerators) that makes the enumeration, enclosed within braces ({}). Each enumerator is identified by a specific name (). In case there are several enumerators, their names are separated by commas (,). An enumeration must contain at least one enumerator and no more than 2^32

So technically it is possible to do 2^32 symbols that get mapped to a 32-bit integer in C (most traditional C99 implementations) and hence that "bad value" may not be possible. It is a bit formalistic, I admit.

More problematic to me is that the IDL-to-C++ language mapping spec states:

6.9 Mapping for Enums

An OMG IDL enum maps directly to the corresponding C++11 type definition. When an enum is used in a structured type, its default value is the first enum value specified.

The IDL-to-C++ language mapping spec is oldish, the one for Java is much newer (2019, IDL4, so post-XTypes):

7.2.4.3.3 "Enumerations"

An IDL enum shall be mapped to a Java public enum with the same name as the IDL enum type in Pascal Case, following the transformation rules defined in 7.1.1.1.1.

The Java enum type shall include a list of the enumerators, a private member to hold the value, and a private constructor to initialize the enumerators with the constant value and name. Additionally, the Java enum type shall have the helper method valueOf(int) to get an enumerator instance from an int.

For example, the IDL:

   enum AnEnum { @value(1) ONE, @value(2) TWO };

Maps to:

    public enum AnEnum {
        ONE(1),
        TWO(2);

        private int value;
        private AnEnum(int value) {
            this.value = value;
        }
        public int getValue() {
            return value;
        }
        public static AnEnum valueOf(int v) {
            // return ONE, TWO, or raise java.lang.RuntimeException
        }
};

So also no additional magic values.

I can't find anything that says these rules do not apply to "appendable" enumerate types. So it is at best murky territory ...

There is an obvious solution to it that I like much better than defining magic additional symbols, which is to leave it to the application. An annotation that specifies which value to use for out-of-range inputs when deserialising, perhaps with a fallback to the "default literal" would be much better, in my view.

But right now, no, Cyclone doesn't do that either ...

@jeffpg144
Copy link
Author

@eboasson Thanks for the detailed comments. My following up thoughts.

  1. By design, X-Types most expect an old subscriber to de-serialize an appended enumeration value with-out error. The questions is, what enumerated value should be returned. (See Where is vortexddslauncher? #3 and various fixes and other changes from atolab/cdds repository #4 for two possible answers)
  2. Agree that following other OMG-DDS vendor conventions of generated a "BAD VALUE" enumeration is not ideal but I wanted to share what I've learned from other OMG-DDS vendors.
  3. Page 45, "Table 9 – Default values for non-optional members" of OMG-DDS spec "Extensible and Dynamic Topic Types for DDS Version 1.3" , ENUM_TYPE should use "The first literal in the enumeration." is another value does not match. With out fully understand all the OMG-DDS specifications, this seems like a clear requirement for how Cyclone DDS could behave.
  4. I completely agree with giveing control to the application. Per X-Types 1.3, page 81 "7.3.1.2.1.10 Default Literal for Enumeration" "@annotation default_literal {};" would allow the application to specify the default enumeration.

It seems like Cyclone DDS could consider #3 first and add #4 in the future.

My application does depend on appendable enumerations and the other organizations I'm collaborating with are interested in Cyclone DDS but this would be a show stopper for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants