Add support for multi-frame messages
Closed this issue · 17 comments
hi,
how is it going with this? any active developement?
Haven't had a chance to get started yet. Still planning the implementation.
Do you have any input and/or suggestions? Perhaps some use cases?
Hi @zacnelson,
I'm about to use your iso-tp implementation and I need multi frame support.
I have to play around with the single frame implementation you did so far to get familiar with your code but it looks really good!
use case:
One of our use cases is the transfer of firmware data to a CAN device.
My assumption is that we do not have to comply with the whole iso-tp standard. From my perspective it looks OK to hardcode values:
- Block Size (BS)
- Seperation Time (ST)
do you think the TP-layer should re-assemble the whole 4095byte payload? In case of an firmware update, this would not be nearly enough to handle the whole process anyway.
input/suggestions:
I implemented a very primitive sort of single frame handling in my code just to demonstrate the basic functionality of operation and I used unions with structs and bitfields to describe the 8 data bytes in a CAN frame:
I really liked the code resulting from it.
what do you think? might it worth to think about using this representation?
maybe we can get some inspiration for multiframe support from this repo:
https://github.com/altelch/iso-tp
Nice find with the Arduino iso-tp repo. The only issue I see with that implementation is the blocking while loop while they wait for the subsequent multi-frames to come in. See here: https://github.com/altelch/iso-tp/blob/master/iso-tp.cpp#L407
We'll need to have an implementation that doesn't block the rest of the CAN parsing going on - as used in https://github.com/openxc/vi-firmware
We've implemented it with a fairly simple state machine in C# and C++... I've been looking for free time to write up a C version for a while but haven't had time yet. This week might be good though. ;)
Nice! I think it could use a few unit tests to verify the behavior. If you open a pull request from that branch we can iterate on it there.
Absolutely. I started messing with the tests yesterday. I'll push the latest and create a pull request.
Awesome! Looks pretty good. Why the 256byte max frame size?
@notsig11 I don't think we have enough memory to allocate the full 4k for every single diagnostic response for the OpenXC VI. Especially if you queue up a bunch at once. Definitely something to address next.
@zacnelson Right but you are only allocating payload_length bytes not the full 4k unless I missed something. I don't think it's necessary.
If that malloc fails the FC frame will never get sent and the sender can try again after the FC timer expires and we can try again to allocate payload_length bytes for the receive_buffer.
For combining all the received frames, you're correct - it only allocates the payload length. But then when we return the completed IsoTpMessage, we have a struct defined with a payload allocation of 256 bytes. See here: https://github.com/openxc/isotp-c/blob/multi-frame/src/isotp/isotp_types.h#L46
Then in the library above this, uds-c, we also have a 256 byte payload struct for the DiagnosticResponse.
Ah! I figured I was missing something there. :)
Pull request has been merged for allow multi-frame responses. Right now limited to 256 byte response. Closing this issue for now. @hackrid take a look when you get a chance.
The current version supports only single frame ISO-TP messages. This is fine for OBD-II diagnostic messages, for example, but this library needs some additional work before it can support sending larger messages.
Is your README out of date?
Partially. multi-frame read is in there. We ran into a memory issue on the VIs with the larger messages so multi-frame write is not fully implemented yet.