PR containing many things

[peterkrull]

Most of the stuff we already talked about in #27, but I will list the major things here again.

• A RawPacket owns its data, but push_bytes now may return a reference to the RawPacket used as a buffer within the PacketReader. This can be cloned as the user sees fit.
• The type (optionally) and CRC are now validated for the construction of a RawPacket, meaning those checks can be skipped later. push_bytes now also returns whichever errors are encountered. We use a "digest" style for the CRC, but not using the Crc library. Apparantly the Digest itself is consumed whenever it is finalized, making it a bit more awkward to work with. This custom method works well.
• One can choose any arbitrary combination of sync bytes, which uses bitflags internally and a builder pattern using a slice to configure the bytes. The config is stored in the reader itself and can only be set on creation using the builder.
• Iterators are returned from PacketReader::iter_raw_packets and PacketReader::iter_packets, which internally handles the iterative consuming of a given buffer.
• The Payload trait is imo more useful now, but there is still room for adding more nice functionality. The user of the crate can now directly get a complete RawPacket for whatever payload type they have constructed. Maybe it would still be nice to be able to fill the encoded data into an existing mut slice, but that is currently not supported here.

I have not had the extended format too much in mind with all this, so some things will obviously need modification after the fact.

[tact1m4n3]

Great. I see that it does fail some linting tests :)) That one with the len is pretty hilarious. You can maybe explicitly ignore all the warnings(with #[allow(...)])

[peterkrull]

Yeah I will get to those linter warnings in the next commit. Just thought I might as well get some feedback on the existing stuff and bundle those things together.

[tact1m4n3]

BufferError is pretty confusing. Because most(maybe all) errors come from invalid slice length I would probably rename it to something more specific and also pass in the expected minimum length as parameter.

I think the NoSyncByte error shouldn't exist. We already return None if there is no packet in the buffer.

The builder pattern for a buffer is I think unnecessary. Maybe we can just pass a config either in new(this is what I would prefer) or on each call.

Also, I would separate sync bytes from addresses(like passing in an array of sync bytes(of type u8) in the config).

These are just some things that I would change but if you think we should leave them as they are, that's still fine by me. Other than that, everything seems great and thank you for your contribution :))

[peterkrull]

I can try to make BufferError a bit more specific.

My reasoning for NoSyncByte is that if you pass in a slice, while the state machine is AwaitingSync, it is imo an error if we do not find any. Whereas if the state machine has started (gone past AwaitingSync), and we are consuming bytes normally, this is where we return None. Essentially every state should have an error condition, for when we did not accomplish what we hoped in that state. It ensures that if we ever consume bytes without it eventually resulting in a RawPacket, we return an error.

I agree the builder pattern is maybe a bit much for just 2 configurables. I would be okay with just supplying a config, so we have PacketReader::new and PacketReader::new_with_cfg, but we could poll @anti-social about that.

I would also like to separate the concepts of sync bytes and addresses, but that would just make the bitflag approach not work, since that relies on the relatively small number of addresses vs 256 different values in a byte. We could also have a heapless::Vec of u8 of some fixed length, e.g. Vec<u8, 8>. Then a user can have up to 8 of any arbitrary combination bytes. Or whichever number is reasonable. Though that would also mean that a call to new could fail if the slice is too long. Unless we assert and panic, or just document that it will ignore any slice longer than 8.

[tact1m4n3]

I would also like to separate the concepts of sync bytes and addresses, but that would just make the bitflag approach not work, since that relies on the relatively small number of addresses vs 256 different values in a byte. We could also have a heapless::Vec of u8 of some fixed length, e.g. Vec<u8, 8>. Then a user can have up to 8 of any arbitrary combination bytes. Or whichever number is reasonable. Though that would also mean that a call to new could fail if the slice is too long. Unless we assert and panic, or just document that it will ignore any slice longer than 8.

A solution would be to specify the config on every call and we could store the slice in the config struct with a lifetime. (I don't think this is such a good solution)

My reasoning for NoSyncByte is that if you pass in a slice, while the state machine is AwaitingSync, it is imo an error if we do not find any. Whereas if the state machine has started (gone past AwaitingSync), and we are consuming bytes normally, this is where we return None. Essentially every state should have an error condition, for when we did not accomplish what we hoped in that state. It ensures that if we ever consume bytes without it eventually resulting in a RawPacket, we return an error.

👍

[tact1m4n3]

We also could take in a static slice of sync bytes. It might fit better into an embedded application

[anti-social]

I would be okay with just supplying a config, so we have PacketReader::new and PacketReader::new_with_cfg, but we could poll @anti-social about that.

I think it's a matter of taste. For our simple case I would prefer a config. But a builder is also OK, possibly later there will be more configuration parameters.

I often see configuration pattern in embedded programming, particularly in Embassy.

[anti-social]

We also could take in a static slice of sync bytes. It might fit better into an embedded application

I also thought about storing a static array of addresses as there are only 14 items. But then @peterkrull suggested to use bitflags crate and although it is not an ideal solution but for no_std I'm pretty satisfied with it.

[anti-social]

@peterkrull Could you add a method to RawPacket to get sync byte of a packet?

[peterkrull]

I think it's a matter of taste. For our simple case I would prefer a config. But a builder is also OK, possibly later there will be more configuration parameters.

Me too, but the builder is a nice way to abstract away the conversion from &[PacketAddress] -> PacketAddressFlags. Of course we could make a lifetimed config, which can contain the slice, which we convert to a config using bitflags.

@peterkrull Could you add a method to RawPacket to get sync byte of a packet?

We could do that, but since RawPacket::as_slice() is available, is it really necessary? I only did the RawPacket::payload(). because that requires a slightly more complex destructuring.

[anti-social]

One point to a builder is that adding new method won't break a user code. With a config adding new field is a breaking change.

[anti-social]

We could do that, but since RawPacket::as_slice() is available, is it really necessary?

Yeah, we definitely can live without it but it is not a very convenient way especially because we have to deal with error handling:

raw_packet.addr()

// vs

PacketAddress::try_from(raw_packet.as_slice()[0]).unwrap()

[tact1m4n3]

Hey! What's the status on your PR?