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

[wip] Optionally parse sync (address) byte #22

Closed
wants to merge 1 commit into from

Conversation

anti-social
Copy link
Contributor

To be honest I'm not sure it is good solution for now.

It provides several constructors to get PacketReader that produces different RawPacket types.

Another option is to put address: Option<PacketAddress> field into RawPacket and Packet structs but the drawback is that we will have to check it before access (or unwrap).

@tact1m4n3
Copy link
Owner

tact1m4n3 commented Apr 25, 2024

I think the buffer struct is a good idea anyway.
In relation to the address encoder decoder generics, it maybe kind off confusing for anyone trying to use the crate(that just wants to parse some packets from a rx :)) ). Maybe we can make it a feature?

@anti-social
Copy link
Contributor Author

I can imagine situation when someone will need both readers. For example some kind of a proxy between an i2c device and a uart one.

@tact1m4n3
Copy link
Owner

We should probably ignore i2c for now as we don't have any examples of it being used. Also, as a side note, there is no specification that the sync byte is actually a packet address :))

I noticed in your code that the header length is no longer a constant. Why is that?

@peterkrull
Copy link
Contributor

I don't like adding too many things, such as generics or optionals, that will have no use for the +90% of users that might just want to parse RcChannelsPacked. I am still a proponent of the owned RawPacket, and I think the simplest version would suffice (I have opted for a heapless::Vec in my wip) where the sync/addr byte should be manually re-evaluated if used for routing:

/// Represents the not-yet-parsed data of a packet.
pub struct RawPacket(Vec<u8, 64>);

or alternatively, with a PacketAddress always present:

/// Represents the not-yet-parsed data of a packet.
/// The `addr` field represents the "sync" byte used
/// to find the start of the packet, which is sometimes
/// also used as a destination address.
pub struct RawPacket {
    addr: PacketAddress, 
    buf: Vec<u8, 64>,
};

And then still use the PacketAddressFlags bitflags when searching for the sync/address byte. The 90% of users will probably never even bother to work with the RawPacket anyway, and would just want the finished parsed Packet.

Since this use case is not really a part of the spec, I would not want many things relating to it to bleed out where others may stumble into it. Wouldn't it be enough to to have the sync byte be customizable via a method, and the contents of the RawPacket available?

@tact1m4n3
Copy link
Owner

I am still a proponent of the owned RawPacket

It's fine by me. It's almost certain that it won't be a performance issue and it can make the api easier to work with

@tact1m4n3
Copy link
Owner

Wouldn't it be enough to to have the sync byte be customizable via a method, and the contents of the RawPacket available?

I agree with you. Since it's something not in accordance with the spec we shouldn't support it make everything so much more complicated. Making a method is an option, as well as having a list of allowed sync bytes that would contain 0xC8, 0xEE and others that may appear and having a sync() -> u8 method in the raw packet struct that would return that byte.

@@ -0,0 +1,40 @@
pub(crate) struct Buf<const C: usize> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not almost just a heapless::Vec, except with a predefined type?

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