From 73d0a9cae94307038344b0d3eac2fd6dac44e139 Mon Sep 17 00:00:00 2001 From: Pavel Kirienko Date: Sun, 30 Apr 2023 02:32:02 +0300 Subject: [PATCH] Refactor the transfer reassembly state machine to enhance its maintainability and robustness (#215) This is a step towards improving the transfer reassembler. This changeset also renames `redundant_transport_index` as `redundant_iface_index` for consistency with the other implementations; this change is not visible at the API level. --- README.md | 8 ++- libcanard/canard.c | 121 ++++++++++++++++++++--------------- libcanard/canard.h | 12 ++-- tests/exposed.hpp | 18 +++--- tests/helpers.hpp | 9 +-- tests/test_private_rx.cpp | 97 +++++++++++++++++----------- tests/test_public_rx.cpp | 131 +++++++++++++++++++++++++++++++++++--- 7 files changed, 273 insertions(+), 123 deletions(-) diff --git a/README.md b/README.md index aa3aeb6..4ebc819 100644 --- a/README.md +++ b/README.md @@ -27,7 +27,7 @@ If you want to contribute, please read [`CONTRIBUTING.md`](/CONTRIBUTING.md). - Detailed time complexity and memory requirement models for the benefit of real-time high-integrity applications. - Purely reactive API without the need for background servicing. - Support for the Classic CAN and CAN FD. -- Support for redundant transports. +- Support for redundant network interfaces. - Compatibility with 8/16/32/64-bit platforms. - Compatibility with extremely resource-constrained baremetal environments starting from 32K ROM and 32K RAM. - Implemented in ≈1000 lines of code. @@ -124,7 +124,7 @@ Use [Nunavut](https://github.com/OpenCyphal/nunavut) to automatically generate The CAN frames generated from the message transfer are now stored in the `queue`. We need to pick them out one by one and have them transmitted. Normally, the following fragment should be invoked periodically to unload the CAN frames from the -prioritized transmission queue (or several, if redundant interfaces are used) into the CAN driver: +prioritized transmission queue (or several, if redundant network interfaces are used) into the CAN driver: ```c for (const CanardTxQueueItem* ti = NULL; (ti = canardTxPeek(&queue)) != NULL;) // Peek at the top of the queue. @@ -237,6 +237,10 @@ If you find the examples to be unclear or incorrect, please, open a ticket. - Simplify the transfer reassembly state machine to address [#212](https://github.com/OpenCyphal/libcanard/issues/212). See also . +#### v3.1.1 + +- Refactor the transfer reassembly state machine to enhance its maintainability and robustness. + ### v3.0 - Update branding as [UAVCAN v1 is renamed to Cyphal](https://forum.opencyphal.org/t/uavcan-v1-is-now-cyphal/1622). diff --git a/libcanard/canard.c b/libcanard/canard.c index 9b2353b..d4577bf 100644 --- a/libcanard/canard.c +++ b/libcanard/canard.c @@ -558,7 +558,7 @@ typedef struct CanardInternalRxSession uint8_t* payload; ///< Dynamically allocated and handed off to the application when done. TransferCRC calculated_crc; ///< Updated with the received payload in real time. CanardTransferID transfer_id; - uint8_t redundant_transport_index; ///< Arbitrary value in [0, 255]. + uint8_t redundant_iface_index; ///< Arbitrary value in [0, 255]. bool toggle; } CanardInternalRxSession; @@ -802,6 +802,53 @@ CANARD_PRIVATE int8_t rxSessionAcceptFrame(CanardInstance* const ins, return out; } +/// 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 frame has arrived over the same interface as the previous transfer. +/// - New transfer-ID is detected. +/// - The transfer-ID timeout has expired. +/// +/// 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, + const RxFrameModel* const frame, + const uint8_t redundant_iface_index, + const CanardMicrosecond transfer_id_timeout_usec) +{ + CANARD_ASSERT(rxs != NULL); + CANARD_ASSERT(frame != NULL); + CANARD_ASSERT(rxs->transfer_id <= CANARD_TRANSFER_ID_MAX); + CANARD_ASSERT(frame->transfer_id <= CANARD_TRANSFER_ID_MAX); + + const bool same_transport = rxs->redundant_iface_index == redundant_iface_index; + // 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; + // 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); + + const bool restartable = (same_transport && tid_new) || // + (same_transport && tid_timeout) || // + (tid_new && tid_timeout); + // 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) + { + CANARD_ASSERT(frame->start_of_transfer); + rxs->total_payload_size = 0U; + rxs->payload_size = 0U; // The buffer is not released because we still need it. + rxs->calculated_crc = CRC_INITIAL; + rxs->transfer_id = frame->transfer_id; + rxs->toggle = INITIAL_TOGGLE_STATE; + rxs->redundant_iface_index = redundant_iface_index; + } +} + /// RX session state machine update is the most intricate part of any Cyphal transport implementation. /// The state model used here is derived from the reference pseudocode given in the original UAVCAN v0 specification. /// The Cyphal/CAN v1 specification, which this library is an implementation of, does not provide any reference @@ -812,7 +859,7 @@ CANARD_PRIVATE int8_t rxSessionAcceptFrame(CanardInstance* const ins, CANARD_PRIVATE int8_t rxSessionUpdate(CanardInstance* const ins, CanardInternalRxSession* const rxs, const RxFrameModel* const frame, - const uint8_t redundant_transport_index, + const uint8_t redundant_iface_index, const CanardMicrosecond transfer_id_timeout_usec, const size_t extent, CanardRxTransfer* const out_transfer) @@ -823,37 +870,7 @@ CANARD_PRIVATE int8_t rxSessionUpdate(CanardInstance* const ins, CANARD_ASSERT(out_transfer != NULL); CANARD_ASSERT(rxs->transfer_id <= CANARD_TRANSFER_ID_MAX); CANARD_ASSERT(frame->transfer_id <= CANARD_TRANSFER_ID_MAX); - - // The transfer ID timeout is measured relative to the timestamp of the last start-of-transfer frame. - // Triggering a TID timeout when the TID is the same is undesirable because it may cause the reassembler to - // switch to another interface if the start-of-transfer frame of the current transfer is duplicated - // on the other interface more than (transfer-ID timeout) units of time after the start of - // the transfer while the reassembly of this transfer is still in progress. - // While this behavior is not visible to the application because the transfer will still be reassembled, - // it may delay the delivery of the transfer. - const bool tid_timed_out = (frame->timestamp_usec > rxs->transfer_timestamp_usec) && - (frame->transfer_id != rxs->transfer_id) && - ((frame->timestamp_usec - rxs->transfer_timestamp_usec) > transfer_id_timeout_usec); - // Examples: rxComputeTransferIDDifference(2, 3)==31 - // rxComputeTransferIDDifference(2, 2)==0 - // rxComputeTransferIDDifference(2, 1)==1 - const bool not_previous_tid = rxComputeTransferIDDifference(rxs->transfer_id, frame->transfer_id) > 1; - // 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. - const bool need_restart = - frame->start_of_transfer && - (tid_timed_out || ((rxs->redundant_transport_index == redundant_transport_index) && not_previous_tid)); - if (need_restart) - { - CANARD_ASSERT(frame->start_of_transfer); - rxs->total_payload_size = 0U; - rxs->payload_size = 0U; - rxs->calculated_crc = CRC_INITIAL; - rxs->transfer_id = frame->transfer_id; - rxs->toggle = INITIAL_TOGGLE_STATE; - rxs->redundant_transport_index = redundant_transport_index; - } - + rxSessionSynchronize(rxs, frame, redundant_iface_index, transfer_id_timeout_usec); int8_t out = 0; // The purpose of the correct_start check is to reduce the possibility of accepting a malformed multi-frame // transfer in the event of a CRC collision. The scenario where this failure mode would manifest is as follows: @@ -864,13 +881,13 @@ CANARD_PRIVATE int8_t rxSessionUpdate(CanardInstance* const ins, // 4. The last frame of the multi-frame transfer is erroneously accepted even though it is malformed. // The correct_start check eliminates this failure mode by ensuring that the first frame is observed. // See https://github.com/OpenCyphal/libcanard/issues/189. - const bool correct_transport = (rxs->redundant_transport_index == redundant_transport_index); - const bool correct_toggle = (frame->toggle == rxs->toggle); - const bool correct_tid = (frame->transfer_id == rxs->transfer_id); - const bool correct_start = frame->start_of_transfer // - ? (0 == rxs->total_payload_size) - : (rxs->total_payload_size > 0); - if (correct_transport && correct_toggle && correct_tid && correct_start) + const bool correct_iface = (rxs->redundant_iface_index == redundant_iface_index); + const bool correct_toggle = (frame->toggle == rxs->toggle); + const bool correct_tid = (frame->transfer_id == rxs->transfer_id); + const bool correct_start = frame->start_of_transfer // + ? (0 == rxs->total_payload_size) + : (rxs->total_payload_size > 0); + if (correct_iface && correct_toggle && correct_tid && correct_start) { out = rxSessionAcceptFrame(ins, rxs, frame, extent, out_transfer); } @@ -880,7 +897,7 @@ CANARD_PRIVATE int8_t rxSessionUpdate(CanardInstance* const ins, CANARD_PRIVATE int8_t rxAcceptFrame(CanardInstance* const ins, CanardRxSubscription* const subscription, const RxFrameModel* const frame, - const uint8_t redundant_transport_index, + const uint8_t redundant_iface_index, CanardRxTransfer* const out_transfer) { CANARD_ASSERT(ins != NULL); @@ -904,14 +921,14 @@ CANARD_PRIVATE int8_t rxAcceptFrame(CanardInstance* const ins, subscription->sessions[frame->source_node_id] = rxs; if (rxs != NULL) { - rxs->transfer_timestamp_usec = frame->timestamp_usec; - rxs->total_payload_size = 0U; - rxs->payload_size = 0U; - rxs->payload = NULL; - rxs->calculated_crc = CRC_INITIAL; - rxs->transfer_id = frame->transfer_id; - rxs->redundant_transport_index = redundant_transport_index; - rxs->toggle = INITIAL_TOGGLE_STATE; + rxs->transfer_timestamp_usec = frame->timestamp_usec; + rxs->total_payload_size = 0U; + rxs->payload_size = 0U; + rxs->payload = NULL; + rxs->calculated_crc = CRC_INITIAL; + rxs->transfer_id = frame->transfer_id; + rxs->redundant_iface_index = redundant_iface_index; + rxs->toggle = INITIAL_TOGGLE_STATE; } else { @@ -925,7 +942,7 @@ CANARD_PRIVATE int8_t rxAcceptFrame(CanardInstance* const ins, out = rxSessionUpdate(ins, subscription->sessions[frame->source_node_id], frame, - redundant_transport_index, + redundant_iface_index, subscription->transfer_id_timeout_usec, subscription->extent, out_transfer); @@ -1098,7 +1115,7 @@ CanardTxQueueItem* canardTxPop(CanardTxQueue* const que, const CanardTxQueueItem int8_t canardRxAccept(CanardInstance* const ins, const CanardMicrosecond timestamp_usec, const CanardFrame* const frame, - const uint8_t redundant_transport_index, + const uint8_t redundant_iface_index, CanardRxTransfer* const out_transfer, CanardRxSubscription** const out_subscription) { @@ -1126,7 +1143,7 @@ int8_t canardRxAccept(CanardInstance* const ins, if (sub != NULL) { CANARD_ASSERT(sub->port_id == model.port_id); - out = rxAcceptFrame(ins, sub, &model, redundant_transport_index, out_transfer); + out = rxAcceptFrame(ins, sub, &model, redundant_iface_index, out_transfer); } else { diff --git a/libcanard/canard.h b/libcanard/canard.h index 7a117d0..0f1b5a7 100644 --- a/libcanard/canard.h +++ b/libcanard/canard.h @@ -65,7 +65,7 @@ /// subscription are truncated following the Implicit Truncation Rule (ITR) defined by the Cyphal Specification -- /// the rule is implemented to facilitate backward-compatible DSDL data type extensibility. /// -/// The library supports a practically unlimited number of redundant transports. +/// The library supports a practically unlimited number of redundant interfaces. /// /// The library is not thread-safe: if used in a concurrent environment, it is the responsibility of the application /// to provide adequate synchronization. @@ -511,9 +511,9 @@ CanardTxQueueItem* canardTxPop(CanardTxQueue* const que, const CanardTxQueueItem /// /// The MTU of the accepted frame can be arbitrary; that is, any MTU is accepted. The DLC validity is irrelevant. /// -/// Any value of redundant_transport_index is accepted; that is, up to 256 redundant transports are supported. -/// The index of the transport from which the transfer is accepted is always the same as redundant_transport_index -/// of the current invocation, so the application can always determine which transport has delivered the transfer. +/// Any value of redundant_iface_index is accepted; that is, up to 256 redundant interfaces are supported. +/// The index of the interface from which the transfer is accepted is always the same as redundant_iface_index +/// of the current invocation, so the application can always determine which interface has delivered the transfer. /// /// Upon return, the out_subscription pointer will point to the instance of CanardRxSubscription that accepted this /// frame; if no matching subscription exists (i.e., frame discarded), the pointer will be NULL. @@ -593,7 +593,7 @@ CanardTxQueueItem* canardTxPop(CanardTxQueue* const que, const CanardTxQueueItem int8_t canardRxAccept(CanardInstance* const ins, const CanardMicrosecond timestamp_usec, const CanardFrame* const frame, - const uint8_t redundant_transport_index, + const uint8_t redundant_iface_index, CanardRxTransfer* const out_transfer, CanardRxSubscription** const out_subscription); @@ -613,7 +613,7 @@ int8_t canardRxAccept(CanardInstance* const ins, /// whether its payload is truncated. /// /// The default transfer-ID timeout value is defined as CANARD_DEFAULT_TRANSFER_ID_TIMEOUT_USEC; use it if not sure. -/// The redundant transport fail-over timeout (if redundant transports are used) is the same as the transfer-ID timeout. +/// The redundant interface fail-over timeout (if redundant interfaces are used) is the same as the transfer-ID timeout. /// It may be reduced in a future release of the library, but it will not affect the backward compatibility. /// /// The return value is 1 if a new subscription has been created as requested. diff --git a/tests/exposed.hpp b/tests/exposed.hpp index 84a442d..f06006f 100644 --- a/tests/exposed.hpp +++ b/tests/exposed.hpp @@ -45,14 +45,14 @@ struct TxItem final : CanardTxQueueItem struct RxSession { - CanardMicrosecond transfer_timestamp_usec = std::numeric_limits::max(); - std::size_t total_payload_size = 0U; - std::size_t payload_size = 0U; - std::uint8_t* payload = nullptr; - TransferCRC calculated_crc = 0U; - CanardTransferID transfer_id = std::numeric_limits::max(); - std::uint8_t redundant_transport_index = std::numeric_limits::max(); - bool toggle = false; + CanardMicrosecond transfer_timestamp_usec = std::numeric_limits::max(); + std::size_t total_payload_size = 0U; + std::size_t payload_size = 0U; + std::uint8_t* payload = nullptr; + TransferCRC calculated_crc = 0U; + CanardTransferID transfer_id = std::numeric_limits::max(); + std::uint8_t redundant_iface_index = std::numeric_limits::max(); + bool toggle = false; }; struct RxFrameModel @@ -112,7 +112,7 @@ void rxSessionRestart(CanardInstance* const ins, RxSession* const rxs); auto rxSessionUpdate(CanardInstance* const ins, RxSession* const rxs, const RxFrameModel* const frame, - const std::uint8_t redundant_transport_index, + const std::uint8_t redundant_iface_index, const CanardMicrosecond transfer_id_timeout_usec, const std::size_t extent, CanardRxTransfer* const out_transfer) -> std::int8_t; diff --git a/tests/helpers.hpp b/tests/helpers.hpp index 0475430..618f3c8 100644 --- a/tests/helpers.hpp +++ b/tests/helpers.hpp @@ -179,16 +179,11 @@ class Instance [[nodiscard]] auto rxAccept(const CanardMicrosecond timestamp_usec, const CanardFrame& frame, - const uint8_t redundant_transport_index, + const uint8_t redundant_iface_index, CanardRxTransfer& out_transfer, CanardRxSubscription** const out_subscription) { - return canardRxAccept(&canard_, - timestamp_usec, - &frame, - redundant_transport_index, - &out_transfer, - out_subscription); + return canardRxAccept(&canard_, timestamp_usec, &frame, redundant_iface_index, &out_transfer, out_subscription); } [[nodiscard]] auto rxSubscribe(const CanardTransferKind transfer_kind, diff --git a/tests/test_private_rx.cpp b/tests/test_private_rx.cpp index acd9711..5ec0ff1 100644 --- a/tests/test_private_rx.cpp +++ b/tests/test_private_rx.cpp @@ -306,22 +306,21 @@ TEST_CASE("rxSessionUpdate") frame.payload = reinterpret_cast("\x01\x01\x01"); RxSession rxs; - rxs.transfer_id = 31; - rxs.redundant_transport_index = 1; + rxs.transfer_id = 31; + rxs.redundant_iface_index = 1; CanardRxTransfer transfer{}; - const auto update = [&](const std::uint8_t redundant_transport_index, - const std::uint64_t tid_timeout_usec, - const std::size_t extent) { - return rxSessionUpdate(&ins.getInstance(), - &rxs, - &frame, - redundant_transport_index, - tid_timeout_usec, - extent, - &transfer); - }; + const auto update = + [&](const std::uint8_t redundant_iface_index, const std::uint64_t tid_timeout_usec, const std::size_t extent) { + return rxSessionUpdate(&ins.getInstance(), + &rxs, + &frame, + redundant_iface_index, + tid_timeout_usec, + extent, + &transfer); + }; const auto crc = [](const char* const string) { return crcAdd(0xFFFF, std::strlen(string), string); }; @@ -333,7 +332,7 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.calculated_crc == 0xFFFF); REQUIRE(rxs.transfer_id == 12U); // Incremented. REQUIRE(rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 1); + REQUIRE(rxs.redundant_iface_index == 1); REQUIRE(transfer.timestamp_usec == 10'000'000); REQUIRE(transfer.metadata.priority == CanardPrioritySlow); REQUIRE(transfer.metadata.transfer_kind == CanardTransferKindMessage); @@ -357,7 +356,7 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.calculated_crc == 0xFFFF); REQUIRE(rxs.transfer_id == 12U); // Incremented. REQUIRE(rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 1); + REQUIRE(rxs.redundant_iface_index == 1); // Correct transport. frame.timestamp_usec = 10'000'050; @@ -369,7 +368,7 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.calculated_crc == 0xFFFF); REQUIRE(rxs.transfer_id == 13U); REQUIRE(rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 1); + REQUIRE(rxs.redundant_iface_index == 1); REQUIRE(transfer.timestamp_usec == 10'000'050); REQUIRE(transfer.metadata.priority == CanardPrioritySlow); REQUIRE(transfer.metadata.transfer_kind == CanardTransferKindMessage); @@ -393,26 +392,50 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.calculated_crc == 0xFFFF); REQUIRE(rxs.transfer_id == 13U); REQUIRE(rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 1); + REQUIRE(rxs.redundant_iface_index == 1); + + // Restart due to TID timeout, same iface. + frame.timestamp_usec = 15'000'000; + frame.transfer_id = 12; + frame.payload = reinterpret_cast("\x05\x05\x05"); + REQUIRE(1 == update(1, 1'000'000, 16)); + REQUIRE(rxs.transfer_timestamp_usec == 15'000'000); + REQUIRE(rxs.payload_size == 0); + REQUIRE(rxs.payload == nullptr); + REQUIRE(rxs.calculated_crc == 0xFFFF); + REQUIRE(rxs.transfer_id == 13U); + REQUIRE(rxs.toggle); + REQUIRE(rxs.redundant_iface_index == 1); + REQUIRE(transfer.timestamp_usec == 15'000'000); + REQUIRE(transfer.metadata.priority == CanardPrioritySlow); + REQUIRE(transfer.metadata.transfer_kind == CanardTransferKindMessage); + REQUIRE(transfer.metadata.port_id == 2'222); + REQUIRE(transfer.metadata.remote_node_id == 55); + REQUIRE(transfer.metadata.transfer_id == 12); + REQUIRE(transfer.payload_size == 3); + REQUIRE(0 == std::memcmp(transfer.payload, "\x05\x05\x05", 3)); + REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); + REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16); + ins.getAllocator().deallocate(transfer.payload); // Restart due to TID timeout, switch iface. frame.timestamp_usec = 20'000'000; - frame.transfer_id = 12; + frame.transfer_id = 11; frame.payload = reinterpret_cast("\x05\x05\x05"); REQUIRE(1 == update(0, 1'000'000, 16)); REQUIRE(rxs.transfer_timestamp_usec == 20'000'000); REQUIRE(rxs.payload_size == 0); REQUIRE(rxs.payload == nullptr); REQUIRE(rxs.calculated_crc == 0xFFFF); - REQUIRE(rxs.transfer_id == 13U); + REQUIRE(rxs.transfer_id == 12U); REQUIRE(rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 0); + REQUIRE(rxs.redundant_iface_index == 0); REQUIRE(transfer.timestamp_usec == 20'000'000); REQUIRE(transfer.metadata.priority == CanardPrioritySlow); REQUIRE(transfer.metadata.transfer_kind == CanardTransferKindMessage); REQUIRE(transfer.metadata.port_id == 2'222); REQUIRE(transfer.metadata.remote_node_id == 55); - REQUIRE(transfer.metadata.transfer_id == 12); + REQUIRE(transfer.metadata.transfer_id == 11); REQUIRE(transfer.payload_size == 3); REQUIRE(0 == std::memcmp(transfer.payload, "\x05\x05\x05", 3)); REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); @@ -432,7 +455,7 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.calculated_crc == crc("\x06\x06\x06\x06\x06\x06\x06")); REQUIRE(rxs.transfer_id == 13U); REQUIRE(!rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 0); + REQUIRE(rxs.redundant_iface_index == 0); REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16); @@ -449,7 +472,7 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.calculated_crc == crc("\x06\x06\x06\x06\x06\x06\x06\x07\x07\x07\x07\x07\x07\x07")); REQUIRE(rxs.transfer_id == 13U); REQUIRE(rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 0); + REQUIRE(rxs.redundant_iface_index == 0); REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16); @@ -466,7 +489,7 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.calculated_crc == crc("\x06\x06\x06\x06\x06\x06\x06\x07\x07\x07\x07\x07\x07\x07")); REQUIRE(rxs.transfer_id == 13U); REQUIRE(rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 0); + REQUIRE(rxs.redundant_iface_index == 0); REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16); @@ -484,7 +507,7 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.calculated_crc == 0xFFFF); REQUIRE(rxs.transfer_id == 14U); REQUIRE(rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 0); + REQUIRE(rxs.redundant_iface_index == 0); REQUIRE(transfer.timestamp_usec == 20'000'100); REQUIRE(transfer.metadata.priority == CanardPrioritySlow); REQUIRE(transfer.metadata.transfer_kind == CanardTransferKindMessage); @@ -509,7 +532,7 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.transfer_timestamp_usec == 20'000'100); // No change. REQUIRE(rxs.transfer_id == 14U); // No change. REQUIRE(rxs.toggle); // No change. - REQUIRE(rxs.redundant_transport_index == 0); // No change. + REQUIRE(rxs.redundant_iface_index == 0); // No change. REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 0); REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 0); @@ -526,9 +549,9 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.payload_size == 7); // From the frame. REQUIRE(rxs.payload != nullptr); REQUIRE(rxs.calculated_crc == 0x23C7); - REQUIRE(rxs.transfer_id == 12U); // Updated from the frame. - REQUIRE(!rxs.toggle); // In anticipation of the next frame. - REQUIRE(rxs.redundant_transport_index == 2); // Updated from the update. + REQUIRE(rxs.transfer_id == 12U); // Updated from the frame. + REQUIRE(!rxs.toggle); // In anticipation of the next frame. + REQUIRE(rxs.redundant_iface_index == 2); // Updated from the update. REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16); @@ -547,7 +570,7 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.calculated_crc == crc("\x0B\x0B\x0B\x0B\x0B\x0B\x0B")); REQUIRE(rxs.transfer_id == 10U); REQUIRE(!rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 2); + REQUIRE(rxs.redundant_iface_index == 2); REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16); @@ -566,7 +589,7 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.calculated_crc == crc("\x0B\x0B\x0B\x0B\x0B\x0B\x0B")); REQUIRE(rxs.transfer_id == 10U); REQUIRE(!rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 2); + REQUIRE(rxs.redundant_iface_index == 2); REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16); @@ -585,7 +608,7 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.calculated_crc == 0xFFFF); REQUIRE(rxs.transfer_id == 11U); REQUIRE(rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 2); + REQUIRE(rxs.redundant_iface_index == 2); REQUIRE(transfer.timestamp_usec == 20'000'200); REQUIRE(transfer.metadata.priority == CanardPrioritySlow); REQUIRE(transfer.metadata.transfer_kind == CanardTransferKindMessage); @@ -613,7 +636,7 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.calculated_crc == crc("\x0E\x0E\x0E\x0E\x0E\x0E\x0E\xF7")); REQUIRE(rxs.transfer_id == 0); REQUIRE(!rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 0); + REQUIRE(rxs.redundant_iface_index == 0); REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16); @@ -632,7 +655,7 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.calculated_crc == 0xFFFF); REQUIRE(rxs.transfer_id == 1U); REQUIRE(rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 0); + REQUIRE(rxs.redundant_iface_index == 0); REQUIRE(transfer.timestamp_usec == 30'000'000); REQUIRE(transfer.metadata.priority == CanardPrioritySlow); REQUIRE(transfer.metadata.transfer_kind == CanardTransferKindMessage); @@ -660,7 +683,7 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.calculated_crc == crc("\x0E\x0E\x0E\x0E\x0E\x0E\x0E\xF7")); REQUIRE(rxs.transfer_id == 31U); REQUIRE(!rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 0); + REQUIRE(rxs.redundant_iface_index == 0); REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16); @@ -679,7 +702,7 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.calculated_crc == 0xFFFF); REQUIRE(rxs.transfer_id == 0U); REQUIRE(rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 0); + REQUIRE(rxs.redundant_iface_index == 0); REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 0); // Deallocated on failure. REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 0); @@ -698,7 +721,7 @@ TEST_CASE("rxSessionUpdate") REQUIRE(rxs.calculated_crc == 0xFFFF); REQUIRE(rxs.transfer_id == 31U); // Reset. REQUIRE(rxs.toggle); - REQUIRE(rxs.redundant_transport_index == 2); + REQUIRE(rxs.redundant_iface_index == 2); REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 0); REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 0); } diff --git a/tests/test_public_rx.cpp b/tests/test_public_rx.cpp index 695f8ef..932d654 100644 --- a/tests/test_public_rx.cpp +++ b/tests/test_public_rx.cpp @@ -22,7 +22,7 @@ TEST_CASE("RxBasic0") CanardRxTransfer transfer{}; CanardRxSubscription* subscription = nullptr; - const auto accept = [&](const std::uint8_t redundant_transport_index, + const auto accept = [&](const std::uint8_t redundant_iface_index, const CanardMicrosecond timestamp_usec, const std::uint32_t extended_can_id, const std::vector& payload) { @@ -32,7 +32,7 @@ TEST_CASE("RxBasic0") frame.extended_can_id = extended_can_id; frame.payload_size = std::size(payload); frame.payload = payload_storage.data(); - return ins.rxAccept(timestamp_usec, frame, redundant_transport_index, transfer, &subscription); + return ins.rxAccept(timestamp_usec, frame, redundant_iface_index, transfer, &subscription); }; ins.getAllocator().setAllocationCeiling(sizeof(RxSession) + 16); // A session and a 16-byte payload buffer. @@ -215,7 +215,7 @@ TEST_CASE("RxAnonymous") CanardRxTransfer transfer{}; CanardRxSubscription* subscription = nullptr; - const auto accept = [&](const std::uint8_t redundant_transport_index, + const auto accept = [&](const std::uint8_t redundant_iface_index, const CanardMicrosecond timestamp_usec, const std::uint32_t extended_can_id, const std::vector& payload) { @@ -225,7 +225,7 @@ TEST_CASE("RxAnonymous") frame.extended_can_id = extended_can_id; frame.payload_size = std::size(payload); frame.payload = payload_storage.data(); - return ins.rxAccept(timestamp_usec, frame, redundant_transport_index, transfer, &subscription); + return ins.rxAccept(timestamp_usec, frame, redundant_iface_index, transfer, &subscription); }; ins.getAllocator().setAllocationCeiling(16); @@ -339,8 +339,8 @@ TEST_CASE("Issue189") // https://github.com/OpenCyphal/libcanard/issues/189 Instance ins; CanardRxTransfer transfer{}; - CanardRxSubscription* subscription = nullptr; - const std::uint8_t redundant_transport_index = 0; + CanardRxSubscription* subscription = nullptr; + const std::uint8_t redundant_iface_index = 0; const auto accept = [&](const CanardMicrosecond timestamp_usec, const std::uint32_t extended_can_id, @@ -351,7 +351,7 @@ TEST_CASE("Issue189") // https://github.com/OpenCyphal/libcanard/issues/189 frame.extended_can_id = extended_can_id; frame.payload_size = std::size(payload); frame.payload = payload_storage.data(); - return ins.rxAccept(timestamp_usec, frame, redundant_transport_index, transfer, &subscription); + return ins.rxAccept(timestamp_usec, frame, redundant_iface_index, transfer, &subscription); }; ins.getAllocator().setAllocationCeiling(sizeof(RxSession) + 50); // A session and the payload buffer. @@ -449,7 +449,7 @@ TEST_CASE("Issue212") CanardRxSubscription* subscription = nullptr; const auto accept = [&](const CanardMicrosecond timestamp_usec, - const std::uint8_t redundant_transport_index, + const std::uint8_t redundant_iface_index, const std::uint32_t extended_can_id, const std::vector& payload) { static std::vector payload_storage; @@ -458,7 +458,7 @@ TEST_CASE("Issue212") frame.extended_can_id = extended_can_id; frame.payload_size = std::size(payload); frame.payload = payload_storage.data(); - return ins.rxAccept(timestamp_usec, frame, redundant_transport_index, transfer, &subscription); + return ins.rxAccept(timestamp_usec, frame, redundant_iface_index, transfer, &subscription); }; ins.getAllocator().setAllocationCeiling(sizeof(RxSession) + 50); // A session and the payload buffer. @@ -509,7 +509,7 @@ TEST_CASE("Issue212") REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); // The payload buffer is gone. REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == sizeof(RxSession)); - // Similar but the reassembler should switch to the other transport. + // Similar but the reassembler should NOT switch to the other transport. REQUIRE(0 == accept(110'000'001, // first frame, transport #1 1, 0b001'00'0'11'0110011001100'0'0100111, @@ -543,3 +543,114 @@ TEST_CASE("Issue212") REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); // The payload buffer is gone. REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == sizeof(RxSession)); } + +TEST_CASE("RxFixedTIDWithSmallTimeout") +{ + using helpers::Instance; + using exposed::RxSession; + + Instance ins; + CanardRxTransfer transfer{}; + CanardRxSubscription* subscription = nullptr; + + const auto accept = [&](const CanardMicrosecond timestamp_usec, + const std::uint32_t extended_can_id, + const std::vector& payload) { + static std::vector payload_storage; + payload_storage = payload; + CanardFrame frame{}; + frame.extended_can_id = extended_can_id; + frame.payload_size = std::size(payload); + frame.payload = payload_storage.data(); + return ins.rxAccept(timestamp_usec, frame, 0, transfer, &subscription); + }; + + ins.getAllocator().setAllocationCeiling(sizeof(RxSession) + 50); // A session and the payload buffer. + + // Create a message subscription with the transfer-ID timeout of just five microseconds. + CanardRxSubscription sub_msg{}; + REQUIRE(1 == ins.rxSubscribe(CanardTransferKindMessage, 0b0110011001100, 50, 5, sub_msg)); + REQUIRE(ins.getMessageSubs().at(0) == &sub_msg); + REQUIRE(ins.getMessageSubs().at(0)->port_id == 0b0110011001100); + REQUIRE(ins.getMessageSubs().at(0)->extent == 50); + REQUIRE(ins.getMessageSubs().at(0)->transfer_id_timeout_usec == 5); + REQUIRE(ensureAllNullptr(ins.getMessageSubs().at(0)->sessions)); + REQUIRE(ins.getResponseSubs().empty()); + REQUIRE(ins.getRequestSubs().empty()); + + // Feed a valid multi-frame transfer. + // Here's how we compute the reference value of the transfer CRC: + // >>> from pycyphal.transport.commons.crc import CRC16CCITT + // >>> CRC16CCITT.new(bytes([1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14])).value_as_bytes + // b'2\xf8' + REQUIRE(0 == accept(100'000'000, // first frame + 0b001'00'0'11'0110011001100'0'0100111, + {1, 2, 3, 4, 5, 6, 7, 0b101'00000})); + REQUIRE(0 == accept(100'000'001, // second frame, one us later + 0b001'00'0'11'0110011001100'0'0100111, + {8, 9, 10, 11, 12, 13, 14, 0b000'00000})); + REQUIRE(1 == accept(100'000'020, // third and last frame, large delay greater than the timeout + 0b001'00'0'11'0110011001100'0'0100111, + {0x32, 0xF8, 0b011'00000})); + REQUIRE(subscription != nullptr); // Subscription exists. + REQUIRE(subscription->port_id == 0b0110011001100); + REQUIRE(transfer.timestamp_usec == 100'000'000); + REQUIRE(transfer.metadata.priority == CanardPriorityImmediate); + REQUIRE(transfer.metadata.transfer_kind == CanardTransferKindMessage); + REQUIRE(transfer.metadata.port_id == 0b0110011001100); + REQUIRE(transfer.metadata.remote_node_id == 0b0100111); + REQUIRE(transfer.metadata.transfer_id == 0); + REQUIRE(transfer.payload_size == 14); + REQUIRE(0 == std::memcmp(transfer.payload, "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0A\x0B\x0C\x0D\x0E", 14)); + REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 2); // The SESSION and the PAYLOAD BUFFER. + REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (sizeof(RxSession) + 50)); + REQUIRE(ins.getMessageSubs().at(0)->sessions[0b0100111] != nullptr); + ins.getAllocator().deallocate(transfer.payload); + REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); // The payload buffer is gone. + REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == sizeof(RxSession)); + + // Another transfer with the same transfer-ID but past the transfer-ID timeout; it should be accepted. + REQUIRE(0 == accept(100'000'100, // first frame + 0b001'00'0'11'0110011001100'0'0100111, + {1, 2, 3, 4, 5, 6, 7, 0b101'00000})); + REQUIRE(1 == accept(100'000'101, // third and last frame + 0b001'00'0'11'0110011001100'0'0100111, + {8, 0x47, 0x92, 0b010'00000})); + REQUIRE(subscription != nullptr); // Subscription exists. + REQUIRE(subscription->port_id == 0b0110011001100); + REQUIRE(transfer.timestamp_usec == 100'000'100); + REQUIRE(transfer.metadata.priority == CanardPriorityImmediate); + REQUIRE(transfer.metadata.transfer_kind == CanardTransferKindMessage); + REQUIRE(transfer.metadata.port_id == 0b0110011001100); + REQUIRE(transfer.metadata.remote_node_id == 0b0100111); + REQUIRE(transfer.metadata.transfer_id == 0); // same + REQUIRE(transfer.payload_size == 8); + REQUIRE(0 == std::memcmp(transfer.payload, "\x01\x02\x03\x04\x05\x06\x07\x08", 8)); + REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 2); // The SESSION and the PAYLOAD BUFFER. + REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (sizeof(RxSession) + 50)); + REQUIRE(ins.getMessageSubs().at(0)->sessions[0b0100111] != nullptr); + ins.getAllocator().deallocate(transfer.payload); + REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); // The payload buffer is gone. + REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == sizeof(RxSession)); + + // Same but now single-frame. + REQUIRE(1 == accept(100'000'200, // the only frame + 0b001'00'0'11'0110011001100'0'0100111, + {1, 2, 3, 4, 5, 6, 7, 0b111'00000})); + REQUIRE(subscription != nullptr); // Subscription exists. + REQUIRE(subscription->port_id == 0b0110011001100); + REQUIRE(transfer.timestamp_usec == 100'000'200); + REQUIRE(transfer.metadata.priority == CanardPriorityImmediate); + REQUIRE(transfer.metadata.transfer_kind == CanardTransferKindMessage); + REQUIRE(transfer.metadata.port_id == 0b0110011001100); + REQUIRE(transfer.metadata.remote_node_id == 0b0100111); + REQUIRE(transfer.metadata.transfer_id == 0); // same + REQUIRE(transfer.payload_size == 7); + REQUIRE(0 == std::memcmp(transfer.payload, "\x01\x02\x03\x04\x05\x06\x07", 7)); + REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 2); // The SESSION and the PAYLOAD BUFFER. + REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (sizeof(RxSession) + 50)); + REQUIRE(ins.getMessageSubs().at(0)->sessions[0b0100111] != nullptr); + ins.getAllocator().deallocate(transfer.payload); + REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); // The payload buffer is gone. + REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == sizeof(RxSession)); +}