Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

introduce deadline queue #sonar #238

Merged
merged 7 commits into from
Dec 5, 2024
Merged

introduce deadline queue #sonar #238

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5670ba5
introduce deadline queue #sonar
serges147 Dec 3, 2024
3c1535f
pr review and build fixes #sonar
serges147 Dec 3, 2024
4f1a28c
build fixes #sonar
serges147 Dec 3, 2024
7ec9051
minor fix
serges147 Dec 3, 2024
c67a59c
style fix
serges147 Dec 3, 2024
ee67adc
`canardTxPush` now flushes expired frames
serges147 Dec 4, 2024
78b62fa
build fix
serges147 Dec 4, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 91 additions & 33 deletions libcanard/canard.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
/// Author: Pavel Kirienko <pavel@opencyphal.org>

#include "canard.h"
#include <stddef.h>
#include <string.h>

// --------------------------------------------- BUILD CONFIGURATION ---------------------------------------------
Expand Down Expand Up @@ -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;
Expand All @@ -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))))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should be near the top. Also, there is risk of this expression accidentally casting away a const qualifier on the pointer. Maybe we should have const and mutable editions of this macro, where the const one would use const type* and const char*?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should have const and mutable editions of this macro, where the const one would use const type* and const char*?

Good point - I will move it to the top, and make two versions: CONTAINER_OF & CONST_CONTAINER_OF

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Usually we assume immutability by default, like with the payload containers, for example.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to have CONTAINER_OF (const version) and MUTABLE_CONTAINER_OF (mutable version) ?


/// 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,
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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
Expand All @@ -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 ---------------------------------------------
Expand Down Expand Up @@ -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,
};
Expand All @@ -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;
pavel-kirienko marked this conversation as resolved.
Show resolved Hide resolved

int32_t out = -CANARD_ERROR_INVALID_ARGUMENT;
if ((ins != NULL) && (que != NULL) && (metadata != NULL) && ((payload.data != NULL) || (0U == payload.size)))
{
Expand Down Expand Up @@ -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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment above (citing the standard) is no longer applicable so it should be removed.

out = CONTAINER_OF(struct CanardTxQueueItem, priority_node, priority_base);
}
return out;
}
Expand All @@ -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;
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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;
Expand All @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
27 changes: 23 additions & 4 deletions libcanard/canard.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -306,6 +306,13 @@ struct CanardMemoryResource
CanardMemoryAllocate allocate; ///< Shall be a valid pointer.
};

/// Holds the statistics of a transmission queue.
struct CanardTxQueueStats
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if we name it just CanardStats, with the possibility of future extension with other metrics? Then dropped_frames becomes tx_dropped_frames.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, if you have redundant media, you can see such stats per TX queue. If we move CanardTxQueueStats out of CanardTxQueue (to the CanaradInstance I assume as more generic CanardStats is proposed) then we will loose such distinction.
Also, under Udpard there is no such thing as "UdpardInstance" as you know of course - my point is that under Udpard we have to keep it per TX queue (I assume that we want similar improvements at Udpard as well, namely: two trees, flush on push, udpardTxPoll).

So, I propose to keep it as I did. The only thing what maybe could be valueable is to split dropped_frames to two separate counters:

  • dropped_expired_frames - incremented by the canardTxPush and canardTxPoll for each expired frame.
  • dropped_failed_frames - incremented by the new canardTxPoll when frame handler returns negative result.
    Currently both cases increment the very same dropped_frames.

{
/// Holds number of dropped TX frames (due to timeout when `now > deadline`).
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not just timeout. If the media fails to accept a frame, we discard it, right? Along with every other frame in the transfer chain.

Copy link
Contributor Author

@serges147 serges147 Dec 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such business logic was out of lizard scope. Yes, at libcyphal I did it as you described. At flux_grip and telega I'm not 100% sure, but it seems that there is no notion of error there - just boolean either accepted or not accepted ifc->push. If it was not accepted then it stays in the queue - for retry purposes.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but if we are designing a template method ("template" in the sense of design patterns, not type-generic programming), then it makes sense to support fallible media, right?

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.
Expand Down Expand Up @@ -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
Expand All @@ -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.
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think here we could use a clarification that we're talking about frames that were enqueued earlier, not the ones that are added by this call.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

already fixed in the next pr branch (sshirokov/v4_tx_poll)

/// 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).
Expand Down Expand Up @@ -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
Expand Down
21 changes: 17 additions & 4 deletions tests/helpers.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::size_t>(ret);
enforce((ret < 0) || ((size_before + num_added) == que_.size), "Unexpected size change after push");
checkInvariants();
Expand Down Expand Up @@ -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++;
});
Expand All @@ -352,7 +364,7 @@ class TxQueue
[[nodiscard]] auto linearize() const -> std::vector<const exposed::TxItem*>
{
std::vector<const exposed::TxItem*> out;
traverse(que_.root, [&](const CanardTreeNode* const item) {
traverse(que_.priority_root, [&](const CanardTreeNode* const item) {
out.push_back(reinterpret_cast<const exposed::TxItem*>(item));
});
enforce(out.size() == getSize(), "Internal error");
Expand Down Expand Up @@ -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_;
Expand Down
Loading
Loading