-
Notifications
You must be signed in to change notification settings - Fork 10
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
Conversation
c32628e
to
1f04332
Compare
1f04332
to
265e084
Compare
I think the buffer struct is a good idea anyway. |
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. |
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? |
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 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? |
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 |
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> { |
There was a problem hiding this comment.
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?
To be honest I'm not sure it is good solution for now.
It provides several constructors to get
PacketReader
that produces differentRawPacket
types.Another option is to put
address: Option<PacketAddress>
field intoRawPacket
andPacket
structs but the drawback is that we will have to check it before access (or unwrap).