Skip to content

Add the DynamicMessage type itself #292

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

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Conversation

nnmm
Copy link
Contributor

@nnmm nnmm commented Nov 5, 2022

As well as conversion from/to concrete message types.

It's clear logically that the alignment has to be 8, since C messages only contain pointers and primitive types of alignment less than or equal to 8 – with the exception of long double. Should we require dynamic messages to be aligned to 16 bytes to support long double? Even test_msgs doesn't use long double.

@nnmm nnmm force-pushed the dynamic_messages_4 branch from f169bff to 3ddd682 Compare November 7, 2022 12:48
@nnmm nnmm requested a review from a team November 10, 2022 18:55
@jhdcs
Copy link
Collaborator

jhdcs commented Nov 14, 2022

If ROS 2 itself supports long double, I'd say yes. If only because we want to match to spec. But I would be fine with just putting in a TODO<name>: for it if it's not a regularly used feature.

Do we know what alignment the ROS 2 standard uses, itself? Do different messages have different alignments depending on contents, or do they just make everything aligned to 16 bytes? Or is that a silly question?

Copy link
Collaborator

@jhdcs jhdcs left a comment

Choose a reason for hiding this comment

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

Only saw a single area where I had questions, otherwise this looks pretty good to me!

// SAFETY: The pointer returned by get_type_support_handle() is always valid.
let type_support = unsafe { &*self.type_support_ptr };
let message_members: &rosidl_message_members_t =
// SAFETY: The data pointer is supposed to be always valid.
Copy link
Collaborator

Choose a reason for hiding this comment

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

The "supposed to be" wording makes this sound like there are instances where the data pointer may not be valid. Is that correct? Is there anything that we can do to detect or defend against that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Someone could always create a shared library containing invalid pointers – but there's no way of protecting against that, I think it's just out of scope for Rust's safety guarantees. The same already holds for any of the other libraries that we link against, like rcl – if we can't trust those, then it's game over anyway.

@nnmm
Copy link
Contributor Author

nnmm commented Nov 14, 2022

If ROS 2 itself supports long double, I'd say yes. If only because we want to match to spec. But I would be fine with just putting in a TODO<name>: for it if it's not a regularly used feature.

I don't know, but you know what, I'll just make it 16 bytes, it doesn't cost us anything really.

Do we know what alignment the ROS 2 standard uses, itself? Do different messages have different alignments depending on contents, or do they just make everything aligned to 16 bytes? Or is that a silly question?

There is no prescribed alignment, it depends on the contents. So a message containing only u16 fields would have alignment of 2 bytes for instance.

@esteve
Copy link
Collaborator

esteve commented Feb 19, 2023

@nnmm do you want to merge this one or shall we wait?

@esteve esteve merged commit 50d42c5 into dynamic_messages_3 Jan 9, 2024
@delete-merged-branch delete-merged-branch bot deleted the dynamic_messages_4 branch January 9, 2024 14:48
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.

3 participants