Design questions around CDC stream handling and loading

[iyavv]

I'm running into a some errors with running 32blit flash and have some design questions with respect to the flash loading process and robustness.

Receive buffer design

This is a bit of a nit, but why is CDCDataStream not designed as a circular buffer? The memmove at the beginning would not be required, and the resulting portion of AddData would be either 1 or 2 memcpy's. Given that GetDataOfLength is always reading one byte at a time, it wouldn't change a lot.

As a more serious flay, is it possible for AddData to be invoked such that CDCDataStream could overflow? There is currently no handling around data overflow in CDCDataStream itself, so it is unclear what in the system design ensures that overflow will not occur. What is the justification for not having bounds checking in CDCDataStream itself? Is it possible that CDCDataStream::AddData overflows in some circumstances, and is just not being detected?

Loading process robustness

As far as I can tell, the loading process is a blind write from the host to the device (e.g. https://github.com/32blit/32blit-tools/blob/a520a742450c8da97f88f6c0ce74ac0038093e02/src/ttblit/core/blitserial.py#L114-L126 ). The device side is dependent on receiving all the bytes, or the loader will remain stuck waiting for bytes (see:

32blit-sdk/firmware/firmware.cpp

Lines 961 to 963 in 3437f5b

	// end of file
	uWriteLen = uByteOffset+1;
	bEOS = true;

).

What provides the guarantee that all bytes written from the host will be received by the device and written to disk? When loading larger .blit files I've seen the device appear to be "stuck", which would potentially match a situation where bytes were lost. Why was this approach taken over approaches that could ensure robustness (e.g. packetized transmission w/ ACK/retry protocol).

Overall data transfer validation

There does not appear to be a post-transmission data check to ensure post-write data validity to the flash or SD card, e.g. CRC / SHA-xxx hash. The STM32H7 appears to have a HASH processor peripheral capable of doing accelerated SHA-1 / SHA-256 calculations. What is the reason for omitting some form of post-write validity check during loading?

[Daft-Freak]

This is a bit of a nit, but why is CDCDataStream not designed as a circular buffer? The memmove at the beginning would not be required, and the resulting portion of AddData would be either 1 or 2 memcpy's. Given that GetDataOfLength is always reading one byte at a time, it wouldn't change a lot.

It originally didn't buffer at all there, I added that to fix an issue with a word being split across two FIFO entries. Went with the smallest change I guess.

As a more serious flay, is it possible for AddData to be invoked such that CDCDataStream could overflow? There is currently no handling around data overflow in CDCDataStream itself, so it is unclear what in the system design ensures that overflow will not occur. What is the justification for not having bounds checking in CDCDataStream itself? Is it possible that CDCDataStream::AddData overflows in some circumstances, and is just not being detected?

It shouldn't be (not that anything verifies this). The most that can be added in one call is 64 bytes, and the most that can be left in the buffer should be <= 3 bytes as the only time a handler returns without finishing/erroring and with data left in the CDCDataStream is if it wants to read a word and there isn't one. (Also starting a new command/handler discards any data left in the stream).

TBH when I wrote the code that relies on reading words I thought the streaming code would handle that across FIFO entries. It didn't, so I added the buffering in CDCDataStream. A couple more fixes just went in because it still wasn't quite right.

For the rest I don't think there's any reason other than nobody writing the code. All the USB serial stuff was a contribution from a beta tester... (Says the beta tester with excessive contributions). (Though I guess you could describe the USB level as "packetized transmission w/ ACK/retry protocol"?)