Simple CAN chunking

[robbederks]

A more simple implementation of #1004. This gets rid of the per-chunk counter, and instead adds a 32 bit magic in the header of the transaction, which should also scale to larger chunk sizes.

Getting rid of the counter makes the packing / unpacking also a lot simpler.

[robbederks]

Another argument for not needing a counter is the fact that there's a bug in boardd where it locks up if there's a mismatch between the counter it received and the one it expected, which is not showing up in the MTBF reports, and no instances are in the logs of devices running a non-dirty comma remote.

[gregjhogan]

I feel like it is a good idea to make sure this is robust to things like the client attempting to send X bytes to the panda, libusb telling you it didn't send everything, and then the client incorrectly sending the rest while recovering. I think there are a set of bugs like this that are pretty easy to write that ideally would be detected in the panda firmware to ensure we don't throw garbage out on the can bus. I think the way the USB receive buffer works in the panda there was also concern packets could get dropped without making things more complicated?

I feel like there must be some standard way people make sure they received a complete set of USB packets of variable length. I guess it is tricky right now because we send can messages before we get all the data.

[adeebshihadeh]

I feel like it is a good idea to make sure this is robust to things like the client attempting to send X bytes to the panda, libusb telling you it didn't send everything, and then the client incorrectly sending the rest while recovering. I think there are a set of bugs like this that are pretty easy to write that ideally would be detected in the panda firmware to ensure we don't throw garbage out on the can bus. I think the way the USB receive buffer works in the panda there was also concern packets could get dropped without making things more complicated?

I think these are some good things to address, but is the concern with this specific change just the removal of the counter? Practically speaking, our safety modes are so strict that total junk will easily get rejected.

[gregjhogan]

I just think it is important we detect when the payload has an issue, the counter was just one way to address it. We also use this library for flashing ECUs and I have run into subtle issues like this after erasing and before flashing completes. Before this change we actively detected and reported when the integrity of the payload was compromised. I really would like to not lose that.

[gregjhogan]

The simplest alternative I can come up with is putting a total packet count in the first packet and then reporting an error if the correct number of packets is not received. We potentially throw garbage out on the bus but at least you can watch the debug console and know something happened. Can you at least do that?

[adeebshihadeh]

The simplest alternative I can come up with is putting a total packet count in the first packet and then reporting an error if the correct number of packets is not received. We potentially throw garbage out on the bus but at least you can watch the debug console and know something happened. Can you at least do that?

But you don't necessarily know the size at the beginning. We can either add the counter back or maintain an intermediate buffer until a whole transaction is done. A count at the very end could work too, but only for knowing afterwards whether there was an issue. Adding the counter back seems like the best choice here.

@robbederks any reason you removed the counter? I saw in another PR you mentioned that it doens't happen in openpilot usage, but that's not really a good reason to remove it.

[robbederks]

Ok, here's what I think I remember about this:

For USB, I'm not sure what issue the counter fixed? Since the USB bulk transfers are handled by the kernel, they never get cut off in the middle of chunks when you stop the application on the host, and are afaik guaranteed by the protocol to be ordered and without dropped chunks (which is confirmed by the fact that we never saw counter issues trigger in the field, since that would crash boardd). I don't think using libusb in python would behave any differently?

For SPI however, there is no guarantee that the whole bulk transfer completes since that's completely handled in the application, so the counters can get out of sync between the host and the STM. Also, the reset of the counter for USB is handled by checking if the received chunk is <64 bytes or if you're at the max bulk transaction size which isn't possible for SPI. I guess both these issues could be fixed by adding a control endpoint to reset the counter which is called by the application before every bulk transfer?

The counter also requires that the chunks are always a fixed length (64 bytes in case of USB). The goal of these PRs (first #1004 and now this) is to get rid of that requirement to allow for longer chunks, since that would have way less overhead over SPI (all the separate transactions for each chunk are pretty slow compared to one big one). Since we do flow control on a transaction-basis and not on a chunk-basis that would be a big benefit.

Another way to handle this would be to just add the counter every 64 bytes anyways, regardless of the chunk boundaries, I guess that's also a possible "fix". Without the reset control endpoint that would still result in lost data, so it still doesn't fix much.

Maybe the only way to solve this for SPI is to add the control endpoint to tell the STM to reset the transaction buffers for both RX and TX? The order of the chunks is guaranteed for both SPI and USB without the counter, so in either case the counter doesn't fix anything, unless I'm missing something?

[robbederks]

Also the code to add / remove the counters and stitch the data back together is pretty messy and error-prone, especially in C.

[gregjhogan]

The only concern was silently losing packets. This can be caused by the client (e.g. libusb tells you it failed and only sent the first X bytes, which you need to ensure you handle correctly) and on the firmware side (e.g. packets can keep coming in even after we tell the USB core to stop clearing NAK causing buffer overflow and I don't understand what the upper bound is on how many packets this can be. The documentation for the USB FIFO is very confusing and I can't remember if we ever saw this hidden buffer overflow).

It seemed reasonable to implement things in such a way that it is easy to detect and report when something like this occurs. If this means we can prevent processing garbage that is returned and prevent throwing garbage out on the bus it seemed appropriate to do so. Perhaps a compromise is we can detect and report the errors after the fact.

The panda firmware bugs I have found flashing ECUs makes me believe looking at what errors happen in openpilot is not a good proxy for if something like this could be an issue.

[gregjhogan]

Doesn't comms_can_write always check if the first byte is the magic number? This means if it ends up in the CAN data at the wrong position we throw garbage out on the bus. We need to also support a client getting disconnected mid stream and starting over with a new stream. One way to do this would be using a control endpoint to sync when a client connects.