Skip to content

feature/implemented_serialization_for_more_array_types#513

Open
TijlJappens wants to merge 4 commits intos2e-systems:mainfrom
TijlJappens:feature/implemented_serialization_for_more_array_types
Open

feature/implemented_serialization_for_more_array_types#513
TijlJappens wants to merge 4 commits intos2e-systems:mainfrom
TijlJappens:feature/implemented_serialization_for_more_array_types

Conversation

@TijlJappens
Copy link
Copy Markdown
Contributor

I implemented serialization for more array types and introduced a macro to keep code bloat manageable.

@TijlJappens
Copy link
Copy Markdown
Contributor Author

The failed test is strange, somehow, when running it locally I don't get the same problem (or maybe I get it occasionally), any idea how this is possible?

I will investigate the failed interopability test locally.

@TijlJappens
Copy link
Copy Markdown
Contributor Author

TijlJappens commented Mar 5, 2026

I have no idea if the interoperability test failure is a result of my changes.

@jrebelo
Copy link
Copy Markdown
Contributor

jrebelo commented Mar 5, 2026

I have no idea if the interoperability test failure is a result of my changes.

It most likely is since the failure seems to be consistent. You probably changed the way data is serialized without intending it. If the message is different then the compatibility is gone.

@TijlJappens
Copy link
Copy Markdown
Contributor Author

I ran the interoperability test on my own fork:

https://github.com/TijlJappens/dust-dds/actions/runs/22725408000/job/65899122931

and there the FastDDS interoperability succeeded, so it does seem to be something intermittent...

Copy link
Copy Markdown
Contributor

@stkimmer stkimmer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me, except the macro

.insert(id, DataStorage::SequenceComplexValue(value));
Ok(())
}
impl_get_sequence_values!(get_int32_values, SequenceInt32, i32);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can that be done without a macro? Macros generally lower code understanding (macros need to learned first)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made a new commit without a macro but it does involve a bit more code repetition and a new trait.

@carlocorradini
Copy link
Copy Markdown
Contributor

Great work!
Would it be possible to add support for enums in both sequences and arrays? 😍

@TijlJappens
Copy link
Copy Markdown
Contributor Author

Great work! Would it be possible to add support for enums in both sequences and arrays? 😍

I am a bit confused about the way that Enums are handled currently so I don't dare to touch it. I mean, why does all enum parsing go through the deserialize_structure function? I would think enums are just numbers and that the deserialization would call an integer deserialization but it does:

        TypeKind::ENUM => dynamic_data.set_complex_value(
            member.get_id(),
            self.deserialize_complex_value(&member_descriptor.r#type)?,
        ),

?

I mean that eventually goes through the struct parsing which will turn it into it into a number but I find the design a bit strange. Could we not just call something like:

         TypeKind::ENUM => {
            match member_descriptor
                .r#type
                .get_descriptor()
                .discriminator_type
                .as_ref()
                .unwrap()
                .get_kind()
            {
                TypeKind::INT8 => dynamic_data
                    .set_int8_value(member.get_id(), self.deserialize_primitive_type()?),
                TypeKind::INT32 => dynamic_data
                    .set_int32_value(member.get_id(), self.deserialize_primitive_type()?),
                d => panic!("Invalid discriminator {d:?}"),
            }
        }
        
Here directly and remove the ENUM option from the match in deserialize_structure? Or what exactly do you mean with:

    // Deserialize the top-level which must be either a struct, an enum or a union.
    // No other types are supported at this stage because this is what we can get from DDS
    
    ?

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

Successfully merging this pull request may close these issues.

4 participants