Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for the "Improve RPC Client Response RX pipeline" issue # 228 #231

Merged
merged 2 commits into from
Sep 24, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 15 additions & 4 deletions libcanard/canard.c
Original file line number Diff line number Diff line change
Expand Up @@ -808,13 +808,20 @@ CANARD_PRIVATE int8_t rxSessionAcceptFrame(CanardInstance* const ins,

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

const bool restartable = (same_transport && tid_new) || //
(same_transport && tid_timeout) || //
(tid_new && tid_timeout);
(tid_timeout && tid_new) || //
(tid_timeout && tid_match && idle);
// Restarting the transfer reassembly only makes sense if the new frame is a start of transfer.
// Otherwise, the new transfer would be impossible to reassemble anyway since the first frame is lost.
if (frame->start_of_transfer && restartable)
Expand Down
2 changes: 1 addition & 1 deletion libcanard/canard.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,7 @@ extern "C" {
/// Semantic version of this library (not the Cyphal specification).
/// API will be backward compatible within the same major version.
#define CANARD_VERSION_MAJOR 3
#define CANARD_VERSION_MINOR 2
#define CANARD_VERSION_MINOR 3

/// The version number of the Cyphal specification implemented by this library.
#define CANARD_CYPHAL_SPECIFICATION_VERSION_MAJOR 1
Expand Down
Loading