introduce deadline queue #sonar #238

serges147 · 2024-12-03T15:21:24Z

No description provided.

pavel-kirienko · 2024-12-03T20:23:33Z

libcanard/canard.c

@@ -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))))


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*?

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

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

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

pavel-kirienko · 2024-12-03T20:32:58Z

libcanard/canard.h

+/// Holds the statistics of a transmission queue.
+struct CanardTxQueueStats
+{
+    /// Holds number of dropped TX frames (due to timeout when `now > deadline`).


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.

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.

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?

sonarcloud · 2024-12-04T15:47:09Z

Quality Gate failed

Failed conditions
90.7% Coverage on New Code (required ≥ 99%)

See analysis details on SonarQube Cloud

libcanard/canard.c

pavel-kirienko

The migration guide requires updating.

pavel-kirienko · 2024-12-05T09:51:41Z

libcanard/canard.h

@@ -502,7 +519,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


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.

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

pavel-kirienko · 2024-12-05T09:54:11Z

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


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

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.

pavel-kirienko · 2024-12-05T09:56:55Z

libcanard/canard.c

@@ -1114,7 +1204,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);


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

pavel-kirienko · 2024-12-05T09:58:47Z

libcanard/canard.c

+                                            const CanardMicrosecond            now_usec)
+{
+    struct CanardTxQueueItem* tx_item = NULL;
+    while (NULL != (tx_item = MUTABLE_CONTAINER_OF(  //


Assignment in an expression violates some MISRA C rule whose number I forgot (ChatGPT can tell you which one); please split this into the longer form.

pavel-kirienko · 2024-12-05T10:04:20Z

libcanard/canard.c

+            break;
+        }
+
+        // All frames of the transfer are released at once b/c they all have the same deadline.


Do we really need this special case here? All frames in a transfer have the same timeout (IIRC it is mentioned in the docs, too), so either all or none will be kept even if you don't include this special case. On the other hand, one might argue that the current solution is more robust because it doesn't make assumptions about timeout equality. So maybe we do need this special case, after all.

There is also a related question: do we really need to locate the extremum at every iteration? This looks like an avoidable Shliemel the painter problem. What we have right now is good enough but perhaps an explanatory comment is warranted, saying that it should be possible to avoid extremum call at every iteration by simply fetching the neighboring node from the current one before deleting it. Obviously this will only work if we rely on the assumption that the timeout is the same for all frames in the transfer (otherwise the loop below will invalidate the tree linkage).

I believe that such flushing is more like for robustness and "self healing" for a special corner case, namely when media stopped to work for some time (f.e. stopped to answer/callback at all), whole TX capacity exhausted, then media recovered but it won't be engaged/triggered anymore b/c we cannot push anything new due to full capacity. Flushing by timeout will resolve such deadlock.

really need to locate the extremum

I think it is okey, especially if you think about nominal use case in which there will be only one call to cavlFindExtremum, it will return NULL (b/c normally everything properly transmitted before deadline), and that is it.

While the current solution is acceptable, I must point out that in real-time programming, one should focus not only on the general case but also on the worst case.

serges147 · 2024-12-05T11:57:05Z

The migration guide requires updating.

Yes, it will be done in the next pr when new canardTxPoll appears.

introduce deadline queue #sonar

5670ba5

serges147 self-assigned this Dec 3, 2024

serges147 requested a review from pavel-kirienko December 3, 2024 15:21

pavel-kirienko reviewed Dec 3, 2024

View reviewed changes

serges147 added 3 commits December 3, 2024 23:48

pr review and build fixes #sonar

3c1535f

build fixes #sonar

4f1a28c

minor fix

7ec9051

serges147 requested a review from pavel-kirienko December 3, 2024 22:43

serges147 and others added 2 commits December 4, 2024 00:44

style fix

c67a59c

canardTxPush now flushes expired frames

ee67adc

serges147 marked this pull request as ready for review December 4, 2024 14:14

build fix

78b62fa

serges147 commented Dec 4, 2024

View reviewed changes

libcanard/canard.c Show resolved Hide resolved

pavel-kirienko approved these changes Dec 5, 2024

View reviewed changes

serges147 merged commit 5220b05 into v4 Dec 5, 2024
16 of 17 checks passed

serges147 deleted the sshirokov/v4_deadline branch December 5, 2024 13:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce deadline queue #sonar #238

introduce deadline queue #sonar #238

serges147 commented Dec 3, 2024

pavel-kirienko Dec 3, 2024

serges147 Dec 3, 2024

pavel-kirienko Dec 4, 2024

serges147 Dec 4, 2024

pavel-kirienko Dec 3, 2024

serges147 Dec 3, 2024 •

edited

Loading

pavel-kirienko Dec 4, 2024

sonarcloud bot commented Dec 4, 2024

pavel-kirienko left a comment

pavel-kirienko Dec 5, 2024

serges147 Dec 5, 2024

pavel-kirienko Dec 5, 2024

serges147 Dec 5, 2024

pavel-kirienko Dec 5, 2024

pavel-kirienko Dec 5, 2024

pavel-kirienko Dec 5, 2024

serges147 Dec 5, 2024 •

edited

Loading

pavel-kirienko Dec 5, 2024

serges147 commented Dec 5, 2024

introduce deadline queue #sonar #238

introduce deadline queue #sonar #238

Conversation

serges147 commented Dec 3, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serges147 Dec 3, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Dec 4, 2024

Quality Gate failed

pavel-kirienko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serges147 Dec 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serges147 commented Dec 5, 2024

serges147 Dec 3, 2024 •

edited

Loading

serges147 Dec 5, 2024 •

edited

Loading