Skip to content

Commit 03fc5fe

Browse files
Fix for the "Improve RPC Client Response RX pipeline" issue # 228 (OpenCyphal#231)
added one more case when it's allowed to "restart" transfer reassembly with different transport interface see OpenCyphal#228 --------- Co-authored-by: Pavel Kirienko <pavel.kirienko@gmail.com>
1 parent 551af7f commit 03fc5fe

2 files changed

Lines changed: 16 additions & 5 deletions

File tree

libcanard/canard.c

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -808,13 +808,20 @@ CANARD_PRIVATE int8_t rxSessionAcceptFrame(CanardInstance* const ins,
808808

809809
/// Evaluates the state of the RX session with respect to time and the new frame, and restarts it if necessary.
810810
/// The next step is to accept the frame if the transfer-ID, toggle but, and transport index match; reject otherwise.
811-
/// The logic of this function is simple: it restarts the reassembler if the start-of-transfer flag is set and
812-
/// any two of the three conditions are met:
811+
/// The logic of this function is simple: in the most cases (see below exception) it restarts the reassembler
812+
/// if the start-of-transfer flag is set and any two of the three conditions are met:
813813
///
814814
/// - The frame has arrived over the same interface as the previous transfer.
815815
/// - New transfer-ID is detected.
816816
/// - The transfer-ID timeout has expired.
817817
///
818+
/// The only exception is when the transfer-ID timeout has expired and the new frame has the same transfer-ID as it
819+
/// was expected BUT not on the same transport as before. In this case, the reassembler is "restarted" only
820+
/// if the total payload size is zero (meaning that the reassembler has not been started yet), and so could be more
821+
/// "relaxed" and not so sticky to the transport index. This case was discovered during libcyphal development when
822+
/// multiple redundant transports were used in context of multiple concurrent RPC clients for the same service id.
823+
/// For more information see https://github.com/OpenCyphal/libcanard/issues/228
824+
///
818825
/// Notice that mere TID-timeout is not enough to restart to prevent the interface index oscillation;
819826
/// while this is not visible at the application layer, it may delay the transfer arrival.
820827
CANARD_PRIVATE void rxSessionSynchronize(CanardInternalRxSession* const rxs,
@@ -831,14 +838,18 @@ CANARD_PRIVATE void rxSessionSynchronize(CanardInternalRxSession* const rxs,
831838
// Examples: rxComputeTransferIDDifference(2, 3)==31
832839
// rxComputeTransferIDDifference(2, 2)==0
833840
// rxComputeTransferIDDifference(2, 1)==1
834-
const bool tid_new = rxComputeTransferIDDifference(rxs->transfer_id, frame->transfer_id) > 1;
841+
const bool tid_match = rxs->transfer_id == frame->transfer_id;
842+
const bool tid_new = rxComputeTransferIDDifference(rxs->transfer_id, frame->transfer_id) > 1;
835843
// The transfer ID timeout is measured relative to the timestamp of the last start-of-transfer frame.
836844
const bool tid_timeout = (frame->timestamp_usec > rxs->transfer_timestamp_usec) &&
837845
((frame->timestamp_usec - rxs->transfer_timestamp_usec) > transfer_id_timeout_usec);
846+
// The total payload size is zero when a new transfer reassembling has not been started yet, hence the idle.
847+
const bool idle = 0U == rxs->total_payload_size;
838848

839849
const bool restartable = (same_transport && tid_new) || //
840850
(same_transport && tid_timeout) || //
841-
(tid_new && tid_timeout);
851+
(tid_timeout && tid_new) || //
852+
(tid_timeout && tid_match && idle);
842853
// Restarting the transfer reassembly only makes sense if the new frame is a start of transfer.
843854
// Otherwise, the new transfer would be impossible to reassemble anyway since the first frame is lost.
844855
if (frame->start_of_transfer && restartable)

libcanard/canard.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -94,7 +94,7 @@ extern "C" {
9494
/// Semantic version of this library (not the Cyphal specification).
9595
/// API will be backward compatible within the same major version.
9696
#define CANARD_VERSION_MAJOR 3
97-
#define CANARD_VERSION_MINOR 2
97+
#define CANARD_VERSION_MINOR 3
9898

9999
/// The version number of the Cyphal specification implemented by this library.
100100
#define CANARD_CYPHAL_SPECIFICATION_VERSION_MAJOR 1

0 commit comments

Comments
 (0)