From 5670ba5e6033d25e131024df765352f053fa67b2 Mon Sep 17 00:00:00 2001 From: Sergei Shirokov Date: Tue, 3 Dec 2024 17:20:49 +0200 Subject: [PATCH 01/14] introduce deadline queue #sonar --- libcanard/canard.c | 124 ++++++++++++++++++++++++++++----------- libcanard/canard.h | 27 +++++++-- tests/helpers.hpp | 21 +++++-- tests/test_public_tx.cpp | 34 +++++------ 4 files changed, 148 insertions(+), 58 deletions(-) diff --git a/libcanard/canard.c b/libcanard/canard.c index 95d8280..f884015 100644 --- a/libcanard/canard.c +++ b/libcanard/canard.c @@ -3,6 +3,7 @@ /// Author: Pavel Kirienko #include "canard.h" +#include #include // --------------------------------------------- BUILD CONFIGURATION --------------------------------------------- @@ -299,10 +300,15 @@ CANARD_PRIVATE struct CanardTxQueueItem* txAllocateQueueItem(struct CanardTxQueu struct CanardTxQueueItem* out = ins->memory.allocate(ins->memory.user_reference, sizeof(struct CanardTxQueueItem)); if (out != NULL) { - out->base.up = NULL; - out->base.lr[0] = NULL; - out->base.lr[1] = NULL; - out->base.bf = 0; + out->priority_base.up = NULL; + out->priority_base.lr[0] = NULL; + out->priority_base.lr[1] = NULL; + out->priority_base.bf = 0; + + out->deadline_base.up = NULL; + out->deadline_base.lr[0] = NULL; + out->deadline_base.lr[1] = NULL; + out->deadline_base.bf = 0; out->next_in_transfer = NULL; // Last by default. out->tx_deadline_usec = deadline_usec; @@ -326,18 +332,39 @@ CANARD_PRIVATE struct CanardTxQueueItem* txAllocateQueueItem(struct CanardTxQueu return out; } +#define CONTAINER_OF(type, ptr, member) ((type*) (((ptr) == NULL) ? NULL : ((char*) (ptr) - offsetof(type, member)))) + /// Frames with identical CAN ID that are added later always compare greater than their counterparts with same CAN ID. /// This ensures that CAN frames with the same CAN ID are transmitted in the FIFO order. /// Frames that should be transmitted earlier compare smaller (i.e., put on the left side of the tree). -CANARD_PRIVATE int8_t txAVLPredicate(void* const user_reference, // NOSONAR Cavl API requires pointer to non-const. - const struct CanardTreeNode* const node) +CANARD_PRIVATE int8_t txAVLPriorityPredicate( // + void* const user_reference, // NOSONAR Cavl API requires pointer to non-const. + const struct CanardTreeNode* const node) { - const struct CanardTxQueueItem* const target = (const struct CanardTxQueueItem*) user_reference; - const struct CanardTxQueueItem* const other = (const struct CanardTxQueueItem*) (const void*) node; + typedef const struct CanardTxQueueItem ConstTxItem; + + ConstTxItem* const target = CONTAINER_OF(ConstTxItem, user_reference, priority_base); + ConstTxItem* const other = CONTAINER_OF(ConstTxItem, node, priority_base); CANARD_ASSERT((target != NULL) && (other != NULL)); return (target->frame.extended_can_id >= other->frame.extended_can_id) ? +1 : -1; } +/// Frames with identical deadline +/// that are added later always compare greater than their counterparts with the same deadline. +/// This ensures that CAN frames with the same deadline are, when timed out, dropped in the FIFO order. +/// Frames that should be dropped earlier compare smaller (i.e., put on the left side of the tree). +CANARD_PRIVATE int8_t txAVLDeadlinePredicate( // + void* const user_reference, // NOSONAR Cavl API requires pointer to non-const. + const struct CanardTreeNode* const node) +{ + typedef const struct CanardTxQueueItem ConstTxItem; + + ConstTxItem* const target = CONTAINER_OF(ConstTxItem, user_reference, deadline_base); + ConstTxItem* const other = CONTAINER_OF(ConstTxItem, node, deadline_base); + CANARD_ASSERT((target != NULL) && (other != NULL)); + return (target->tx_deadline_usec >= other->tx_deadline_usec) ? +1 : -1; +} + /// Returns the number of frames enqueued or error (i.e., =1 or <0). CANARD_PRIVATE int32_t txPushSingleFrame(struct CanardTxQueue* const que, const struct CanardInstance* const ins, @@ -369,11 +396,19 @@ CANARD_PRIVATE int32_t txPushSingleFrame(struct CanardTxQueue* const que, uint8_t* const frame_bytes = tqi->frame.payload.data; (void) memset(frame_bytes + payload.size, PADDING_BYTE_VALUE, padding_size); // NOLINT *(frame_bytes + (frame_payload_size - 1U)) = txMakeTailByte(true, true, true, transfer_id); - // Insert the newly created TX item into the queue. - const struct CanardTreeNode* const res = - cavlSearch(&que->root, &tqi->base, &txAVLPredicate, &avlTrivialFactory); - (void) res; - CANARD_ASSERT(res == &tqi->base); + + // Insert the newly created TX item into the priority queue. + const struct CanardTreeNode* const priority_queue_res = + cavlSearch(&que->priority_root, &tqi->priority_base, &txAVLPriorityPredicate, &avlTrivialFactory); + (void) priority_queue_res; + CANARD_ASSERT(priority_queue_res == &tqi->priority_base); + + // Insert the newly created TX item into the deadline queue. + const struct CanardTreeNode* const deadline_queue_res = + cavlSearch(&que->deadline_root, &tqi->deadline_base, &txAVLDeadlinePredicate, &avlTrivialFactory); + (void) deadline_queue_res; + CANARD_ASSERT(deadline_queue_res == &tqi->deadline_base); + que->size++; CANARD_ASSERT(que->size <= que->capacity); out = 1; // One frame enqueued. @@ -514,11 +549,18 @@ CANARD_PRIVATE int32_t txPushMultiFrame(struct CanardTxQueue* const que, struct CanardTxQueueItem* next = sq.head; do { - const struct CanardTreeNode* const res = - cavlSearch(&que->root, &next->base, &txAVLPredicate, &avlTrivialFactory); - (void) res; - CANARD_ASSERT(res == &next->base); - CANARD_ASSERT(que->root != NULL); + const struct CanardTreeNode* const priority_queue_res = + cavlSearch(&que->priority_root, &next->priority_base, &txAVLPriorityPredicate, &avlTrivialFactory); + (void) priority_queue_res; + CANARD_ASSERT(priority_queue_res == &next->priority_base); + CANARD_ASSERT(que->priority_root != NULL); + + const struct CanardTreeNode* const deadline_queue_res = + cavlSearch(&que->deadline_root, &next->deadline_base, &txAVLDeadlinePredicate, &avlTrivialFactory); + (void) deadline_queue_res; + CANARD_ASSERT(deadline_queue_res == &next->deadline_base); + CANARD_ASSERT(que->deadline_root != NULL); + next = next->next_in_transfer; } while (next != NULL); CANARD_ASSERT(num_frames == sq.size); @@ -1006,7 +1048,7 @@ rxSubscriptionPredicateOnPortID(void* const user_reference, // NOSONAR Cavl API const struct CanardTreeNode* const node) { const CanardPortID sought = *((const CanardPortID*) user_reference); - const CanardPortID other = ((const struct CanardRxSubscription*) (const void*) node)->port_id; + const CanardPortID other = CONTAINER_OF(const struct CanardRxSubscription, node, base)->port_id; static const int8_t NegPos[2] = {-1, +1}; // Clang-Tidy mistakenly identifies a narrowing cast to int8_t here, which is incorrect. return (sought == other) ? 0 : NegPos[sought > other]; // NOLINT no narrowing conversion is taking place here @@ -1016,7 +1058,9 @@ CANARD_PRIVATE int8_t rxSubscriptionPredicateOnStruct(void* const user_reference, // NOSONAR Cavl API requires pointer to non-const. const struct CanardTreeNode* const node) { - return rxSubscriptionPredicateOnPortID(&((struct CanardRxSubscription*) user_reference)->port_id, node); + return rxSubscriptionPredicateOnPortID( // + &CONTAINER_OF(struct CanardRxSubscription, user_reference, base)->port_id, + node); } // --------------------------------------------- PUBLIC API --------------------------------------------- @@ -1056,7 +1100,8 @@ struct CanardTxQueue canardTxInit(const size_t capacity, .capacity = capacity, .mtu_bytes = mtu_bytes, .size = 0, - .root = NULL, + .priority_root = NULL, + .deadline_root = NULL, .memory = memory, .user_reference = NULL, }; @@ -1067,8 +1112,11 @@ int32_t canardTxPush(struct CanardTxQueue* const que, const struct CanardInstance* const ins, const CanardMicrosecond tx_deadline_usec, const struct CanardTransferMetadata* const metadata, - const struct CanardPayload payload) + const struct CanardPayload payload, + const CanardMicrosecond now_usec) { + (void) now_usec; + int32_t out = -CANARD_ERROR_INVALID_ARGUMENT; if ((ins != NULL) && (que != NULL) && (metadata != NULL) && ((payload.data != NULL) || (0U == payload.size))) { @@ -1114,7 +1162,8 @@ struct CanardTxQueueItem* canardTxPeek(const struct CanardTxQueue* const que) { // Paragraph 6.7.2.1.15 of the C standard says: // A pointer to a structure object, suitably converted, points to its initial member, and vice versa. - out = (struct CanardTxQueueItem*) (void*) cavlFindExtremum(que->root, false); + struct CanardTreeNode* const priority_node = cavlFindExtremum(que->priority_root, false); + out = CONTAINER_OF(struct CanardTxQueueItem, priority_node, priority_base); } return out; } @@ -1127,7 +1176,8 @@ struct CanardTxQueueItem* canardTxPop(struct CanardTxQueue* const que, struct Ca // A pointer to a structure object, suitably converted, points to its initial member, and vice versa. // Note that the highest-priority frame is always a leaf node in the AVL tree, which means that it is very // cheap to remove. - cavlRemove(&que->root, &item->base); + cavlRemove(&que->priority_root, &item->priority_base); + cavlRemove(&que->deadline_root, &item->deadline_base); que->size--; } return item; @@ -1169,11 +1219,13 @@ int8_t canardRxAccept(struct CanardInstance* const ins, // This is the reason the function has a logarithmic time complexity of the number of subscriptions. // Note also that this one of the two variable-complexity operations in the RX pipeline; the other one // is memcpy(). Excepting these two cases, the entire RX pipeline contains neither loops nor recursion. - struct CanardRxSubscription* const sub = (struct CanardRxSubscription*) (void*) + struct CanardRxSubscription* const sub = CONTAINER_OF( // + struct CanardRxSubscription, cavlSearch(&ins->rx_subscriptions[(size_t) model.transfer_kind], &model.port_id, &rxSubscriptionPredicateOnPortID, - NULL); + NULL), + base); if (out_subscription != NULL) { *out_subscription = sub; // Expose selected instance to the caller. @@ -1230,7 +1282,7 @@ int8_t canardRxSubscribe(struct CanardInstance* const ins, out_subscription->sessions[i] = NULL; } const struct CanardTreeNode* const res = cavlSearch(&ins->rx_subscriptions[tk], - out_subscription, + &out_subscription->base, &rxSubscriptionPredicateOnStruct, &avlTrivialFactory); (void) res; @@ -1249,9 +1301,12 @@ int8_t canardRxUnsubscribe(struct CanardInstance* const ins, const size_t tk = (size_t) transfer_kind; if ((ins != NULL) && (tk < CANARD_NUM_TRANSFER_KINDS)) { - CanardPortID port_id_mutable = port_id; - struct CanardRxSubscription* const sub = (struct CanardRxSubscription*) (void*) - cavlSearch(&ins->rx_subscriptions[tk], &port_id_mutable, &rxSubscriptionPredicateOnPortID, NULL); + CanardPortID port_id_mutable = port_id; + + struct CanardRxSubscription* const sub = CONTAINER_OF( // + struct CanardRxSubscription, + cavlSearch(&ins->rx_subscriptions[tk], &port_id_mutable, &rxSubscriptionPredicateOnPortID, NULL), + base); if (sub != NULL) { cavlRemove(&ins->rx_subscriptions[tk], &sub->base); @@ -1287,9 +1342,12 @@ int8_t canardRxGetSubscription(struct CanardInstance* const ins, const size_t tk = (size_t) transfer_kind; if ((ins != NULL) && (tk < CANARD_NUM_TRANSFER_KINDS)) { - CanardPortID port_id_mutable = port_id; - struct CanardRxSubscription* const sub = (struct CanardRxSubscription*) (void*) - cavlSearch(&ins->rx_subscriptions[tk], &port_id_mutable, &rxSubscriptionPredicateOnPortID, NULL); + CanardPortID port_id_mutable = port_id; + + struct CanardRxSubscription* const sub = CONTAINER_OF( // + struct CanardRxSubscription, + cavlSearch(&ins->rx_subscriptions[tk], &port_id_mutable, &rxSubscriptionPredicateOnPortID, NULL), + base); if (sub != NULL) { CANARD_ASSERT(sub->port_id == port_id); diff --git a/libcanard/canard.h b/libcanard/canard.h index 443ccf2..d76bb7c 100644 --- a/libcanard/canard.h +++ b/libcanard/canard.h @@ -306,6 +306,13 @@ struct CanardMemoryResource CanardMemoryAllocate allocate; ///< Shall be a valid pointer. }; +/// Holds the statistics of a transmission queue. +struct CanardTxQueueStats +{ + /// Holds number of dropped TX frames (due to timeout when `now > deadline`). + size_t dropped_frames; +}; + /// Prioritized transmission queue that keeps CAN frames destined for transmission via one CAN interface. /// Applications with redundant interfaces are expected to have one instance of this type per interface. /// Applications that are not interested in transmission may have zero queues. @@ -335,7 +342,10 @@ struct CanardTxQueue size_t size; /// The root of the priority queue is NULL if the queue is empty. Do not modify this field! - struct CanardTreeNode* root; + struct CanardTreeNode* priority_root; + + /// The root of the deadline queue is NULL if the queue is empty. Do not modify this field! + struct CanardTreeNode* deadline_root; /// The memory resource used by this queue for allocating the payload data (CAN frames). /// There is exactly one allocation of payload buffer per enqueued item (not considering the item itself @@ -354,13 +364,19 @@ struct CanardTxQueue /// This field can be arbitrarily mutated by the user. It is never accessed by the library. /// Its purpose is to simplify integration with OOP interfaces. void* user_reference; + + /// Holds the statistics of this TX queue. + struct CanardTxQueueStats stats; }; /// One frame stored in the transmission queue along with its metadata. struct CanardTxQueueItem { /// Internal use only; do not access this field. - struct CanardTreeNode base; + struct CanardTreeNode priority_base; + + /// Internal use only; do not access this field. + struct CanardTreeNode deadline_base; /// Points to the next frame in this transfer or NULL. This field is mostly intended for own needs of the library. /// Normally, the application would not use it because transfer frame ordering is orthogonal to global TX ordering. @@ -502,7 +518,9 @@ struct CanardTxQueue canardTxInit(const size_t capacity, /// frames (so all frames will have the same timestamp value). This feature is intended to facilitate transmission /// deadline tracking, i.e., aborting frames that could not be transmitted before the specified deadline. /// Therefore, normally, the timestamp value should be in the future. -/// The library itself, however, does not use or check this value in any way, so it can be zero if not needed. +/// The library uses `now > deadline` comparison to determine which frames timed out, and so could +/// be dropped (incrementing `CanardTxQueueStats::dropped_frames` field per such a frame). +/// If this timeout behavior is not needed, the timestamp value can be set to zero. /// /// The function returns the number of frames enqueued into the prioritized TX queue (which is always a positive /// number) in case of success (so that the application can track the number of items in the TX queue if necessary). @@ -539,7 +557,8 @@ int32_t canardTxPush(struct CanardTxQueue* const que, const struct CanardInstance* const ins, const CanardMicrosecond tx_deadline_usec, const struct CanardTransferMetadata* const metadata, - const struct CanardPayload payload); + const struct CanardPayload payload, + const CanardMicrosecond now_usec); /// This function accesses the top element of the prioritized transmission queue. The queue itself is not modified /// (i.e., the accessed element is not removed). The application should invoke this function to collect the transport diff --git a/tests/helpers.hpp b/tests/helpers.hpp index 12fdd33..322d90d 100644 --- a/tests/helpers.hpp +++ b/tests/helpers.hpp @@ -296,11 +296,12 @@ class TxQueue [[nodiscard]] auto push(CanardInstance* const ins, const CanardMicrosecond transmission_deadline_usec, const CanardTransferMetadata& metadata, - const struct CanardPayload payload) + const struct CanardPayload payload, + const CanardMicrosecond now_usec = 0ULL) { checkInvariants(); const auto size_before = que_.size; - const auto ret = canardTxPush(&que_, ins, transmission_deadline_usec, &metadata, payload); + const auto ret = canardTxPush(&que_, ins, transmission_deadline_usec, &metadata, payload, now_usec); const auto num_added = static_cast(ret); enforce((ret < 0) || ((size_before + num_added) == que_.size), "Unexpected size change after push"); checkInvariants(); @@ -341,7 +342,18 @@ class TxQueue [[nodiscard]] auto getSize() const { std::size_t out = 0; - traverse(que_.root, [&](auto* _) { + traverse(que_.priority_root, [&](auto* _) { + (void) _; + out++; + }); + enforce(que_.size == out, "Size miscalculation"); + return out; + } + + [[nodiscard]] auto getDeadlineQueueSize() const + { + std::size_t out = 0; + traverse(que_.deadline_root, [&](auto* _) { (void) _; out++; }); @@ -352,7 +364,7 @@ class TxQueue [[nodiscard]] auto linearize() const -> std::vector { std::vector out; - traverse(que_.root, [&](const CanardTreeNode* const item) { + traverse(que_.priority_root, [&](const CanardTreeNode* const item) { out.push_back(reinterpret_cast(item)); }); enforce(out.size() == getSize(), "Internal error"); @@ -393,6 +405,7 @@ class TxQueue { enforce(que_.user_reference == this, "User reference damaged"); enforce(que_.size == getSize(), "Size miscalculation"); + enforce(que_.size == getDeadlineQueueSize(), "Deadline queue size miscalculation"); } TestAllocator allocator_; diff --git a/tests/test_public_tx.cpp b/tests/test_public_tx.cpp index 416816b..c852a2d 100644 --- a/tests/test_public_tx.cpp +++ b/tests/test_public_tx.cpp @@ -30,7 +30,7 @@ TEST_CASE("TxBasic0") REQUIRE(0 == que.getSize()); REQUIRE(0 == alloc.getNumAllocatedFragments()); - alloc.setAllocationCeiling(400); + alloc.setAllocationCeiling(496); CanardTransferMetadata meta{}; @@ -72,7 +72,7 @@ TEST_CASE("TxBasic0") REQUIRE(3 == que.getSize()); REQUIRE(6 == alloc.getNumAllocatedFragments()); REQUIRE(20 < alloc.getTotalAllocatedAmount()); - REQUIRE(400 > alloc.getTotalAllocatedAmount()); + REQUIRE(496 > alloc.getTotalAllocatedAmount()); // Check the TX queue. { @@ -123,7 +123,7 @@ TEST_CASE("TxBasic0") REQUIRE(3 == que.getSize()); REQUIRE(6 == alloc.getNumAllocatedFragments()); REQUIRE(20 < alloc.getTotalAllocatedAmount()); - REQUIRE(400 > alloc.getTotalAllocatedAmount()); + REQUIRE(496 > alloc.getTotalAllocatedAmount()); // Pop the queue. // hex(pycyphal.transport.commons.crc.CRC16CCITT.new(list(range(8))).value) @@ -190,7 +190,7 @@ TEST_CASE("TxBasic0") REQUIRE(3 == que.getSize()); REQUIRE(6 == alloc.getNumAllocatedFragments()); REQUIRE(40 < alloc.getTotalAllocatedAmount()); - REQUIRE(400 > alloc.getTotalAllocatedAmount()); + REQUIRE(496 > alloc.getTotalAllocatedAmount()); // Read the generated frames. ti = que.peek(); REQUIRE(nullptr != ti); @@ -235,7 +235,7 @@ TEST_CASE("TxBasic0") REQUIRE(3 == que.getSize()); REQUIRE(6 == alloc.getNumAllocatedFragments()); REQUIRE(40 < alloc.getTotalAllocatedAmount()); - REQUIRE(400 > alloc.getTotalAllocatedAmount()); + REQUIRE(496 > alloc.getTotalAllocatedAmount()); // Read the generated frames. ti = que.peek(); REQUIRE(nullptr != ti); @@ -341,11 +341,11 @@ TEST_CASE("TxBasic0") REQUIRE(nullptr == ti); // Error handling. - REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == canardTxPush(nullptr, nullptr, 0, nullptr, {0, nullptr})); - REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == canardTxPush(nullptr, nullptr, 0, &meta, {0, nullptr})); - REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == canardTxPush(nullptr, &ins.getInstance(), 0, &meta, {0, nullptr})); + REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == canardTxPush(nullptr, nullptr, 0, nullptr, {0, nullptr}, 0)); + REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == canardTxPush(nullptr, nullptr, 0, &meta, {0, nullptr}, 0)); + REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == canardTxPush(nullptr, &ins.getInstance(), 0, &meta, {0, nullptr}, 0)); REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == - canardTxPush(&que.getInstance(), &ins.getInstance(), 0, nullptr, {0, nullptr})); + canardTxPush(&que.getInstance(), &ins.getInstance(), 0, nullptr, {0, nullptr}, 0)); REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == que.push(&ins.getInstance(), 1'000'000'006'000ULL, meta, {1, nullptr})); REQUIRE(nullptr == canardTxPeek(nullptr)); @@ -408,7 +408,7 @@ TEST_CASE("TxBasic1") REQUIRE(3 == que.getSize()); REQUIRE(6 == alloc.getNumAllocatedFragments()); REQUIRE(20 < alloc.getTotalAllocatedAmount()); - REQUIRE(400 > alloc.getTotalAllocatedAmount()); + REQUIRE(496 > alloc.getTotalAllocatedAmount()); // Check the TX queue. { @@ -449,7 +449,7 @@ TEST_CASE("TxBasic1") REQUIRE(3 == que.getSize()); REQUIRE(6 == alloc.getNumAllocatedFragments()); REQUIRE(20 < alloc.getTotalAllocatedAmount()); - REQUIRE(400 > alloc.getTotalAllocatedAmount()); + REQUIRE(496 > alloc.getTotalAllocatedAmount()); // Pop the queue. // hex(pycyphal.transport.commons.crc.CRC16CCITT.new(list(range(8))).value) @@ -514,7 +514,7 @@ TEST_CASE("TxBasic1") REQUIRE(3 == que.getSize()); REQUIRE(6 == alloc.getNumAllocatedFragments()); REQUIRE(40 < alloc.getTotalAllocatedAmount()); - REQUIRE(400 > alloc.getTotalAllocatedAmount()); + REQUIRE(496 > alloc.getTotalAllocatedAmount()); // Read the generated frames. ti = que.peek(); REQUIRE(nullptr != ti); @@ -559,7 +559,7 @@ TEST_CASE("TxBasic1") REQUIRE(3 == que.getSize()); REQUIRE(6 == alloc.getNumAllocatedFragments()); REQUIRE(40 < alloc.getTotalAllocatedAmount()); - REQUIRE(400 > alloc.getTotalAllocatedAmount()); + REQUIRE(496 > alloc.getTotalAllocatedAmount()); // Read the generated frames. ti = que.peek(); REQUIRE(nullptr != ti); @@ -665,11 +665,11 @@ TEST_CASE("TxBasic1") REQUIRE(nullptr == ti); // Error handling. - REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == canardTxPush(nullptr, nullptr, 0, nullptr, {0, nullptr})); - REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == canardTxPush(nullptr, nullptr, 0, &meta, {0, nullptr})); - REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == canardTxPush(nullptr, &ins.getInstance(), 0, &meta, {0, nullptr})); + REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == canardTxPush(nullptr, nullptr, 0, nullptr, {0, nullptr}, 0)); + REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == canardTxPush(nullptr, nullptr, 0, &meta, {0, nullptr}, 0)); + REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == canardTxPush(nullptr, &ins.getInstance(), 0, &meta, {0, nullptr}, 0)); REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == - canardTxPush(&que.getInstance(), &ins.getInstance(), 0, nullptr, {0, nullptr})); + canardTxPush(&que.getInstance(), &ins.getInstance(), 0, nullptr, {0, nullptr}, 0)); REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == que.push(&ins.getInstance(), 1'000'000'006'000ULL, meta, {1, nullptr})); REQUIRE(nullptr == canardTxPeek(nullptr)); From 3c1535f4e816f9c70d913ab356ee513915bec716 Mon Sep 17 00:00:00 2001 From: Sergei Date: Tue, 3 Dec 2024 23:48:14 +0200 Subject: [PATCH 02/14] pr review and build fixes #sonar --- libcanard/canard.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/libcanard/canard.c b/libcanard/canard.c index f884015..3f3b000 100644 --- a/libcanard/canard.c +++ b/libcanard/canard.c @@ -71,6 +71,11 @@ #define INITIAL_TOGGLE_STATE true +#define CONTAINER_OF(type, ptr, member) \ + ((type*) (((ptr) == NULL) ? NULL : (void*) ((char*) (ptr) - offsetof(type, member)))) +#define CONST_CONTAINER_OF(type, ptr, member) \ + ((const type*) (((ptr) == NULL) ? NULL : (const void*) ((const char*) (ptr) - offsetof(type, member)))) + /// Used for inserting new items into AVL trees. CANARD_PRIVATE struct CanardTreeNode* avlTrivialFactory(void* const user_reference) { @@ -332,8 +337,6 @@ CANARD_PRIVATE struct CanardTxQueueItem* txAllocateQueueItem(struct CanardTxQueu return out; } -#define CONTAINER_OF(type, ptr, member) ((type*) (((ptr) == NULL) ? NULL : ((char*) (ptr) - offsetof(type, member)))) - /// Frames with identical CAN ID that are added later always compare greater than their counterparts with same CAN ID. /// This ensures that CAN frames with the same CAN ID are transmitted in the FIFO order. /// Frames that should be transmitted earlier compare smaller (i.e., put on the left side of the tree). @@ -341,10 +344,10 @@ CANARD_PRIVATE int8_t txAVLPriorityPredicate( // void* const user_reference, // NOSONAR Cavl API requires pointer to non-const. const struct CanardTreeNode* const node) { - typedef const struct CanardTxQueueItem ConstTxItem; + typedef struct CanardTxQueueItem TxItem; - ConstTxItem* const target = CONTAINER_OF(ConstTxItem, user_reference, priority_base); - ConstTxItem* const other = CONTAINER_OF(ConstTxItem, node, priority_base); + const TxItem* const target = CONST_CONTAINER_OF(TxItem, user_reference, priority_base); + const TxItem* const other = CONST_CONTAINER_OF(TxItem, node, priority_base); CANARD_ASSERT((target != NULL) && (other != NULL)); return (target->frame.extended_can_id >= other->frame.extended_can_id) ? +1 : -1; } @@ -357,10 +360,10 @@ CANARD_PRIVATE int8_t txAVLDeadlinePredicate( // void* const user_reference, // NOSONAR Cavl API requires pointer to non-const. const struct CanardTreeNode* const node) { - typedef const struct CanardTxQueueItem ConstTxItem; + typedef struct CanardTxQueueItem TxItem; - ConstTxItem* const target = CONTAINER_OF(ConstTxItem, user_reference, deadline_base); - ConstTxItem* const other = CONTAINER_OF(ConstTxItem, node, deadline_base); + const TxItem* const target = CONST_CONTAINER_OF(TxItem, user_reference, deadline_base); + const TxItem* const other = CONST_CONTAINER_OF(TxItem, node, deadline_base); CANARD_ASSERT((target != NULL) && (other != NULL)); return (target->tx_deadline_usec >= other->tx_deadline_usec) ? +1 : -1; } @@ -1048,7 +1051,7 @@ rxSubscriptionPredicateOnPortID(void* const user_reference, // NOSONAR Cavl API const struct CanardTreeNode* const node) { const CanardPortID sought = *((const CanardPortID*) user_reference); - const CanardPortID other = CONTAINER_OF(const struct CanardRxSubscription, node, base)->port_id; + const CanardPortID other = CONST_CONTAINER_OF(struct CanardRxSubscription, node, base)->port_id; static const int8_t NegPos[2] = {-1, +1}; // Clang-Tidy mistakenly identifies a narrowing cast to int8_t here, which is incorrect. return (sought == other) ? 0 : NegPos[sought > other]; // NOLINT no narrowing conversion is taking place here From 4f1a28c4a0888bc9d898b783821cabe2bb457f52 Mon Sep 17 00:00:00 2001 From: Sergei Date: Wed, 4 Dec 2024 00:14:35 +0200 Subject: [PATCH 03/14] build fixes #sonar --- libcanard/canard.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/libcanard/canard.c b/libcanard/canard.c index 3f3b000..3661dad 100644 --- a/libcanard/canard.c +++ b/libcanard/canard.c @@ -72,9 +72,9 @@ #define INITIAL_TOGGLE_STATE true #define CONTAINER_OF(type, ptr, member) \ - ((type*) (((ptr) == NULL) ? NULL : (void*) ((char*) (ptr) - offsetof(type, member)))) + ((type*) (((ptr) == NULL) ? NULL : (void*) ((char*) (ptr) -offsetof(type, member)))) #define CONST_CONTAINER_OF(type, ptr, member) \ - ((const type*) (((ptr) == NULL) ? NULL : (const void*) ((const char*) (ptr) - offsetof(type, member)))) + ((const type*) (((ptr) == NULL) ? NULL : (const void*) ((const char*) (ptr) -offsetof(type, member)))) /// Used for inserting new items into AVL trees. CANARD_PRIVATE struct CanardTreeNode* avlTrivialFactory(void* const user_reference) @@ -1050,6 +1050,7 @@ CANARD_PRIVATE int8_t rxSubscriptionPredicateOnPortID(void* const user_reference, // NOSONAR Cavl API requires pointer to non-const. const struct CanardTreeNode* const node) { + CANARD_ASSERT((user_reference != NULL) && (node != NULL)); const CanardPortID sought = *((const CanardPortID*) user_reference); const CanardPortID other = CONST_CONTAINER_OF(struct CanardRxSubscription, node, base)->port_id; static const int8_t NegPos[2] = {-1, +1}; From 7ec9051e9ccbd627ecec2a1acee716426c76daf7 Mon Sep 17 00:00:00 2001 From: Sergei Date: Wed, 4 Dec 2024 00:43:15 +0200 Subject: [PATCH 04/14] minor fix --- libcanard/canard.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libcanard/canard.c b/libcanard/canard.c index 3661dad..7f01803 100644 --- a/libcanard/canard.c +++ b/libcanard/canard.c @@ -72,9 +72,9 @@ #define INITIAL_TOGGLE_STATE true #define CONTAINER_OF(type, ptr, member) \ - ((type*) (((ptr) == NULL) ? NULL : (void*) ((char*) (ptr) -offsetof(type, member)))) + ((type*) (((ptr) == NULL) ? NULL : (void*) (((char*) (ptr)) -offsetof(type, member)))) #define CONST_CONTAINER_OF(type, ptr, member) \ - ((const type*) (((ptr) == NULL) ? NULL : (const void*) ((const char*) (ptr) -offsetof(type, member)))) + ((const type*) (((ptr) == NULL) ? NULL : (const void*) (((const char*) (ptr)) -offsetof(type, member)))) /// Used for inserting new items into AVL trees. CANARD_PRIVATE struct CanardTreeNode* avlTrivialFactory(void* const user_reference) From c67a59c42a87d9e4485fc68535c6dc40c3ff3017 Mon Sep 17 00:00:00 2001 From: Sergei Date: Wed, 4 Dec 2024 00:44:56 +0200 Subject: [PATCH 05/14] style fix --- libcanard/canard.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/libcanard/canard.c b/libcanard/canard.c index 7f01803..dcf9bfd 100644 --- a/libcanard/canard.c +++ b/libcanard/canard.c @@ -72,9 +72,9 @@ #define INITIAL_TOGGLE_STATE true #define CONTAINER_OF(type, ptr, member) \ - ((type*) (((ptr) == NULL) ? NULL : (void*) (((char*) (ptr)) -offsetof(type, member)))) + ((type*) (((ptr) == NULL) ? NULL : (void*) (((char*) (ptr)) - offsetof(type, member)))) #define CONST_CONTAINER_OF(type, ptr, member) \ - ((const type*) (((ptr) == NULL) ? NULL : (const void*) (((const char*) (ptr)) -offsetof(type, member)))) + ((const type*) (((ptr) == NULL) ? NULL : (const void*) (((const char*) (ptr)) - offsetof(type, member)))) /// Used for inserting new items into AVL trees. CANARD_PRIVATE struct CanardTreeNode* avlTrivialFactory(void* const user_reference) From ee67adc57b2a9efa02bd14ed28c61a7dc04a394b Mon Sep 17 00:00:00 2001 From: Sergei Shirokov Date: Wed, 4 Dec 2024 16:07:50 +0200 Subject: [PATCH 06/14] `canardTxPush` now flushes expired frames --- libcanard/canard.c | 62 +++++++++++++++++---- libcanard/canard.h | 7 ++- tests/helpers.hpp | 14 +++-- tests/test_public_tx.cpp | 116 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 180 insertions(+), 19 deletions(-) diff --git a/libcanard/canard.c b/libcanard/canard.c index dcf9bfd..098d362 100644 --- a/libcanard/canard.c +++ b/libcanard/canard.c @@ -72,9 +72,9 @@ #define INITIAL_TOGGLE_STATE true #define CONTAINER_OF(type, ptr, member) \ - ((type*) (((ptr) == NULL) ? NULL : (void*) (((char*) (ptr)) - offsetof(type, member)))) -#define CONST_CONTAINER_OF(type, ptr, member) \ ((const type*) (((ptr) == NULL) ? NULL : (const void*) (((const char*) (ptr)) - offsetof(type, member)))) +#define MUTABLE_CONTAINER_OF(type, ptr, member) \ + ((type*) (((ptr) == NULL) ? NULL : (void*) (((char*) (ptr)) - offsetof(type, member)))) /// Used for inserting new items into AVL trees. CANARD_PRIVATE struct CanardTreeNode* avlTrivialFactory(void* const user_reference) @@ -346,8 +346,8 @@ CANARD_PRIVATE int8_t txAVLPriorityPredicate( // { typedef struct CanardTxQueueItem TxItem; - const TxItem* const target = CONST_CONTAINER_OF(TxItem, user_reference, priority_base); - const TxItem* const other = CONST_CONTAINER_OF(TxItem, node, priority_base); + const TxItem* const target = CONTAINER_OF(TxItem, user_reference, priority_base); + const TxItem* const other = CONTAINER_OF(TxItem, node, priority_base); CANARD_ASSERT((target != NULL) && (other != NULL)); return (target->frame.extended_can_id >= other->frame.extended_can_id) ? +1 : -1; } @@ -362,8 +362,8 @@ CANARD_PRIVATE int8_t txAVLDeadlinePredicate( // { typedef struct CanardTxQueueItem TxItem; - const TxItem* const target = CONST_CONTAINER_OF(TxItem, user_reference, deadline_base); - const TxItem* const other = CONST_CONTAINER_OF(TxItem, node, deadline_base); + const TxItem* const target = CONTAINER_OF(TxItem, user_reference, deadline_base); + const TxItem* const other = CONTAINER_OF(TxItem, node, deadline_base); CANARD_ASSERT((target != NULL) && (other != NULL)); return (target->tx_deadline_usec >= other->tx_deadline_usec) ? +1 : -1; } @@ -592,6 +592,35 @@ CANARD_PRIVATE int32_t txPushMultiFrame(struct CanardTxQueue* const que, return out; } +/// Flushes expired transfers by comparing deadline timestamps of the pending transfers with the current time. +CANARD_PRIVATE void txFlushExpiredTransfers(struct CanardTxQueue* const que, + const struct CanardInstance* const ins, + const CanardMicrosecond now_usec) +{ + struct CanardTxQueueItem* tx_item = NULL; + while (NULL != (tx_item = MUTABLE_CONTAINER_OF( // + struct CanardTxQueueItem, + cavlFindExtremum(que->deadline_root, false), + deadline_base))) + { + if (now_usec <= tx_item->tx_deadline_usec) + { + // The queue is sorted by deadline, so we can stop here. + break; + } + + // All frames of the transfer are released at once b/c they all have the same deadline. + struct CanardTxQueueItem* tx_item_to_free = NULL; + while (NULL != (tx_item_to_free = canardTxPop(que, tx_item))) + { + tx_item = tx_item_to_free->next_in_transfer; + canardTxFree(que, ins, tx_item_to_free); + + que->stats.dropped_frames++; + } + } +} + // --------------------------------------------- RECEPTION --------------------------------------------- #define RX_SESSIONS_PER_SUBSCRIPTION (CANARD_NODE_ID_MAX + 1U) @@ -1052,7 +1081,7 @@ rxSubscriptionPredicateOnPortID(void* const user_reference, // NOSONAR Cavl API { CANARD_ASSERT((user_reference != NULL) && (node != NULL)); const CanardPortID sought = *((const CanardPortID*) user_reference); - const CanardPortID other = CONST_CONTAINER_OF(struct CanardRxSubscription, node, base)->port_id; + const CanardPortID other = CONTAINER_OF(struct CanardRxSubscription, node, base)->port_id; static const int8_t NegPos[2] = {-1, +1}; // Clang-Tidy mistakenly identifies a narrowing cast to int8_t here, which is incorrect. return (sought == other) ? 0 : NegPos[sought > other]; // NOLINT no narrowing conversion is taking place here @@ -1063,7 +1092,7 @@ rxSubscriptionPredicateOnStruct(void* const user_reference, // NOSONAR Cavl API const struct CanardTreeNode* const node) { return rxSubscriptionPredicateOnPortID( // - &CONTAINER_OF(struct CanardRxSubscription, user_reference, base)->port_id, + &MUTABLE_CONTAINER_OF(struct CanardRxSubscription, user_reference, base)->port_id, node); } @@ -1119,6 +1148,15 @@ int32_t canardTxPush(struct CanardTxQueue* const que, const struct CanardPayload payload, const CanardMicrosecond now_usec) { + // Before pushing payload (potentially in multiple frames), we need to try to flush any expired transfers. + // This is necessary to ensure that we don't exhaust the capacity of the queue by holding outdated frames. + // The flushing is done by comparing deadline timestamps of the pending transfers with the current time, + // which makes sense only if the current time is known (bigger than zero). + if (now_usec > 0) + { + txFlushExpiredTransfers(que, ins, now_usec); + } + (void) now_usec; int32_t out = -CANARD_ERROR_INVALID_ARGUMENT; @@ -1167,7 +1205,7 @@ struct CanardTxQueueItem* canardTxPeek(const struct CanardTxQueue* const que) // Paragraph 6.7.2.1.15 of the C standard says: // A pointer to a structure object, suitably converted, points to its initial member, and vice versa. struct CanardTreeNode* const priority_node = cavlFindExtremum(que->priority_root, false); - out = CONTAINER_OF(struct CanardTxQueueItem, priority_node, priority_base); + out = MUTABLE_CONTAINER_OF(struct CanardTxQueueItem, priority_node, priority_base); } return out; } @@ -1223,7 +1261,7 @@ int8_t canardRxAccept(struct CanardInstance* const ins, // This is the reason the function has a logarithmic time complexity of the number of subscriptions. // Note also that this one of the two variable-complexity operations in the RX pipeline; the other one // is memcpy(). Excepting these two cases, the entire RX pipeline contains neither loops nor recursion. - struct CanardRxSubscription* const sub = CONTAINER_OF( // + struct CanardRxSubscription* const sub = MUTABLE_CONTAINER_OF( // struct CanardRxSubscription, cavlSearch(&ins->rx_subscriptions[(size_t) model.transfer_kind], &model.port_id, @@ -1307,7 +1345,7 @@ int8_t canardRxUnsubscribe(struct CanardInstance* const ins, { CanardPortID port_id_mutable = port_id; - struct CanardRxSubscription* const sub = CONTAINER_OF( // + struct CanardRxSubscription* const sub = MUTABLE_CONTAINER_OF( // struct CanardRxSubscription, cavlSearch(&ins->rx_subscriptions[tk], &port_id_mutable, &rxSubscriptionPredicateOnPortID, NULL), base); @@ -1348,7 +1386,7 @@ int8_t canardRxGetSubscription(struct CanardInstance* const ins, { CanardPortID port_id_mutable = port_id; - struct CanardRxSubscription* const sub = CONTAINER_OF( // + struct CanardRxSubscription* const sub = MUTABLE_CONTAINER_OF( // struct CanardRxSubscription, cavlSearch(&ins->rx_subscriptions[tk], &port_id_mutable, &rxSubscriptionPredicateOnPortID, NULL), base); diff --git a/libcanard/canard.h b/libcanard/canard.h index d76bb7c..8ef472e 100644 --- a/libcanard/canard.h +++ b/libcanard/canard.h @@ -309,7 +309,7 @@ struct CanardMemoryResource /// Holds the statistics of a transmission queue. struct CanardTxQueueStats { - /// Holds number of dropped TX frames (due to timeout when `now > deadline`). + /// Holds number of dropped TX frames due to timeout (when `now > deadline`) or b/c of transmission failures. size_t dropped_frames; }; @@ -458,8 +458,9 @@ struct CanardInstance /// The time complexity models given in the API documentation are made on the assumption that the memory management /// functions have constant complexity O(1). /// - /// The following API functions may allocate memory: canardRxAccept(), canardTxPush(). - /// The following API functions may deallocate memory: canardRxAccept(), canardRxSubscribe(), canardRxUnsubscribe(). + /// The following API functions may allocate memory: canardTxPush(), canardRxAccept(). + /// The following API functions may deallocate memory: canardTxPush(), canardTxFree(), canardRxAccept(), + /// canardRxSubscribe(), canardRxUnsubscribe(). /// The exact memory requirement and usage model is specified for each function in its documentation. struct CanardMemoryResource memory; diff --git a/tests/helpers.hpp b/tests/helpers.hpp index 322d90d..94d2c26 100644 --- a/tests/helpers.hpp +++ b/tests/helpers.hpp @@ -300,11 +300,17 @@ class TxQueue const CanardMicrosecond now_usec = 0ULL) { checkInvariants(); - const auto size_before = que_.size; - const auto ret = canardTxPush(&que_, ins, transmission_deadline_usec, &metadata, payload, now_usec); - const auto num_added = static_cast(ret); - enforce((ret < 0) || ((size_before + num_added) == que_.size), "Unexpected size change after push"); + + const auto size_before = que_.size; + const auto dropped_before = que_.stats.dropped_frames; + + const auto ret = canardTxPush(&que_, ins, transmission_deadline_usec, &metadata, payload, now_usec); + const auto num_added = static_cast(ret); + + enforce((ret < 0) || ((size_before + num_added + dropped_before - que_.stats.dropped_frames) == que_.size), + "Unexpected size change after push"); checkInvariants(); + return ret; } diff --git a/tests/test_public_tx.cpp b/tests/test_public_tx.cpp index c852a2d..c45ab03 100644 --- a/tests/test_public_tx.cpp +++ b/tests/test_public_tx.cpp @@ -814,3 +814,119 @@ TEST_CASE("TxPayloadOwnership") } } } + +TEST_CASE("TxFlushExpired") +{ + helpers::Instance ins; + helpers::TxQueue que{2, CANARD_MTU_CAN_FD}; // Limit capacity at 2 frames. + + auto& tx_alloc = que.getAllocator(); + auto& ins_alloc = ins.getAllocator(); + + std::array payload{}; + std::iota(payload.begin(), payload.end(), 0U); + + REQUIRE(CANARD_NODE_ID_UNSET == ins.getNodeID()); + REQUIRE(CANARD_MTU_CAN_FD == que.getMTU()); + REQUIRE(0 == que.getSize()); + REQUIRE(0 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(0 == ins_alloc.getNumAllocatedFragments()); + + CanardMicrosecond now = 10'000'000ULL; // 10s + const CanardMicrosecond deadline = 1'000'000ULL; // 1s + + CanardTransferMetadata meta{}; + + // 1. Push single-frame with padding, peek. @ 10s + { + meta.priority = CanardPriorityNominal; + meta.transfer_kind = CanardTransferKindMessage; + meta.port_id = 321; + meta.remote_node_id = CANARD_NODE_ID_UNSET; + meta.transfer_id = 21; + REQUIRE(1 == que.push(&ins.getInstance(), now + deadline, meta, {8, payload.data()}, now)); + REQUIRE(1 == que.getSize()); + REQUIRE(1 == tx_alloc.getNumAllocatedFragments()); + REQUIRE((8 + 4) == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(1 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(sizeof(CanardTxQueueItem) * 1 == ins_alloc.getTotalAllocatedAmount()); + + // Peek and check the payload. + CanardTxQueueItem* ti = que.peek(); + REQUIRE(nullptr != ti); // Make sure we get the same frame again. + REQUIRE(ti->frame.payload.size == 12); + REQUIRE(ti->frame.payload.allocated_size == 12); + REQUIRE(0 == std::memcmp(ti->frame.payload.data, payload.data(), 8)); + REQUIRE(ti->tx_deadline_usec == now + deadline); + REQUIRE(1 == tx_alloc.getNumAllocatedFragments()); + REQUIRE((8 + 4) == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(1 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(sizeof(CanardTxQueueItem) * 1 == ins_alloc.getTotalAllocatedAmount()); + + // Don't pop and free the item - we gonna flush it by the next push at 12s. + } + + now += 2 * deadline; // 10s -> 12s + + // 2. Push two-frames, peek. @ 12s (after 2x deadline) + // These 2 frames should still fit into the queue (with capacity 2) despite one expired frame still there.` + { + que.setMTU(8); + ins.setNodeID(42); + meta.transfer_id = 22; + REQUIRE(2 == que.push(&ins.getInstance(), now + deadline, meta, {8, payload.data()}, now)); + REQUIRE(2 == que.getSize()); + REQUIRE(2 == tx_alloc.getNumAllocatedFragments()); + REQUIRE((8 + 4) == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(2 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(sizeof(CanardTxQueueItem) * 2 == ins_alloc.getTotalAllocatedAmount()); + REQUIRE(1 == que.getInstance().stats.dropped_frames); + + // a) Peek and check the payload of the 1st frame + CanardTxQueueItem* ti = NULL; + { + ti = que.peek(); + REQUIRE(nullptr != ti); + REQUIRE(ti->frame.payload.size == 8); + REQUIRE(ti->frame.payload.allocated_size == 8); + REQUIRE(0 == std::memcmp(ti->frame.payload.data, payload.data(), 7)); + REQUIRE(ti->tx_deadline_usec == now + deadline); + REQUIRE(2 == tx_alloc.getNumAllocatedFragments()); + REQUIRE((8 + 4) == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(2 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(sizeof(CanardTxQueueItem) * 2 == ins_alloc.getTotalAllocatedAmount()); + + // Don't pop and free the item - we gonna flush it by the next push @ 14s. + } + // b) Check the payload of the 2nd frame + { + ti = ti->next_in_transfer; + REQUIRE(nullptr != ti); + REQUIRE(ti->frame.payload.size == 4); + REQUIRE(ti->frame.payload.allocated_size == 4); + REQUIRE(0 == std::memcmp(ti->frame.payload.data, payload.data() + 7, 1)); + REQUIRE(ti->tx_deadline_usec == now + deadline); + + // Don't pop and free the item - we gonna flush it by the next push @ 14s. + } + } + + now += 2 * deadline; // 12s -> 14s + + // 3. Push three-frames, peek. @ 14s (after another 2x deadline) + // These 3 frames should not fit into the queue (with capacity 2), + // but as a side effect, the expired frames (from push @ 12s) should be flushed as well. + { + meta.transfer_id = 23; + REQUIRE(-CANARD_ERROR_OUT_OF_MEMORY == + que.push(&ins.getInstance(), now + deadline, meta, {8 * 2, payload.data()}, now)); + REQUIRE(0 == que.getSize()); + REQUIRE(0 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(0 == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(0 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(0 == ins_alloc.getTotalAllocatedAmount()); + REQUIRE(1 + 2 == que.getInstance().stats.dropped_frames); + + REQUIRE(nullptr == que.peek()); + } +} From 78b62fa05b8878f3c79f753795bc0774c4a4e2a5 Mon Sep 17 00:00:00 2001 From: Sergei Shirokov Date: Wed, 4 Dec 2024 17:37:46 +0200 Subject: [PATCH 07/14] build fix --- tests/test_public_tx.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/test_public_tx.cpp b/tests/test_public_tx.cpp index c45ab03..3047cfd 100644 --- a/tests/test_public_tx.cpp +++ b/tests/test_public_tx.cpp @@ -883,7 +883,7 @@ TEST_CASE("TxFlushExpired") REQUIRE(1 == que.getInstance().stats.dropped_frames); // a) Peek and check the payload of the 1st frame - CanardTxQueueItem* ti = NULL; + CanardTxQueueItem* ti = nullptr; { ti = que.peek(); REQUIRE(nullptr != ti); @@ -919,7 +919,7 @@ TEST_CASE("TxFlushExpired") { meta.transfer_id = 23; REQUIRE(-CANARD_ERROR_OUT_OF_MEMORY == - que.push(&ins.getInstance(), now + deadline, meta, {8 * 2, payload.data()}, now)); + que.push(&ins.getInstance(), now + deadline, meta, {8ULL * 2ULL, payload.data()}, now)); REQUIRE(0 == que.getSize()); REQUIRE(0 == tx_alloc.getNumAllocatedFragments()); REQUIRE(0 == tx_alloc.getTotalAllocatedAmount()); From df974af8b7ed49c137b019bd1d9cc408ff974114 Mon Sep 17 00:00:00 2001 From: Sergei Shirokov Date: Wed, 4 Dec 2024 18:45:05 +0200 Subject: [PATCH 08/14] canardTxPoll --- libcanard/canard.c | 74 ++++++++++++++++++++++++++++++++++++++++------ libcanard/canard.h | 48 ++++++++++++++++++++++++++++++ 2 files changed, 113 insertions(+), 9 deletions(-) diff --git a/libcanard/canard.c b/libcanard/canard.c index 098d362..b17e5ca 100644 --- a/libcanard/canard.c +++ b/libcanard/canard.c @@ -592,6 +592,25 @@ CANARD_PRIVATE int32_t txPushMultiFrame(struct CanardTxQueue* const que, return out; } +CANARD_PRIVATE void txPopAndFreeTransfer(struct CanardTxQueue* const que, + const struct CanardInstance* const ins, + struct CanardTxQueueItem* tx_item, + const bool drop_whole_transfer) +{ + struct CanardTxQueueItem* tx_item_to_free = NULL; + while (NULL != (tx_item_to_free = canardTxPop(que, tx_item))) + { + tx_item = tx_item_to_free->next_in_transfer; + canardTxFree(que, ins, tx_item_to_free); + + if (!drop_whole_transfer) + { + break; + } + que->stats.dropped_frames++; + } +} + /// Flushes expired transfers by comparing deadline timestamps of the pending transfers with the current time. CANARD_PRIVATE void txFlushExpiredTransfers(struct CanardTxQueue* const que, const struct CanardInstance* const ins, @@ -609,15 +628,8 @@ CANARD_PRIVATE void txFlushExpiredTransfers(struct CanardTxQueue* const q break; } - // All frames of the transfer are released at once b/c they all have the same deadline. - struct CanardTxQueueItem* tx_item_to_free = NULL; - while (NULL != (tx_item_to_free = canardTxPop(que, tx_item))) - { - tx_item = tx_item_to_free->next_in_transfer; - canardTxFree(que, ins, tx_item_to_free); - - que->stats.dropped_frames++; - } + // All frames of the transfer are dropped at once b/c they all have the same deadline. + txPopAndFreeTransfer(que, ins, tx_item, true); // drop the whole transfer } } @@ -1242,6 +1254,50 @@ void canardTxFree(struct CanardTxQueue* const que, } } +int8_t canardTxPoll(struct CanardTxQueue* const que, + const struct CanardInstance* const ins, + const CanardMicrosecond now_usec, + void* const user_reference, + const CanardTxFrameHandler frame_handler) +{ + int8_t out = 0; + + // Before peeking a frame to transmit, we need to try to flush any expired transfers. + // This will not only ensure asap freeing of the queue capacity, but also makes sure that the following + // `canardTxPeek` will return a not expired item (if any), so we don't need to check the deadline again. + // The flushing is done by comparing deadline timestamps of the pending transfers with the current time, + // which makes sense only if the current time is known (bigger than zero). + if (now_usec > 0) + { + txFlushExpiredTransfers(que, ins, now_usec); + } + + if (frame_handler != NULL) + { + struct CanardTxQueueItem* const tx_item = canardTxPeek(que); + if (tx_item != NULL) + { + // No need to check the deadline again, as we have already flushed all expired transfers. + out = frame_handler(user_reference, tx_item->tx_deadline_usec, &tx_item->frame); + + // We gonna release (pop and free) the frame if the handler returned: + // - either a positive value - the frame has been successfully accepted by the handler; + // - or a negative value - the frame has been rejected by the handler due to failure. + // Zero value means that the handler cannot accept the frame at the moment, so we keep it in the queue. + if (out != 0) + { + // In case of a failure, it makes sense to drop the whole transfer immediately + // b/c at least one this frame has been rejected, so the whole transfer is useless. + const bool drop_whole_transfer = (out < 0); + txPopAndFreeTransfer(que, ins, tx_item, drop_whole_transfer); + } + } + } + + CANARD_ASSERT(out <= 1); + return out; +} + int8_t canardRxAccept(struct CanardInstance* const ins, const CanardMicrosecond timestamp_usec, const struct CanardFrame* const frame, diff --git a/libcanard/canard.h b/libcanard/canard.h index 8ef472e..905e74e 100644 --- a/libcanard/canard.h +++ b/libcanard/canard.h @@ -313,6 +313,29 @@ struct CanardTxQueueStats size_t dropped_frames; }; +/// Defines the signature of the TX frame handler function. +/// +/// The handler function is intended to be invoked from Canard TX polling (see details for the `canardTxPoll()`). +/// +/// @param user_reference The user reference passed to `canardTxPoll()`. +/// @param deadline_usec The deadline of the frame that is being handled. +/// @param frame The mutable frame that is being handled. +/// @return The result of the handling operation: +/// - Any positive value: the frame was successfully handled. +/// This indicates that the frame payload was accepted (and its ownership could be potentially moved, +/// see `canardTxPeek` for the details), and the frame can be safely removed from the queue. +/// - Zero: the frame was not handled, and so the frame should be kept in the queue. +/// It will be retried on some future `canardTxPoll()` call according to the queue state in the future. +/// This case is useful when TX hardware is busy, and the frame should be retried later. +/// - Any negative value: the frame was rejected due to an unrecoverable failure. +/// This indicates to the caller (`canardTxPoll`) that the frame should be dropped from the queue, +/// as well as all other frames belonging to the same transfer. The `dropped_frames` counter in the TX queue stats +/// will be incremented for each frame dropped in this way. +/// +typedef int8_t (*CanardTxFrameHandler)(void* const user_reference, + const CanardMicrosecond deadline_usec, + struct CanardMutableFrame* frame); + /// Prioritized transmission queue that keeps CAN frames destined for transmission via one CAN interface. /// Applications with redundant interfaces are expected to have one instance of this type per interface. /// Applications that are not interested in transmission may have zero queues. @@ -613,6 +636,31 @@ void canardTxFree(struct CanardTxQueue* const que, const struct CanardInstance* const ins, struct CanardTxQueueItem* const item); +/// This is a helper that combines several Canard TX calls (`canardTxPeek`, `canardTxPop` and `canardTxFree`) +/// into one "polling" algorythm. It simplifies the whole process of transmitting frames to just two function calls: +/// - `canardTxPush` to enqueue the frames +/// - `canardTxPoll` to dequeue, transmit and free a single frame +/// +/// The algorythm implements a typical pattern of de-queuing, transmitting and freeing a TX queue item, +/// as well as handling transmission failures, retries, and deadline timeouts. +/// +/// The function is intended to be periodically called, most probably on a signal that the previous TX frame +/// transmission has been completed, and so the next TX frame (if any) could be polled from the TX queue. +/// +/// @param que The TX queue to poll. +/// @param ins The Canard instance. +/// @param now_usec The current time in microseconds. It is used to determine if the frame has timed out. +/// @param user_reference The user reference to be passed to the frame handler. +/// @param frame_handler The frame handler function that will be called to transmit the frame. +/// @return Zero if the queue is empty or there is no frame handler (NULL). +/// Otherwise, the result from the frame handler call. See `CanardTxFrameHandler` documentation. +/// +int8_t canardTxPoll(struct CanardTxQueue* const que, + const struct CanardInstance* const ins, + const CanardMicrosecond now_usec, + void* const user_reference, + const CanardTxFrameHandler frame_handler); + /// This function implements the transfer reassembly logic. It accepts a transport frame from any of the redundant /// interfaces, locates the appropriate subscription state, and, if found, updates it. If the frame completed a /// transfer, the return value is 1 (one) and the out_transfer pointer is populated with the parameters of the From 18ea9fe3a3bb44d33235115c92434fd16704cc58 Mon Sep 17 00:00:00 2001 From: Sergei Date: Wed, 4 Dec 2024 23:32:43 +0200 Subject: [PATCH 09/14] minor fixes #sonar --- libcanard/canard.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/libcanard/canard.c b/libcanard/canard.c index b17e5ca..6b5498d 100644 --- a/libcanard/canard.c +++ b/libcanard/canard.c @@ -1169,8 +1169,6 @@ int32_t canardTxPush(struct CanardTxQueue* const que, txFlushExpiredTransfers(que, ins, now_usec); } - (void) now_usec; - int32_t out = -CANARD_ERROR_INVALID_ARGUMENT; if ((ins != NULL) && (que != NULL) && (metadata != NULL) && ((payload.data != NULL) || (0U == payload.size))) { @@ -1263,7 +1261,7 @@ int8_t canardTxPoll(struct CanardTxQueue* const que, int8_t out = 0; // Before peeking a frame to transmit, we need to try to flush any expired transfers. - // This will not only ensure asap freeing of the queue capacity, but also makes sure that the following + // This will not only ensure ASAP freeing of the queue capacity, but also makes sure that the following // `canardTxPeek` will return a not expired item (if any), so we don't need to check the deadline again. // The flushing is done by comparing deadline timestamps of the pending transfers with the current time, // which makes sense only if the current time is known (bigger than zero). @@ -1282,12 +1280,12 @@ int8_t canardTxPoll(struct CanardTxQueue* const que, // We gonna release (pop and free) the frame if the handler returned: // - either a positive value - the frame has been successfully accepted by the handler; - // - or a negative value - the frame has been rejected by the handler due to failure. + // - or a negative value - the frame has been rejected by the handler due to a failure. // Zero value means that the handler cannot accept the frame at the moment, so we keep it in the queue. if (out != 0) { // In case of a failure, it makes sense to drop the whole transfer immediately - // b/c at least one this frame has been rejected, so the whole transfer is useless. + // b/c at least this frame has been rejected, so the whole transfer is useless. const bool drop_whole_transfer = (out < 0); txPopAndFreeTransfer(que, ins, tx_item, drop_whole_transfer); } From 2d834f8cced19401254f45657060dd7e9a246d18 Mon Sep 17 00:00:00 2001 From: Sergei Date: Thu, 5 Dec 2024 01:34:46 +0200 Subject: [PATCH 10/14] unit tests #sonar --- libcanard/canard.c | 7 +- tests/helpers.hpp | 30 +++- tests/test_public_tx.cpp | 323 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 353 insertions(+), 7 deletions(-) diff --git a/libcanard/canard.c b/libcanard/canard.c index 6b5498d..3ec49d9 100644 --- a/libcanard/canard.c +++ b/libcanard/canard.c @@ -594,13 +594,14 @@ CANARD_PRIVATE int32_t txPushMultiFrame(struct CanardTxQueue* const que, CANARD_PRIVATE void txPopAndFreeTransfer(struct CanardTxQueue* const que, const struct CanardInstance* const ins, - struct CanardTxQueueItem* tx_item, + struct CanardTxQueueItem* const tx_item, const bool drop_whole_transfer) { + struct CanardTxQueueItem* curr_tx_item = tx_item; struct CanardTxQueueItem* tx_item_to_free = NULL; - while (NULL != (tx_item_to_free = canardTxPop(que, tx_item))) + while (NULL != (tx_item_to_free = canardTxPop(que, curr_tx_item))) { - tx_item = tx_item_to_free->next_in_transfer; + curr_tx_item = tx_item_to_free->next_in_transfer; canardTxFree(que, ins, tx_item_to_free); if (!drop_whole_transfer) diff --git a/tests/helpers.hpp b/tests/helpers.hpp index 94d2c26..a296415 100644 --- a/tests/helpers.hpp +++ b/tests/helpers.hpp @@ -9,10 +9,14 @@ #include #include #include +#include #include #include +#include +#include #include -#include +#include +#include #include #include @@ -87,7 +91,7 @@ class TestAllocator std::uint8_t* p = nullptr; if ((amount > 0U) && ((getTotalAllocatedAmount() + amount) <= ceiling_)) { - const auto amount_with_canaries = amount + canary_.size() * 2U; + const auto amount_with_canaries = amount + (canary_.size() * 2U); // Clang-tidy complains about manual memory management. Suppressed because we need it for testing purposes. p = static_cast(std::malloc(amount_with_canaries)); // NOLINT if (p == nullptr) @@ -127,7 +131,7 @@ class TestAllocator std::to_string(reinterpret_cast(user_pointer))); } std::generate_n(p - canary_.size(), // Damage the memory to make sure it's not used after deallocation. - amount + canary_.size() * 2U, + amount + (canary_.size() * 2U), []() { return getRandomNatural(256U); }); std::free(p - canary_.size()); // NOLINT we require manual memory management here. allocated_.erase(it); @@ -345,6 +349,26 @@ class TxQueue void freeItem(Instance& ins, CanardTxQueueItem* const item) { canardTxFree(&que_, &ins.getInstance(), item); } + using FrameHandler = std::function; + + [[nodiscard]] auto poll(Instance& ins, const CanardMicrosecond now_usec, FrameHandler frame_handler) + { + if (!frame_handler) + { + return canardTxPoll(&que_, &ins.getInstance(), now_usec, nullptr, nullptr); + } + + return canardTxPoll(&que_, + &ins.getInstance(), + now_usec, + &frame_handler, + [](auto* user_reference, const auto deadline_usec, auto* frame) -> std::int8_t { + // + const auto* const handler_ptr = static_cast(user_reference); + return (*handler_ptr)(deadline_usec, *frame); + }); + } + [[nodiscard]] auto getSize() const { std::size_t out = 0; diff --git a/tests/test_public_tx.cpp b/tests/test_public_tx.cpp index 3047cfd..eb9aad0 100644 --- a/tests/test_public_tx.cpp +++ b/tests/test_public_tx.cpp @@ -815,7 +815,7 @@ TEST_CASE("TxPayloadOwnership") } } -TEST_CASE("TxFlushExpired") +TEST_CASE("TxPushFlushExpired") { helpers::Instance ins; helpers::TxQueue que{2, CANARD_MTU_CAN_FD}; // Limit capacity at 2 frames. @@ -930,3 +930,324 @@ TEST_CASE("TxFlushExpired") REQUIRE(nullptr == que.peek()); } } + +TEST_CASE("TxPollSingleFrame") +{ + helpers::Instance ins; + helpers::TxQueue que{2, CANARD_MTU_CAN_FD}; // Limit capacity at 2 frames. + + que.setMTU(8); + ins.setNodeID(42); + + auto& tx_alloc = que.getAllocator(); + auto& ins_alloc = ins.getAllocator(); + + std::array payload{}; + std::iota(payload.begin(), payload.end(), 0U); + + REQUIRE(42 == ins.getNodeID()); + REQUIRE(CANARD_MTU_CAN_CLASSIC == que.getMTU()); + REQUIRE(0 == que.getSize()); + REQUIRE(0 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(0 == ins_alloc.getNumAllocatedFragments()); + + CanardMicrosecond now = 10'000'000ULL; // 10s + constexpr CanardMicrosecond deadline = 1'000'000ULL; // 1s + + CanardTransferMetadata meta{}; + + // 1. Push single frame @ 10s + // + meta.priority = CanardPriorityNominal; + meta.transfer_kind = CanardTransferKindMessage; + meta.port_id = 321; + meta.remote_node_id = CANARD_NODE_ID_UNSET; + meta.transfer_id = 21; + REQUIRE(1 == que.push(&ins.getInstance(), now + deadline, meta, {7, payload.data()}, now)); + REQUIRE(1 == que.getSize()); + REQUIRE(1 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(8 == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(1 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(sizeof(CanardTxQueueItem) * 1 == ins_alloc.getTotalAllocatedAmount()); + + // 2. Poll; emulate media is busy @ 10s + 100us + // + std::size_t total_handler_calls = 0; + REQUIRE(0 == que.poll(ins, now + 100, [&](auto deadline_usec, auto& frame) -> std::int8_t { + // + ++total_handler_calls; + REQUIRE(deadline_usec == now + deadline); + REQUIRE(frame.payload.size == 8); + REQUIRE(frame.payload.allocated_size == 8); + REQUIRE(0 == std::memcmp(frame.payload.data, payload.data(), 7)); + return 0; // Emulate that TX media is busy. + })); + REQUIRE(1 == total_handler_calls); + REQUIRE(1 == que.getSize()); + REQUIRE(1 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(8 == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(1 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(sizeof(CanardTxQueueItem) * 1 == ins_alloc.getTotalAllocatedAmount()); + REQUIRE(0 == que.getInstance().stats.dropped_frames); + + // 3. Poll; emulate media is ready @ 10s + 200us + // + REQUIRE(1 == que.poll(ins, now + 200, [&](auto deadline_usec, auto& frame) -> std::int8_t { + // + ++total_handler_calls; + REQUIRE(deadline_usec == now + deadline); + REQUIRE(frame.payload.size == 8); + REQUIRE(frame.payload.allocated_size == 8); + REQUIRE(0 == std::memcmp(frame.payload.data, payload.data(), 7)); + return 1; // Emulate that TX media accepted the frame. + })); + REQUIRE(2 == total_handler_calls); + REQUIRE(0 == que.getSize()); + REQUIRE(0 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(0 == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(0 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(sizeof(CanardTxQueueItem) * 0 == ins_alloc.getTotalAllocatedAmount()); + REQUIRE(0 == que.getInstance().stats.dropped_frames); + + // 3. Poll when queue is empty @ 10s + 300us + // + REQUIRE(0 == que.poll(ins, now + 300, [&](auto, auto&) -> std::int8_t { + // + ++total_handler_calls; + FAIL("This should not be called."); + return -1; + })); + REQUIRE(2 == total_handler_calls); + REQUIRE(0 == que.getSize()); +} + +TEST_CASE("TxPollMultiFrame") +{ + helpers::Instance ins; + helpers::TxQueue que{2, CANARD_MTU_CAN_FD}; // Limit capacity at 2 frames. + + que.setMTU(8); + ins.setNodeID(42); + + auto& tx_alloc = que.getAllocator(); + auto& ins_alloc = ins.getAllocator(); + + std::array payload{}; + std::iota(payload.begin(), payload.end(), 0U); + + REQUIRE(42 == ins.getNodeID()); + REQUIRE(CANARD_MTU_CAN_CLASSIC == que.getMTU()); + REQUIRE(0 == que.getSize()); + REQUIRE(0 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(0 == ins_alloc.getNumAllocatedFragments()); + + CanardMicrosecond now = 10'000'000ULL; // 10s + constexpr CanardMicrosecond deadline = 1'000'000ULL; // 1s + + CanardTransferMetadata meta{}; + + // 1. Push two frames @ 10s + // + meta.priority = CanardPriorityNominal; + meta.transfer_kind = CanardTransferKindMessage; + meta.port_id = 321; + meta.remote_node_id = CANARD_NODE_ID_UNSET; + meta.transfer_id = 21; + REQUIRE(2 == que.push(&ins.getInstance(), now + deadline, meta, {8, payload.data()}, now)); + REQUIRE(2 == que.getSize()); + REQUIRE(2 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(8 + 4 == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(2 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(sizeof(CanardTxQueueItem) * 2 == ins_alloc.getTotalAllocatedAmount()); + + // 2. Poll 1st frame @ 10s + 100us + // + std::size_t total_handler_calls = 0; + REQUIRE(1 == que.poll(ins, now + 100, [&](auto deadline_usec, auto& frame) -> std::int8_t { + // + ++total_handler_calls; + REQUIRE(deadline_usec == now + deadline); + REQUIRE(frame.payload.size == 8); + REQUIRE(frame.payload.allocated_size == 8); + REQUIRE(0 == std::memcmp(frame.payload.data, payload.data(), 7)); + return 1; + })); + REQUIRE(1 == total_handler_calls); + REQUIRE(1 == que.getSize()); + REQUIRE(1 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(4 == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(1 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(sizeof(CanardTxQueueItem) * 1 == ins_alloc.getTotalAllocatedAmount()); + REQUIRE(0 == que.getInstance().stats.dropped_frames); + + // 3. Poll 2nd frame @ 10s + 200us + // + REQUIRE(1 == que.poll(ins, now + 200, [&](auto deadline_usec, auto& frame) -> std::int8_t { + // + ++total_handler_calls; + REQUIRE(deadline_usec == now + deadline); + REQUIRE(frame.payload.size == 4); + REQUIRE(frame.payload.allocated_size == 4); + REQUIRE(0 == std::memcmp(frame.payload.data, payload.data() + 7, 1)); + return 1; + })); + REQUIRE(2 == total_handler_calls); + REQUIRE(0 == que.getSize()); + REQUIRE(0 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(0 == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(0 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(sizeof(CanardTxQueueItem) * 0 == ins_alloc.getTotalAllocatedAmount()); + REQUIRE(0 == que.getInstance().stats.dropped_frames); +} + +TEST_CASE("TxPollDropFrameOnFailure") +{ + helpers::Instance ins; + helpers::TxQueue que{2, CANARD_MTU_CAN_FD}; // Limit capacity at 2 frames. + + que.setMTU(8); + ins.setNodeID(42); + + auto& tx_alloc = que.getAllocator(); + auto& ins_alloc = ins.getAllocator(); + + std::array payload{}; + std::iota(payload.begin(), payload.end(), 0U); + + REQUIRE(42 == ins.getNodeID()); + REQUIRE(CANARD_MTU_CAN_CLASSIC == que.getMTU()); + REQUIRE(0 == que.getSize()); + REQUIRE(0 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(0 == ins_alloc.getNumAllocatedFragments()); + + constexpr CanardMicrosecond now = 10'000'000ULL; // 10s + constexpr CanardMicrosecond deadline = 1'000'000ULL; // 1s + + CanardTransferMetadata meta{}; + + // 1. Push two frames @ 10s + // + meta.priority = CanardPriorityNominal; + meta.transfer_kind = CanardTransferKindMessage; + meta.port_id = 321; + meta.remote_node_id = CANARD_NODE_ID_UNSET; + meta.transfer_id = 21; + REQUIRE(2 == que.push(&ins.getInstance(), now + deadline, meta, {8, payload.data()}, now)); + REQUIRE(2 == que.getSize()); + REQUIRE(2 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(8 + 4 == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(2 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(sizeof(CanardTxQueueItem) * 2 == ins_alloc.getTotalAllocatedAmount()); + + // 2. Poll 1st frame; emulate media failure @ 10s + 100us + // + std::size_t total_handler_calls = 0; + REQUIRE(-1 == que.poll(ins, now + 100, [&](auto deadline_usec, auto& frame) -> std::int8_t { + // + ++total_handler_calls; + REQUIRE(deadline_usec == now + deadline); + REQUIRE(frame.payload.size == 8); + REQUIRE(frame.payload.allocated_size == 8); + REQUIRE(0 == std::memcmp(frame.payload.data, payload.data(), 7)); + return -1; + })); + REQUIRE(1 == total_handler_calls); + REQUIRE(0 == que.getSize()); + REQUIRE(0 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(0 == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(0 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(sizeof(CanardTxQueueItem) * 0 == ins_alloc.getTotalAllocatedAmount()); + REQUIRE(2 == que.getInstance().stats.dropped_frames); +} + +TEST_CASE("TxPollDropExpired") +{ + helpers::Instance ins; + helpers::TxQueue que{2, CANARD_MTU_CAN_FD}; // Limit capacity at 2 frames. + + que.setMTU(8); + ins.setNodeID(42); + + auto& tx_alloc = que.getAllocator(); + auto& ins_alloc = ins.getAllocator(); + + std::array payload{}; + std::iota(payload.begin(), payload.end(), 0U); + + REQUIRE(42 == ins.getNodeID()); + REQUIRE(CANARD_MTU_CAN_CLASSIC == que.getMTU()); + REQUIRE(0 == que.getSize()); + REQUIRE(0 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(0 == ins_alloc.getNumAllocatedFragments()); + + CanardMicrosecond now = 10'000'000ULL; // 10s + constexpr CanardMicrosecond deadline = 1'000'000ULL; // 1s + + CanardTransferMetadata meta{}; + + // 1. Push nominal priority frame @ 10s + // + meta.priority = CanardPriorityNominal; + meta.transfer_kind = CanardTransferKindMessage; + meta.port_id = 321; + meta.remote_node_id = CANARD_NODE_ID_UNSET; + meta.transfer_id = 21; + REQUIRE(1 == que.push(&ins.getInstance(), now + deadline, meta, {7, payload.data()}, now)); + REQUIRE(1 == que.getSize()); + REQUIRE(1 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(8 == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(1 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(sizeof(CanardTxQueueItem) * 1 == ins_alloc.getTotalAllocatedAmount()); + + // 2. Push high priority frame @ 10s + 1'000us + // + meta.priority = CanardPriorityHigh; + meta.transfer_kind = CanardTransferKindMessage; + meta.port_id = 321; + meta.transfer_id = 22; + REQUIRE(1 == que.push(&ins.getInstance(), now + deadline - 1, meta, {7, payload.data() + 100}, now + 1'000)); + REQUIRE(2 == que.getSize()); + REQUIRE(2 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(8 + 8 == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(2 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(sizeof(CanardTxQueueItem) * 2 == ins_alloc.getTotalAllocatedAmount()); + + // 3. Poll a frame (should be the high priority one); emulate media is busy @ 10s + 2'000us + // + std::size_t total_handler_calls = 0; + REQUIRE(0 == que.poll(ins, now + 2'000, [&](auto deadline_usec, auto& frame) -> std::int8_t { + // + ++total_handler_calls; + REQUIRE(deadline_usec == now + deadline - 1); + REQUIRE(frame.payload.size == 8); + REQUIRE(frame.payload.allocated_size == 8); + REQUIRE(0 == std::memcmp(frame.payload.data, payload.data() + 100, 7)); + return 0; + })); + REQUIRE(1 == total_handler_calls); + REQUIRE(2 == que.getSize()); + REQUIRE(2 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(8 + 8 == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(2 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(sizeof(CanardTxQueueItem) * 2 == ins_alloc.getTotalAllocatedAmount()); + REQUIRE(0 == que.getInstance().stats.dropped_frames); + + // 3. Poll a frame (should be nominal priority one b/c the high has been expired) @ 10s + deadline + // + REQUIRE(1 == que.poll(ins, now + deadline, [&](auto deadline_usec, auto& frame) -> std::int8_t { + // + ++total_handler_calls; + REQUIRE(deadline_usec == now + deadline); + REQUIRE(frame.payload.size == 8); + REQUIRE(frame.payload.allocated_size == 8); + REQUIRE(0 == std::memcmp(frame.payload.data, payload.data(), 7)); + return 1; + })); + REQUIRE(2 == total_handler_calls); + REQUIRE(0 == que.getSize()); + REQUIRE(0 == tx_alloc.getNumAllocatedFragments()); + REQUIRE(0 == tx_alloc.getTotalAllocatedAmount()); + REQUIRE(0 == ins_alloc.getNumAllocatedFragments()); + REQUIRE(sizeof(CanardTxQueueItem) * 0 == ins_alloc.getTotalAllocatedAmount()); + REQUIRE(1 == que.getInstance().stats.dropped_frames); +} From e1147fa96b474fed12f75cf7dd8e8bdf83b5abb5 Mon Sep 17 00:00:00 2001 From: Sergei Date: Thu, 5 Dec 2024 01:49:09 +0200 Subject: [PATCH 11/14] more coverage #sonar --- tests/test_public_tx.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/tests/test_public_tx.cpp b/tests/test_public_tx.cpp index eb9aad0..b7cbed8 100644 --- a/tests/test_public_tx.cpp +++ b/tests/test_public_tx.cpp @@ -970,7 +970,11 @@ TEST_CASE("TxPollSingleFrame") REQUIRE(1 == ins_alloc.getNumAllocatedFragments()); REQUIRE(sizeof(CanardTxQueueItem) * 1 == ins_alloc.getTotalAllocatedAmount()); - // 2. Poll; emulate media is busy @ 10s + 100us + // 2. Poll without time and handler. + // + REQUIRE(0 == que.poll(ins, 0, nullptr)); + + // 3. Poll; emulate media is busy @ 10s + 100us // std::size_t total_handler_calls = 0; REQUIRE(0 == que.poll(ins, now + 100, [&](auto deadline_usec, auto& frame) -> std::int8_t { @@ -990,7 +994,7 @@ TEST_CASE("TxPollSingleFrame") REQUIRE(sizeof(CanardTxQueueItem) * 1 == ins_alloc.getTotalAllocatedAmount()); REQUIRE(0 == que.getInstance().stats.dropped_frames); - // 3. Poll; emulate media is ready @ 10s + 200us + // 4. Poll; emulate media is ready @ 10s + 200us // REQUIRE(1 == que.poll(ins, now + 200, [&](auto deadline_usec, auto& frame) -> std::int8_t { // From 2b5bfae00f40e779374f8c4670a8ae9c1fe75d9c Mon Sep 17 00:00:00 2001 From: Sergei Shirokov Date: Thu, 5 Dec 2024 15:09:22 +0200 Subject: [PATCH 12/14] more docs; previous pr review fixes --- libcanard/canard.c | 27 ++++++++++++++++----------- libcanard/canard.h | 12 +++++++++--- 2 files changed, 25 insertions(+), 14 deletions(-) diff --git a/libcanard/canard.c b/libcanard/canard.c index 3ec49d9..aff91b3 100644 --- a/libcanard/canard.c +++ b/libcanard/canard.c @@ -597,11 +597,11 @@ CANARD_PRIVATE void txPopAndFreeTransfer(struct CanardTxQueue* const que, struct CanardTxQueueItem* const tx_item, const bool drop_whole_transfer) { - struct CanardTxQueueItem* curr_tx_item = tx_item; - struct CanardTxQueueItem* tx_item_to_free = NULL; - while (NULL != (tx_item_to_free = canardTxPop(que, curr_tx_item))) + struct CanardTxQueueItem* next_tx_item = tx_item; + struct CanardTxQueueItem* tx_item_to_free = canardTxPop(que, next_tx_item); + while (NULL != tx_item_to_free) { - curr_tx_item = tx_item_to_free->next_in_transfer; + next_tx_item = tx_item_to_free->next_in_transfer; canardTxFree(que, ins, tx_item_to_free); if (!drop_whole_transfer) @@ -609,6 +609,8 @@ CANARD_PRIVATE void txPopAndFreeTransfer(struct CanardTxQueue* const que, break; } que->stats.dropped_frames++; + + tx_item_to_free = canardTxPop(que, next_tx_item); } } @@ -617,11 +619,11 @@ CANARD_PRIVATE void txFlushExpiredTransfers(struct CanardTxQueue* const q const struct CanardInstance* const ins, const CanardMicrosecond now_usec) { - struct CanardTxQueueItem* tx_item = NULL; - while (NULL != (tx_item = MUTABLE_CONTAINER_OF( // - struct CanardTxQueueItem, - cavlFindExtremum(que->deadline_root, false), - deadline_base))) + struct CanardTxQueueItem* tx_item = MUTABLE_CONTAINER_OF( // + struct CanardTxQueueItem, + cavlFindExtremum(que->deadline_root, false), + deadline_base); + while (NULL != tx_item) { if (now_usec <= tx_item->tx_deadline_usec) { @@ -631,6 +633,11 @@ CANARD_PRIVATE void txFlushExpiredTransfers(struct CanardTxQueue* const q // All frames of the transfer are dropped at once b/c they all have the same deadline. txPopAndFreeTransfer(que, ins, tx_item, true); // drop the whole transfer + + tx_item = MUTABLE_CONTAINER_OF( // + struct CanardTxQueueItem, + cavlFindExtremum(que->deadline_root, false), + deadline_base); } } @@ -1213,8 +1220,6 @@ struct CanardTxQueueItem* canardTxPeek(const struct CanardTxQueue* const que) struct CanardTxQueueItem* out = NULL; if (que != NULL) { - // Paragraph 6.7.2.1.15 of the C standard says: - // A pointer to a structure object, suitably converted, points to its initial member, and vice versa. struct CanardTreeNode* const priority_node = cavlFindExtremum(que->priority_root, false); out = MUTABLE_CONTAINER_OF(struct CanardTxQueueItem, priority_node, priority_base); } diff --git a/libcanard/canard.h b/libcanard/canard.h index 905e74e..726d656 100644 --- a/libcanard/canard.h +++ b/libcanard/canard.h @@ -322,7 +322,7 @@ struct CanardTxQueueStats /// @param frame The mutable frame that is being handled. /// @return The result of the handling operation: /// - Any positive value: the frame was successfully handled. -/// This indicates that the frame payload was accepted (and its ownership could be potentially moved, +/// This indicates that the frame payload was accepted (and its payload ownership could be potentially moved, /// see `canardTxPeek` for the details), and the frame can be safely removed from the queue. /// - Zero: the frame was not handled, and so the frame should be kept in the queue. /// It will be retried on some future `canardTxPoll()` call according to the queue state in the future. @@ -546,6 +546,11 @@ struct CanardTxQueue canardTxInit(const size_t capacity, /// be dropped (incrementing `CanardTxQueueStats::dropped_frames` field per such a frame). /// If this timeout behavior is not needed, the timestamp value can be set to zero. /// +/// The described above automatic dropping of timed-out frames was added in the v4 of the library as an optional +/// feature. It is applied only to the frames that are already in the TX queue (not the new ones that are being pushed +/// in this call). The feature can be disabled by passing zero time in the `now_usec` parameter, +/// so that it will be up to the application to track the `tx_deadline_usec` (see `canardTxPeek`). +/// /// The function returns the number of frames enqueued into the prioritized TX queue (which is always a positive /// number) in case of success (so that the application can track the number of items in the TX queue if necessary). /// In case of failure, the function returns a negated error code: either invalid argument or out-of-memory. @@ -590,8 +595,8 @@ int32_t canardTxPush(struct CanardTxQueue* const que, /// /// The timestamp values of returned frames are initialized with tx_deadline_usec from canardTxPush(). /// Timestamps are used to specify the transmission deadline. It is up to the application and/or the media layer -/// to implement the discardment of timed-out transport frames. The library does not check it, so a frame that is -/// already timed out may be returned here. +/// to implement the discardment of timed-out transport frames. The library does not check it in this call, +/// so a frame that is already timed out may be returned here. /// /// If the queue is empty or if the argument is NULL, the returned value is NULL. /// @@ -650,6 +655,7 @@ void canardTxFree(struct CanardTxQueue* const que, /// @param que The TX queue to poll. /// @param ins The Canard instance. /// @param now_usec The current time in microseconds. It is used to determine if the frame has timed out. +/// Use zero value to disable automatic dropping of timed-out frames. /// @param user_reference The user reference to be passed to the frame handler. /// @param frame_handler The frame handler function that will be called to transmit the frame. /// @return Zero if the queue is empty or there is no frame handler (NULL). From f18c606d5bccd4ec2555f61ce1f4abcff455b720 Mon Sep 17 00:00:00 2001 From: Sergei Shirokov Date: Thu, 5 Dec 2024 16:05:28 +0200 Subject: [PATCH 13/14] updated migration guide --- MIGRATION_v3.x_to_v4.0.md | 124 +++++++++++++++++++++++++++++++------- libcanard/canard.h | 6 +- tests/helpers.hpp | 2 +- 3 files changed, 107 insertions(+), 25 deletions(-) diff --git a/MIGRATION_v3.x_to_v4.0.md b/MIGRATION_v3.x_to_v4.0.md index 852653c..37d5b61 100644 --- a/MIGRATION_v3.x_to_v4.0.md +++ b/MIGRATION_v3.x_to_v4.0.md @@ -30,6 +30,24 @@ These changes do not affect wire compatibility. ``` - **Usage**: After popping a transmission queue item using `canardTxPop`, use `canardTxFree` to deallocate its memory. +- **`canardTxPoll`**: + - **Description**: A helper function simplifies the transmission process by combining frame retrieval, transmission, and cleanup into a single function. + - **Prototype**: + ```c + int8_t canardTxPoll( + struct CanardTxQueue* const que, + const struct CanardInstance* const ins, + const CanardMicrosecond now_usec, + void* const user_reference, + const CanardTxFrameHandler frame_handler + ``` + - **Purpose**: Streamlines the process of handling frames from the TX queue. + - **Functionality**: + - Retrieves the next frame to be transmitted. + - Invokes a user-provided `frame_handler` to transmit the frame. + - Manages frame cleanup based on the handler's return value. + - Automatically drops timed-out frames if `now_usec` is provided. + ### Modified Functions Several functions have updated prototypes and usage patterns: @@ -49,7 +67,7 @@ Several functions have updated prototypes and usage patterns: - **Changes**: - Replaces `CanardMemoryAllocate` and `CanardMemoryFree` function pointers with a `CanardMemoryResource` struct. -2. **`canardTxInit`**: +1. **`canardTxInit`**: - **Old Prototype**: ```c CanardTxQueue canardTxInit( @@ -66,7 +84,7 @@ Several functions have updated prototypes and usage patterns: - **Changes**: - Adds a `CanardMemoryResource` parameter for memory allocation of payload data. -3. **`canardTxPush`**: +1. **`canardTxPush`**: - **Old Prototype**: ```c int32_t canardTxPush( @@ -84,12 +102,17 @@ Several functions have updated prototypes and usage patterns: const struct CanardInstance* const ins, const CanardMicrosecond tx_deadline_usec, const struct CanardTransferMetadata* const metadata, - const struct CanardPayload payload); + const struct CanardPayload payload, + const CanardMicrosecond now_usec); ``` - **Changes**: - Replaces `payload_size` and `payload` with a single `CanardPayload` struct. + - Adds a `now_usec` parameter for handling timed-out frames. + - **Purpose**: Allows the library to automatically drop frames that have exceeded their transmission deadlines (`tx_deadline_usec`). + - **Behavior**: If `now_usec` is greater than `tx_deadline_usec`, the frames already in the TX queue will be dropped, and the `dropped_frames` counter in `CanardTxQueueStats` will be incremented. + - **Optional Feature**: Passing `0` for `now_usec` disables automatic dropping, maintaining previous behavior. -4. **`canardTxPeek`** and **`canardTxPop`**: +1. **`canardTxPeek`** and **`canardTxPop`**: - The functions now return and accept pointers to mutable `struct CanardTxQueueItem` instead of const pointers. ### Removed Functions @@ -122,6 +145,16 @@ Several functions have updated prototypes and usage patterns: struct CanardInstance { ... }; ``` +- **Function pointers**: + - **Added** `CanardTxFrameHandler` function pointer type. + ```c + typedef int8_t (*CanardTxFrameHandler)( + void* const user_reference, + const CanardMicrosecond deadline_usec, + struct CanardMutableFrame* const frame + ); + ``` + ### Struct Modifications - **`CanardFrame`**: @@ -152,6 +185,7 @@ Several functions have updated prototypes and usage patterns: - **`CanardTxQueue`**: - Includes a `CanardMemoryResource` for payload data allocation. + - Includes a `CanardTxQueueStats` for tracking number of dropped frames. - **`CanardMemoryResource`** and **`CanardMemoryDeleter`**: - New structs to encapsulate memory allocation and deallocation functions along with user references. @@ -177,17 +211,37 @@ Several functions have updated prototypes and usage patterns: void* const pointer); ``` +## Automatic Dropping of Timed-Out Frames + +#### Description + +Frames in the TX queue that have exceeded their `tx_deadline_usec` can now be automatically dropped when `now_usec` is provided to `canardTxPush()` or `canardTxPoll()`. + +- **Benefit**: Reduces the need for manual management of timed-out frames. +- **Optional**: Feature can be disabled by passing `0` for `now_usec`. + +#### Migration Steps + +1. **Enable or Disable Automatic Dropping**: + + - **Enable**: Provide the current time to `now_usec` in both `canardTxPush()` and `canardTxPoll()`. + - **Disable**: Pass `0` to `now_usec` to retain manual control. + +2. **Adjust Application Logic**: + + - Monitor the `dropped_frames` counter in `CanardTxQueueStats` if tracking of dropped frames is required. + ## Migration Steps 1. **Update Type Definitions**: - Replace all `typedef`-based enum and struct types with direct `struct` and `enum` declarations. - For example, change `CanardInstance` to `struct CanardInstance`. -2. **Adjust Memory Management Code**: +1. **Adjust Memory Management Code**: - Replace separate memory allocation and deallocation function pointers with `CanardMemoryResource` and `CanardMemoryDeleter` structs. - Update function calls and definitions accordingly. -3. **Modify Function Calls**: +1. **Modify Function Calls**: - Update all function calls to match the new prototypes. - **`canardInit`**: - Before: @@ -223,30 +277,58 @@ Several functions have updated prototypes and usage patterns: .size = payload_size, .data = payload }; - canardTxPush(que, ins, tx_deadline_usec, metadata, payload_struct); + canardTxPush(que, ins, tx_deadline_usec, metadata, payload_struct, now_usec); ``` -4. **Handle New Functions**: +1. **Handle New Functions**: - Use `canardTxFree` to deallocate transmission queue items after popping them. - - Example: - ```c - struct CanardTxQueueItem* item = canardTxPeek(&tx_queue); - while (item != NULL) { - // Transmit the frame... - canardTxPop(&tx_queue, item); - canardTxFree(&tx_queue, &canard_instance, item); - item = canardTxPeek(&tx_queue); - } - ``` + - Example: + ```c + struct CanardTxQueueItem* item = canardTxPeek(&tx_queue); + while (item != NULL) { + // Transmit the frame... + canardTxPop(&tx_queue, item); + canardTxFree(&tx_queue, &canard_instance, item); + item = canardTxPeek(&tx_queue); + } + ``` -5. **Update Struct Field Access**: + - If currently using `canardTxPeek()`, `canardTxPop()`, and `canardTxFree()`, consider replacing that logic with `canardTxPoll()` for simplicity. + - Define a function matching the `CanardTxFrameHandler` signature: + ```c + int8_t myFrameHandler( + void* const user_reference, + const CanardMicrosecond deadline_usec, + struct CanardMutableFrame* frame + ) { + // Implement transmission logic here + // Return positive value on success - the frame will be released + // Return zero to retry later - the frame will stay in the TX queue + // Return negative value on failure - whole transfer (including this frame) will be dropped + } + ``` + - Example: + ```c + // Before + struct CanardTxQueueItem* item = canardTxPeek(queue); + if (item != NULL) { + // Handle deadline + // Transmit item->frame + item = canardTxPop(queue, item); + canardTxFree(queue, instance, item); + } + + // After + int8_t result = canardTxPoll(queue, instance, now_usec, user_reference, myFrameHandler); + ``` +1. **Update Struct Field Access**: - Adjust your code to access struct fields directly, considering the changes in struct definitions. - For example, access `payload.size` instead of `payload_size`. -6. **Adjust Memory Allocation Logic**: +1. **Adjust Memory Allocation Logic**: - Ensure that your memory allocation and deallocation functions conform to the new prototypes. - Pay attention to the additional `size` parameter in the deallocation function. -7. **Test Thoroughly**: +1. **Test Thoroughly**: - After making the changes, thoroughly test your application to ensure that it functions correctly with the new library version. - Pay special attention to memory management and potential leaks. diff --git a/libcanard/canard.h b/libcanard/canard.h index 726d656..d54f088 100644 --- a/libcanard/canard.h +++ b/libcanard/canard.h @@ -332,9 +332,9 @@ struct CanardTxQueueStats /// as well as all other frames belonging to the same transfer. The `dropped_frames` counter in the TX queue stats /// will be incremented for each frame dropped in this way. /// -typedef int8_t (*CanardTxFrameHandler)(void* const user_reference, - const CanardMicrosecond deadline_usec, - struct CanardMutableFrame* frame); +typedef int8_t (*CanardTxFrameHandler)(void* const user_reference, + const CanardMicrosecond deadline_usec, + struct CanardMutableFrame* const frame); /// Prioritized transmission queue that keeps CAN frames destined for transmission via one CAN interface. /// Applications with redundant interfaces are expected to have one instance of this type per interface. diff --git a/tests/helpers.hpp b/tests/helpers.hpp index a296415..70fb99d 100644 --- a/tests/helpers.hpp +++ b/tests/helpers.hpp @@ -362,7 +362,7 @@ class TxQueue &ins.getInstance(), now_usec, &frame_handler, - [](auto* user_reference, const auto deadline_usec, auto* frame) -> std::int8_t { + [](auto* user_reference, const auto deadline_usec, auto* const frame) -> std::int8_t { // const auto* const handler_ptr = static_cast(user_reference); return (*handler_ptr)(deadline_usec, *frame); From ce8f52f3ab3137cea8f949e88a800c2327434d75 Mon Sep 17 00:00:00 2001 From: Sergei Shirokov Date: Mon, 9 Dec 2024 13:01:19 +0200 Subject: [PATCH 14/14] updated migration guide --- MIGRATION_v3.x_to_v4.0.md | 2 +- libcanard/canard.c | 90 ++++++++++++++++++++++----------------- libcanard/canard.h | 27 +++++------- tests/test_public_tx.cpp | 9 +++- 4 files changed, 69 insertions(+), 59 deletions(-) diff --git a/MIGRATION_v3.x_to_v4.0.md b/MIGRATION_v3.x_to_v4.0.md index 37d5b61..669ffd0 100644 --- a/MIGRATION_v3.x_to_v4.0.md +++ b/MIGRATION_v3.x_to_v4.0.md @@ -217,7 +217,7 @@ Several functions have updated prototypes and usage patterns: Frames in the TX queue that have exceeded their `tx_deadline_usec` can now be automatically dropped when `now_usec` is provided to `canardTxPush()` or `canardTxPoll()`. -- **Benefit**: Reduces the need for manual management of timed-out frames. +- **Benefit**: Reduces the worst-case peak memory footprint. - **Optional**: Feature can be disabled by passing `0` for `now_usec`. #### Migration Steps diff --git a/libcanard/canard.c b/libcanard/canard.c index aff91b3..563025c 100644 --- a/libcanard/canard.c +++ b/libcanard/canard.c @@ -597,6 +597,10 @@ CANARD_PRIVATE void txPopAndFreeTransfer(struct CanardTxQueue* const que, struct CanardTxQueueItem* const tx_item, const bool drop_whole_transfer) { + CANARD_ASSERT(que != NULL); + CANARD_ASSERT(ins != NULL); + CANARD_ASSERT(tx_item != NULL); + struct CanardTxQueueItem* next_tx_item = tx_item; struct CanardTxQueueItem* tx_item_to_free = canardTxPop(que, next_tx_item); while (NULL != tx_item_to_free) @@ -619,10 +623,12 @@ CANARD_PRIVATE void txFlushExpiredTransfers(struct CanardTxQueue* const q const struct CanardInstance* const ins, const CanardMicrosecond now_usec) { - struct CanardTxQueueItem* tx_item = MUTABLE_CONTAINER_OF( // - struct CanardTxQueueItem, - cavlFindExtremum(que->deadline_root, false), - deadline_base); + CANARD_ASSERT(que != NULL); + CANARD_ASSERT(ins != NULL); + CANARD_ASSERT(now_usec > 0); + + struct CanardTreeNode* tx_node = cavlFindExtremum(que->deadline_root, false); + struct CanardTxQueueItem* tx_item = MUTABLE_CONTAINER_OF(struct CanardTxQueueItem, tx_node, deadline_base); while (NULL != tx_item) { if (now_usec <= tx_item->tx_deadline_usec) @@ -634,10 +640,8 @@ CANARD_PRIVATE void txFlushExpiredTransfers(struct CanardTxQueue* const q // All frames of the transfer are dropped at once b/c they all have the same deadline. txPopAndFreeTransfer(que, ins, tx_item, true); // drop the whole transfer - tx_item = MUTABLE_CONTAINER_OF( // - struct CanardTxQueueItem, - cavlFindExtremum(que->deadline_root, false), - deadline_base); + tx_node = cavlFindExtremum(que->deadline_root, false); + tx_item = MUTABLE_CONTAINER_OF(struct CanardTxQueueItem, tx_node, deadline_base); } } @@ -1245,7 +1249,7 @@ void canardTxFree(struct CanardTxQueue* const que, const struct CanardInstance* const ins, struct CanardTxQueueItem* item) { - if (item != NULL) + if ((que != NULL) && (ins != NULL) && (item != NULL)) { if (item->frame.payload.data != NULL) { @@ -1264,20 +1268,19 @@ int8_t canardTxPoll(struct CanardTxQueue* const que, void* const user_reference, const CanardTxFrameHandler frame_handler) { - int8_t out = 0; - - // Before peeking a frame to transmit, we need to try to flush any expired transfers. - // This will not only ensure ASAP freeing of the queue capacity, but also makes sure that the following - // `canardTxPeek` will return a not expired item (if any), so we don't need to check the deadline again. - // The flushing is done by comparing deadline timestamps of the pending transfers with the current time, - // which makes sense only if the current time is known (bigger than zero). - if (now_usec > 0) + int8_t out = -CANARD_ERROR_INVALID_ARGUMENT; + if ((que != NULL) && (ins != NULL) && (frame_handler != NULL)) { - txFlushExpiredTransfers(que, ins, now_usec); - } + // Before peeking a frame to transmit, we need to try to flush any expired transfers. + // This will not only ensure ASAP freeing of the queue capacity, but also makes sure that the following + // `canardTxPeek` will return a not expired item (if any), so we don't need to check the deadline again. + // The flushing is done by comparing deadline timestamps of the pending transfers with the current time, + // which makes sense only if the current time is known (bigger than zero). + if (now_usec > 0) + { + txFlushExpiredTransfers(que, ins, now_usec); + } - if (frame_handler != NULL) - { struct CanardTxQueueItem* const tx_item = canardTxPeek(que); if (tx_item != NULL) { @@ -1291,11 +1294,15 @@ int8_t canardTxPoll(struct CanardTxQueue* const que, if (out != 0) { // In case of a failure, it makes sense to drop the whole transfer immediately - // b/c at least this frame has been rejected, so the whole transfer is useless. + // b/c at least this frame has been rejected, so the whole transfer is useless. const bool drop_whole_transfer = (out < 0); txPopAndFreeTransfer(que, ins, tx_item, drop_whole_transfer); } } + else + { + out = 0; // No frames to transmit. + } } CANARD_ASSERT(out <= 1); @@ -1321,13 +1328,12 @@ int8_t canardRxAccept(struct CanardInstance* const ins, // This is the reason the function has a logarithmic time complexity of the number of subscriptions. // Note also that this one of the two variable-complexity operations in the RX pipeline; the other one // is memcpy(). Excepting these two cases, the entire RX pipeline contains neither loops nor recursion. - struct CanardRxSubscription* const sub = MUTABLE_CONTAINER_OF( // - struct CanardRxSubscription, - cavlSearch(&ins->rx_subscriptions[(size_t) model.transfer_kind], - &model.port_id, - &rxSubscriptionPredicateOnPortID, - NULL), - base); + struct CanardTreeNode* const sub_node = cavlSearch(&ins->rx_subscriptions[(size_t) model.transfer_kind], + &model.port_id, + &rxSubscriptionPredicateOnPortID, + NULL); + struct CanardRxSubscription* const sub = + MUTABLE_CONTAINER_OF(struct CanardRxSubscription, sub_node, base); if (out_subscription != NULL) { *out_subscription = sub; // Expose selected instance to the caller. @@ -1405,13 +1411,15 @@ int8_t canardRxUnsubscribe(struct CanardInstance* const ins, { CanardPortID port_id_mutable = port_id; - struct CanardRxSubscription* const sub = MUTABLE_CONTAINER_OF( // - struct CanardRxSubscription, - cavlSearch(&ins->rx_subscriptions[tk], &port_id_mutable, &rxSubscriptionPredicateOnPortID, NULL), - base); - if (sub != NULL) + struct CanardTreeNode* const sub_node = cavlSearch( // + &ins->rx_subscriptions[tk], + &port_id_mutable, + &rxSubscriptionPredicateOnPortID, + NULL); + if (sub_node != NULL) { - cavlRemove(&ins->rx_subscriptions[tk], &sub->base); + struct CanardRxSubscription* const sub = MUTABLE_CONTAINER_OF(struct CanardRxSubscription, sub_node, base); + cavlRemove(&ins->rx_subscriptions[tk], sub_node); CANARD_ASSERT(sub->port_id == port_id); out = 1; for (size_t i = 0; i < RX_SESSIONS_PER_SUBSCRIPTION; i++) @@ -1446,12 +1454,14 @@ int8_t canardRxGetSubscription(struct CanardInstance* const ins, { CanardPortID port_id_mutable = port_id; - struct CanardRxSubscription* const sub = MUTABLE_CONTAINER_OF( // - struct CanardRxSubscription, - cavlSearch(&ins->rx_subscriptions[tk], &port_id_mutable, &rxSubscriptionPredicateOnPortID, NULL), - base); - if (sub != NULL) + struct CanardTreeNode* const sub_node = cavlSearch( // + &ins->rx_subscriptions[tk], + &port_id_mutable, + &rxSubscriptionPredicateOnPortID, + NULL); + if (sub_node != NULL) { + struct CanardRxSubscription* const sub = MUTABLE_CONTAINER_OF(struct CanardRxSubscription, sub_node, base); CANARD_ASSERT(sub->port_id == port_id); if (out_subscription != NULL) { diff --git a/libcanard/canard.h b/libcanard/canard.h index d54f088..170806e 100644 --- a/libcanard/canard.h +++ b/libcanard/canard.h @@ -313,14 +313,10 @@ struct CanardTxQueueStats size_t dropped_frames; }; -/// Defines the signature of the TX frame handler function. -/// /// The handler function is intended to be invoked from Canard TX polling (see details for the `canardTxPoll()`). /// -/// @param user_reference The user reference passed to `canardTxPoll()`. -/// @param deadline_usec The deadline of the frame that is being handled. -/// @param frame The mutable frame that is being handled. -/// @return The result of the handling operation: +/// The user reference parameter what was passed to canardTxPoll. +/// The return result of the handling operation: /// - Any positive value: the frame was successfully handled. /// This indicates that the frame payload was accepted (and its payload ownership could be potentially moved, /// see `canardTxPeek` for the details), and the frame can be safely removed from the queue. @@ -642,24 +638,23 @@ void canardTxFree(struct CanardTxQueue* const que, struct CanardTxQueueItem* const item); /// This is a helper that combines several Canard TX calls (`canardTxPeek`, `canardTxPop` and `canardTxFree`) -/// into one "polling" algorythm. It simplifies the whole process of transmitting frames to just two function calls: +/// into one "polling" algorithm. It simplifies the whole process of transmitting frames to just two function calls: /// - `canardTxPush` to enqueue the frames /// - `canardTxPoll` to dequeue, transmit and free a single frame /// -/// The algorythm implements a typical pattern of de-queuing, transmitting and freeing a TX queue item, +/// The algorithm implements a typical pattern of de-queuing, transmitting and freeing a TX queue item, /// as well as handling transmission failures, retries, and deadline timeouts. /// /// The function is intended to be periodically called, most probably on a signal that the previous TX frame /// transmission has been completed, and so the next TX frame (if any) could be polled from the TX queue. /// -/// @param que The TX queue to poll. -/// @param ins The Canard instance. -/// @param now_usec The current time in microseconds. It is used to determine if the frame has timed out. -/// Use zero value to disable automatic dropping of timed-out frames. -/// @param user_reference The user reference to be passed to the frame handler. -/// @param frame_handler The frame handler function that will be called to transmit the frame. -/// @return Zero if the queue is empty or there is no frame handler (NULL). -/// Otherwise, the result from the frame handler call. See `CanardTxFrameHandler` documentation. +/// The current time is used to determine if the frame has timed out. Use zero value to disable automatic dropping +/// of timed-out frames. The user reference will be passed to the frame handler (see CanardTxFrameHandler), which +/// will be called to transmit the frame. +/// +/// Return value is zero if the queue is empty, +/// or `-CANARD_ERROR_INVALID_ARGUMENT` if there is no (NULL) queue, instance or handler. +/// Otherwise, the value will be from the result of the frame handler call (see CanardTxFrameHandler). /// int8_t canardTxPoll(struct CanardTxQueue* const que, const struct CanardInstance* const ins, diff --git a/tests/test_public_tx.cpp b/tests/test_public_tx.cpp index b7cbed8..b253860 100644 --- a/tests/test_public_tx.cpp +++ b/tests/test_public_tx.cpp @@ -970,9 +970,14 @@ TEST_CASE("TxPollSingleFrame") REQUIRE(1 == ins_alloc.getNumAllocatedFragments()); REQUIRE(sizeof(CanardTxQueueItem) * 1 == ins_alloc.getTotalAllocatedAmount()); - // 2. Poll without time and handler. + // 2. Poll with invalid arguments. // - REQUIRE(0 == que.poll(ins, 0, nullptr)); + REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == // null queue + canardTxPoll(nullptr, &ins.getInstance(), 0, nullptr, [](auto*, auto, auto*) -> std::int8_t { return 0; })); + REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == // null instance + canardTxPoll(&que.getInstance(), nullptr, 0, nullptr, [](auto*, auto, auto*) -> std::int8_t { return 0; })); + REQUIRE(-CANARD_ERROR_INVALID_ARGUMENT == // null handler + canardTxPoll(&que.getInstance(), &ins.getInstance(), 0, nullptr, nullptr)); // 3. Poll; emulate media is busy @ 10s + 100us //