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

Add PacketOwned variants #44

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

maximeborges
Copy link
Collaborator

@maximeborges maximeborges commented Sep 26, 2022

This PR allows to copy parsed packets into data-owned packets that can be moved to other threads.

I'm not really happy about duplicating all the packets and having big matching tables here, but I'm not really sure how else this could be implemented. Maybe refactoring so you have PacketOwned as a base data structure that you can borrow from to get PacketRef.

Also unintentionally run cargo fmt so it cleaned up some other code, which is not that bad in the end (but I can split that into two commits at least if it helps for the review).

This allows to copy parsed packets into data-owned packets that can be
moved to other threads etc.
@gwbres
Copy link
Collaborator

gwbres commented Sep 26, 2022

@maximeborges very good contribution 👍

/// Packet not supported yet by this crate
#[derive(Clone, Debug)]
pub struct UbxUnknownPacketOwned {
pub payload: Vec<u8>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to put this behind the alloc flag? (for clients who haven't defined an allocator)

match packet {
#(#matches_ref_to_owned)*
#union_enum_name_ref::Unknown(#unknown_var_ref {payload, class, msg_id}) => {
PacketOwned::Unknown(#unknown_var_owned { payload: payload.to_vec(), class: *class, msg_id: *msg_id })
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is the only place affected by my alloc comment above, correct me if I'm wrong.

@lkolbly
Copy link
Collaborator

lkolbly commented Oct 9, 2022

Thanks for the PR! I concur, it's a little weird that we have to duplicate the struct (and methods). I think we do have to do that, though: If the owned variant was a base that the ref referred to, then I don't think we could do the zero-copy thing that Ref can do (where it references some buffer that the user holds)

...that said, maybe we could do something clever with traits and default methods. You could imagine a trait like so:

pub trait SomePacket {
    fn latitude(&self) -> u32 {
        u32::from_le_bytes(self.bytes()[0..=3].try_into().unwrap())
    }
    
    fn bytes(&self) -> &[u8];
}

and then you could have PacketRef and PacketOwned each implement the bytes method, like so:

pub struct PacketRef<'a>(&'a [u8]);
pub struct PacketOwned([u8; 4]);

impl<'a> SomePacket for PacketRef<'a> {
    fn bytes(&self) -> &[u8] {
        self.0
    }
}

impl SomePacket for PacketOwned {
    fn bytes<'a>(&'a self) -> &'a [u8] {
        &self.0
    }
}

Which also lets you write code that's agnostic to ref or owned:

fn extract_field(bytes: impl SomePacket) -> u32 {
    bytes.latitude()
}

pub fn cvt_bytes(bytes: [u8; 4]) -> u32 {
    let p = PacketOwned(bytes);
    extract_field(p)
}

Which, as always, compiles entertainingly well.

Anyway, I'm sure we could go on forever down that rabbit hole. Maybe we can roll it into #43 somehow. For now though, this looks good, barring my comment about alloc.

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