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

Use new Serialization and zenoh-ext API #279

Merged
merged 4 commits into from
Oct 1, 2024

Conversation

Charles-Schleich
Copy link
Member

@Charles-Schleich Charles-Schleich commented Sep 30, 2024

Porting Ros2dds plugin over to new Serialization API
Tracking issue
eclipse-zenoh/zenoh#1474

Note: update Dependencies when eclipse-zenoh/zenoh#1473 is merged

Copy link

PR missing one of the required labels: {'enhancement', 'breaking-change', 'bug', 'dependencies', 'new feature', 'documentation', 'internal'}

@Charles-Schleich Charles-Schleich added enhancement New feature or request release Part of the next release internal Changes not included in the changelog labels Sep 30, 2024
@@ -228,16 +228,22 @@ impl CddsRequestHeader {
buf[16] = self.is_little_endian as u8;

hashmap.insert(ATTACHMENT_KEY_REQUEST_HEADER, buf);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure why a hashmap was used... probably for ease of serialization via ZBytes::from_iter(hashmap.iter()).
But now that this ease is gone, I don't see the point of this hashmap. We could just do:

writer.append(ZBytes::from(ATTACHMENT_KEY_REQUEST_HEADER));
writer.append(ZBytes::from(buf));
writer.finish();

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed Hashmap, packing bytes directly.

}
}

impl TryFrom<&ZBytes> for CddsRequestHeader {
type Error = ZError;

fn try_from(value: &ZBytes) -> Result<Self, Self::Error> {
let hashmap: HashMap<[u8; 3], [u8; 17]> =
Copy link
Member

Choose a reason for hiding this comment

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

On receiving side, we expect the the attachment value to contain only:

  • the ATTACHMENT_KEY_REQUEST_HEADER
  • the header (16 bytes)

Rather than using a hashmap, we could just do:

  • try to read len(ATTACHMENT_KEY_REQUEST_HEADER) bytes (return Err if fail)
  • check if those bytes are equal to ATTACHMENT_KEY_REQUEST_HEADER (return Err if fail)
  • try to read 16 bytes (return Err if fail)

Copy link
Member Author

@Charles-Schleich Charles-Schleich Oct 1, 2024

Choose a reason for hiding this comment

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

Reading Bytes from slice, failing if not enough bytes are found or if bytes do not match ATTACHMENT_KEY_REQUEST_HEADER values.

@JEnoch JEnoch merged commit 5acac6b into eclipse-zenoh:main Oct 1, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request internal Changes not included in the changelog release Part of the next release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants