BPIO binmode beta crashes:
Here are the crashes I’ve caused in the BPIO firmware, as mentioned in the forum thread. These are all ultimately malformed input but ideally they’d be rejected before crashing by the flatbuffer verifier. I didn’t have a probe hooked up so I’m judging “crash” by the LED animation freezing and the terminal becoming unresponsive.
A 4-byte size prefix (0x30 0 0 0) or no size prefix (first four bytes 0x10 0 0 0, replace 48 below with 16) cause only the length prints to occur. I imagine it’s choking on the “data” starting with zeroes.
At the moment it’s very raw, I haven’t gone into verifiers yet. Does the two byte length prefix work though?
You’re right, the flatcc builder version does take the length as a variable.
All around error handling (packets, bus, client side) seems to be the next thing to work on. I’d like to hunt down the slowness issue first though to make sure this isn’t wasted effort.
I believe most of the above crashes could be resolved by the automatic function that verifies the flatbuffer contents vs. the size of the actual packet.
As of 2b36c77 these examples no longer cause a crash, thanks Ian!
One further problem, though, is that after seeing and rejecting the invalid size prefixes, the Bus Pirate tries to parse the following data – looking for a size prefix and the data afterwards.
For instance, given an invalid 4-byte LE size prefix 0x30 0x00 0x00 0x00, and the following data packet:
The Bus Pirate attempts to read (and rejects) two packets:
[BPIO] Flatbuffer received, length: 48
[BPIO] Error: Invalid flatbuffer packet received, verify returned table header out of range or unaligned
[Error Response] Invalid flatbuffer packet received
[Send Packet] Length 84
[Send Packet] Finalized buffer
[BPIO] Received length: 0
[BPIO] Flatbuffer size error: 0
[Error Response] Flatbuffer size error: Invalid length
[Send Packet] Length 88
[Send Packet] Finalized buffer
It seems like we need a way to reject the rest of the sent bytes. Looking at bpio.c, do you think that the count returned from tud_cdc_n_available be sufficient to skip past the garbled packet?
This requires packetization / framing… which is one more encoding layer. In essence, there’s currently no way to know where the start of the packet is right now. The current thinking is to use COBS. This will add negligible overhead, and allow reliably detecting the start/end of packets. I think it’s next on Ian’s list.
The 2 byte length was just for ease of prototyping. To make it bullet proof we need to wrap it in one more layer.
Inside I feel like this is overkill, as the old Bus Pirate had no protection whatsoever and getting stuck in transactions was pretty rare. Other than malformed packets during development, which will timeout after 500ms, I’m not sure when it would really be an issue. We’re not going to have network packet loss or data corruption over USB. A USB serial port can’t be connected in the middle of a transaction like a physical serial port.
But, if it is simple enough, we should do it for professionalism’s sake.
At the moment I want to focus on buffer lengths and memory management to ensure we can get the best speed with large packets (an no crashing).
IMHO, using COBS is not overkill, but rather a foundational requirement for reliable serial communications of packetized data. In particular, it ensures recovery when something goes awry.
Have you found a decent MIT-licensed, C11 library for COBS?
If not, I could create one fairly rapidly, and would base it loosely around the rust COBS crate’s API (since it already defines a conceptual framework that seems reasonable).
Let me know if a C11 library is desired, and I can generate the header / API surface. In fact, I might do this anyways, as it’s a great functionality to have generally available.
I remember having seen a version that supported various decode options (e.g., buffer-in-place decode, or using two distinct buffers). I cannot find that currently.
Decode recommendation
Because the use case here would always have the entire packet in-memory at one time (to support flatbuffers), I recommend a COBS API that decodes in-place (no second buffer).
Handling input stream then looks like:
( Initialization between packets: zero valid bytes )
if non-zero incoming byte, add valid byte to buffer (or set error for too large a packet)
else if buffer is empty, do nothing
else ask for COBS to decode an entire packet … providing (buffer + length)
Where the input is not malformed, the decoder has zero failure points … it won’t ever fail. By decoding the packet in-place, it also provides the valid length of the packet, which is required for flatbuffers to properly validate the packet.
The only malformed packet type is where the last reference to where the next zero-byte should occur points past the end of the buffer. This can be simply scanned before doing any overwrite of data, so that the malformed data can be dumped in debugging.
Benefits:
Very simple decoding.
Very simple error conditions.
No extra memory buffers.
Minimal stack overhead.
Encode recommendation
The maximum packet size can be determined at compilation time based on the largest message size:
This allows pre-allocation of the largest required buffer.
Encoding can also be RAM-efficient:
Verify the incoming message size <= MaxMessageSize
Verify the buffer provided is large enough for worst-case encoding.
Move the buffer contents forward to align with the end of the buffer (at 100Mhz+, this is negligible)
Encode in-place … no extra buffers required
The desired API is conceptually then:
// returns zero for errors / malformed data
size_t cobs_inplace_decode(void* buffer, size_t encoded_bytes);
// Errors: buffer_bytes is too small for encoding data, or valid_bytes being zero
// On error, the incoming buffer is left unmodified.
bool cobs_inplace_encode(void* buffer, size_t valid_bytes, size_t buffer_bytes);
This is an MIT-licensed implementation that appears to support inline encode and inline decode, has unit tests, and (if later need to use it) even supports incremental encoding:
I’m going to have a look at this now as it may impact how we do our memory planning. I’ll read the docs, I’m super curious if it contains a length header itself.
ETA:
Okay, I understand this now.
When non-zero data is available stuff it in the buffer.
When 0x00, end of buffer
Pass buffer and received length to COBS
Get decoded length and actual packet from COBs
Proceed as normal with flatbuffers
BTW: I’m using the flat buffer validator that does not take length as a variable. The version with length always failed, or maybe it was some other failure. I’ll try again.
COBS encoding just one direction (python script to Bus Pirate) with a 512 byte read size decreases the average speed by ~7K/s. That’s only one way and not the data intensive side.
Super easy to implement though. Let’s get it going in the other direction.
COBS encoding the Bus Pirate response has a few issues:
We now need a second temporary buffer to encode into, instead of writing directly from the flat buffer. This is currently limited to <700bytes, and will limit our ability to increase the max read packet size for better speeds.
A second point that has slipped my mind but was very urgent. It’ll come back to me.
It appears to be slooooowww
Ave rate KB/s
No COBS
77.5
COBS RX
71.3
COBS RX/TX
30.8
I’m inclined to take the small hit on the RX side so the Bus Pirate can handle garbled packets, but taking a 60% speed hit on the TX side seems unfortunate.
If you’re using 0x00 as the end-of-packet byte, I wonder if it would help to change it to something else? (Encoded flatbuffers seem to have a lot of zeroes.)
That seems trivial on the C side (change the value in the header file), but on the python side it looks like something that is just not done. There’s no apparent option for it in the primarily python cobs module.
