Restart RX Session if out-of-order multiframe transfer received #20
Conversation
lydia-at-amazon
requested review from
SchoberMJ,
emrainey,
pavel-kirienko and
thirtytwobits
| UdpardRxSubscription* subscription = nullptr; | ||
| std::int8_t result = | ||
| ins_rx.rxAccept(ti->tx_deadline_usec, ti->frame, 0, ti->specifier, transfer, &subscription); | ||
| REQUIRE(((-UDPARD_ERROR_OUT_OF_ORDER == ins_rx.rxAccept(ti->tx_deadline_usec, |
There was a problem hiding this comment.
Was this covering the case where the second frame was received as duplicate with the same index?
There was a problem hiding this comment.
Hey Erik, glad you asked! That's what I thought it was doing, but looking more closely, it was covering the case where we call rxAccept on a transport index of 0 and then call rxAccept on a transport index of 1. So, yes, the frame received on the second rxAccept would have the same index as on the first rxAccept. I thought the sessions for different transport indexes would be handled separately, but it seems like this is not currently the case. So, if we try to call rxAccept on the same frame for different transports, we are currently getting an OUT_OF_ORDER error because the frame indexes are the same.
This made me realize that there is another bug. The multiframe code wasn't checking that the rx session transport index matched the index passed into rxSessionUpdate. It should have a check like this: https://github.com/OpenCyphal-Garage/libudpard/blob/main/libudpard/udpard.c#L1001
There was a problem hiding this comment.
Just to confirm before I make the change, each unique transport index should have its own RX session, correct? So, in rxAcceptFrame, we should return immediately if rxs->redundant_transport_index != redundant_transport_index: https://github.com/OpenCyphal-Garage/libudpard/blob/main/libudpard/udpard.c#L936
There was a problem hiding this comment.
I'll make this change in another CR since we're anxious to get the original fix merged. I have already made the change locally so it will be a quick turnaround.
During on-target testing, it was found that if a frame is received out of order during a multiframe transfer, then libudpard cannot receive any subsequent transfers for any port IDs because it reports an out of memory error. This is because the memory allocated for the out-of-order multiframe transfer is never freed. Since out-of-order multiframe transfers are not currently supported, the best course of action is drop the entire payload and require the user to resend it.