stab

Author: pavel-kirienko

libcanard/canard.c

@@ -1229,48 +1229,6 @@ static void rx_slot_advance(rx_slot_t* const slot, const size_t extent, const ca
     slot->expected_toggle ^= 1U;
 }
 
-// This value is unreachable even if seqno is incremented by full transfer-ID period every frame at 5k frames/s;
-// at that rate, it would take about 56 years to wrap around.
-#define RX_SEQNO_MAX ((UINT64_C(1) << 48U) - 1U)
-
-// A compact representation is needed because we need to store an array of these in dynamic memory.
-typedef struct
-{
-    uint16_t limbs[3];
-} rx_seqno_packed_t;
-
-static uint64_t rx_seqno_unpack(const rx_seqno_packed_t v)
-{
-    return ((uint64_t)v.limbs[0]) | (((uint64_t)v.limbs[1]) << 16U) | (((uint64_t)v.limbs[2]) << 32U);
-}
-static rx_seqno_packed_t rx_seqno_pack(const uint64_t v)
-{
-    return (rx_seqno_packed_t){
-        { (uint16_t)(v & 0xFFFFU), (uint16_t)((v >> 16U) & 0xFFFFU), (uint16_t)((v >> 32U) & 0xFFFFU) }
-    };
-}
-
-// The linearizer logic results in a behavior that differs compared to v4 significantly: v4 used to admit any
-// transfer-ID except the same and the one older compared to the last transfer; in terms of the linearization logic it
-// means that the reference point was biased.
-static uint64_t rx_seqno_linearize(const uint64_t ref_seqno, const byte_t transfer_id)
-{
-    CANARD_ASSERT(transfer_id <= CANARD_TRANSFER_ID_MAX);
-    const byte_t ref_tid = (byte_t)(ref_seqno & CANARD_TRANSFER_ID_MAX);
-    int16_t      delta   = ((int16_t)transfer_id) - ((int16_t)ref_tid); // NOLINT(*-narrowing-conversions)
-    // Select the nearest congruent seqno in the half-open interval [-16, +16) around the reference.
-    if (delta > (int16_t)(CANARD_TRANSFER_ID_MAX / 2U)) {
-        delta -= (int16_t)CANARD_TRANSFER_ID_MODULO;
-    } else if (delta < -((int16_t)(CANARD_TRANSFER_ID_MAX / 2U) + 1)) {
-        delta += (int16_t)CANARD_TRANSFER_ID_MODULO;
-    }
-    // Near the origin the backward representative may underflow, so use the next forward one instead.
-    if ((delta < 0) && (ref_seqno < (uint64_t)(-delta))) {
-        delta += (int16_t)CANARD_TRANSFER_ID_MODULO;
-    }
-    return (uint64_t)(((int64_t)ref_seqno) + ((int64_t)delta));
-}
-
 // Up to libcanard v4 we used a fixed-capacity array of pointers for per-remote sessions for constant-time lookup,
 // but it was too costly on MCUs: with a 32-bit pointer it took 512 bytes for the array plus overheads,
 // resulting in 1 KiB o1heap blocks per session, very expensive. Here we use a much less RAM-heavy approach with
@@ -1305,9 +1263,9 @@ typedef struct
     canard_us_t            last_admission_ts;
     rx_slot_t*             slots[CANARD_PRIO_COUNT]; // Indexed by priority level to allow preemption.
     canard_subscription_t* owner;
-    rx_seqno_packed_t      seqno_frontier[CANARD_PRIO_COUNT];
-    byte_t                 iface_index;
     byte_t                 node_id;
+    byte_t                 last_admitted_transfer_id : CANARD_TRANSFER_ID_BIT_LENGTH;
+    byte_t                 iface_index : IFACE_INDEX_BIT_LENGTH;
 } rx_session_t;
 static_assert((sizeof(void*) > 4) || (sizeof(rx_session_t) <= 120), "too large");
 
@@ -1375,109 +1333,67 @@ static size_t rx_session_cleanup(rx_session_t* const ses, const canard_us_t now)
     return n_slots;
 }
 
-// Maximum seqno seen from the given highest priority level (numerically lowest, inclusive) and down.
-static uint64_t rx_session_seqno_frontier(const rx_session_t* const ses, const canard_prio_t highest_priority)
-{
-    uint64_t seqno = 0;
-    for (size_t i = (size_t)highest_priority; i < CANARD_PRIO_COUNT; i++) {
-        seqno = max_u64(seqno, rx_seqno_unpack(ses->seqno_frontier[i]));
-    }
-    return seqno;
-}
-
 static void rx_session_record_admission(rx_session_t* const ses,
-                                        const canard_prio_t priority,
-                                        const uint64_t      seqno,
+                                        const byte_t        transfer_id,
                                         const canard_us_t   ts,
                                         const byte_t        iface_index)
 {
-    // Seqno per priority cannot go back. When we reset it on a transfer-ID timeout, we simply bump the seqno state by
-    // a multiple of transfer-ID overflow periods to ensure monotonicity. Earlier I tried dumb direct approaches
-    // where we erase the seqno states to zero on tid-timeout, or introduce epoch counters, or per-seqno timestamp,
-    // but this solution is so much simpler while achieves the same goal: leave older seqnos behind, start new epoch.
-    CANARD_ASSERT(seqno > rx_seqno_unpack(ses->seqno_frontier[priority]));
-    ses->seqno_frontier[priority] = rx_seqno_pack(seqno); // nothing older than this at this & higher prio from now on
-    ses->last_admission_ts        = ts;
-    ses->iface_index              = iface_index;
+    ses->last_admission_ts         = ts;
+    ses->last_admitted_transfer_id = transfer_id & CANARD_TRANSFER_ID_MAX;
+    ses->iface_index               = iface_index & ((1U << IFACE_INDEX_BIT_LENGTH) - 1U);
 }
 
-#define RX_SESSION_ADMISSION_REJECTED     (UINT64_MAX)
-#define RX_SESSION_ADMISSION_CONTINUATION (UINT64_MAX - 1)
-
 // Frame admittance solver. A complex piece, redesigned after v4 to support priority preemption.
-// Key ideas: 1. Separate reassembly state per priority level. 2. TID is linearized into seqno.
-// Once we admit a transfer at some priority with a certain seqno, we know that any older seqno at this or higher
-// priority would be stale, since only higher priority transfers can preempt lower priority ones.
-// On a transfer-ID timeout the seqno is bumped by a full transfer-ID timeout to ensure that it becomes the new
-// frontier matching the current transfer-ID while maintaining monotonicity.
-static uint64_t rx_session_solve_admission(const rx_session_t* const ses,
-                                           const canard_us_t         ts,
-                                           const canard_prio_t       priority,
-                                           const bool                start,
-                                           const bool                toggle,
-                                           const byte_t              transfer_id,
-                                           const byte_t              iface_index)
+static bool rx_session_solve_admission(const rx_session_t* const ses,
+                                       const canard_us_t         ts,
+                                       const canard_prio_t       priority,
+                                       const bool                start,
+                                       const bool                toggle,
+                                       const byte_t              transfer_id,
+                                       const byte_t              iface_index)
 {
     // Continuation frames cannot create new state so their handling is simpler.
     // They are only accepted if there is a slot with an exact match of all transfer parameters.
     // We ignore the transfer-ID timeout to avoid breaking transfers that are preempted for a long time,
     // and especially to allow reassembly of multi-frame transfers even when the transfer-ID timeout is zero.
     if (!start) {
         const rx_slot_t* const slot = ses->slots[priority];
-        const bool admit = (slot != NULL) && (slot->transfer_id == transfer_id) && (slot->iface_index == iface_index) &&
-                           (slot->expected_toggle == toggle);
-        return admit ? RX_SESSION_ADMISSION_CONTINUATION : RX_SESSION_ADMISSION_REJECTED;
+        return (slot != NULL) && (slot->transfer_id == transfer_id) && (slot->iface_index == iface_index) &&
+               (slot->expected_toggle == toggle);
     }
-
-    // It is best to postpone seqno derivation until the last moment because it is costly.
-    // Life would have been so much easier if we could just use normal non-wrapping IDs like we have in Cyphal/UDP!
-    const uint64_t frontier_global = rx_session_seqno_frontier(ses, canard_prio_exceptional);
-    const uint64_t seqno           = rx_seqno_linearize(frontier_global, transfer_id);
-
     // Duplicate start frames do not require special treatment because a duplicate frame can only follow the original
     // without any frames belonging to the same transfer in between (see the assumptions). If we get a duplicate start,
     // with a nonzero TID timeout it will be rejected as not-new; even if the timeout is zero, accepting the duplicate
     // will simply restart the slot without loss of information since there were no other frames in between.
     //
-    // In the case of a single priority level, the seqno would be considered new if it is greater than last admitted.
-    // Priority preemption makes this simple condition insufficient, because a newer higher-priority transfer with
-    // a greater seqno may push older lower-priority transfers aside, causing the receiver to observe seqno going
-    // backward for new transfers. Our solution is to keep a dedicated seqno frontier per priority level.
-    // The lowest bound of a genuinely new seqno would then be located at the lowest priority level, since that level
-    // cannot preempt others. To decide if a given seqno is new, one needs to scan all priority levels from the
-    // current one (inclusive) down to the lowest level (numerically greater).
-    // Higher priority levels (numerically lesser) may have greater seqno frontiers which bear no relevance.
-    //
-    // The original design had a special case that enabled admittance of a transfer with the next seqno from a
+    // The original design had a special case that enabled admittance of a transfer with the next transfer-ID from a
     // different interface if there is no pending reassembly in progress; see details here and the original v4 code:
     // https://github.com/OpenCyphal/libcanard/issues/228. This behavior is no longer present here mostly because we
     // now update the session state only on admittance and not upon completion of a transfer, which changes the logic
     // considerably. One side effect is that even after a timeout (potentially a very long time as long as the session
     // survives) we will still reject a new transfer arriving from a different interface if it happened to roll the
-    // same transfer-ID timeout. This is not an issue because we would still accept new transfers on the same iface,
+    // same transfer-ID. This is not an issue because we would still accept new transfers on the same iface,
     // and after the RX_SESSION_TIMEOUT the session is destroyed and all new transfers will be accepted unconditionally.
-    const uint64_t frontier_priority = rx_session_seqno_frontier(ses, priority);
-    const bool     seqno_new         = seqno > frontier_priority;
-    const bool     iface_match       = ses->iface_index == iface_index;
-    const bool     timed_out         = ts > (ses->last_admission_ts + ses->owner->transfer_id_timeout);
-    const bool     admit = (seqno_new && iface_match) || (iface_match && timed_out) || (timed_out && seqno_new);
-    if (!admit) {
-        return RX_SESSION_ADMISSION_REJECTED;
-    }
-
-    // It is vital that seqno is monotonically increasing even across timeouts, otherwise following a transfer-ID
-    // timeout further arrivals on higher priority levels may be rejected if their seqnos are greater.
-    // Instead of sweeping seqnos across all priority levels, we ensure monotonicity without breaking transfer-ID
-    // matching by bumping the seqno by the minimal required number of overflow periods.
-    uint64_t admitted_seqno = seqno;
-    if (!seqno_new) {
-        static const uint64_t mod = CANARD_TRANSFER_ID_MODULO;
-        CANARD_ASSERT(frontier_priority >= admitted_seqno);
-        const uint64_t periods = ((frontier_priority - admitted_seqno) + mod) / mod;
-        admitted_seqno += periods * mod;
-    }
-    CANARD_ASSERT(admitted_seqno > frontier_priority);
-    return admitted_seqno;
+    //
+    // This design has a problem: suppose the transfer-ID modulo is 4, i.e., the values are {0, 1, 2, 3};
+    // a low-priority transfer is sent but preempted by higher-priority peers before it hits the bus.
+    // This may potentially be addressed with local transfer-ID unrolling into seqno as illustrated.
+    //
+    //                  SENDER SIDE
+    //  P=low       0
+    //  P=high          1   2   3   0
+    //
+    //                  RECEIVER SIDE
+    //  P=low                           0   <-- rejected as duplicate!
+    //  P=high          1   2   3   0
+    //
+    //                  RECEIVER SIDE [with unrolling]
+    //  P=low                           5   <-- thanks to unrolling we observe this as not a duplicate
+    //  P=high          1   2   3   4       <-- unrolled
+    const bool tid_new     = transfer_id != ses->last_admitted_transfer_id;
+    const bool iface_match = ses->iface_index == iface_index;
+    const bool timed_out   = ts > (ses->last_admission_ts + ses->owner->transfer_id_timeout);
+    return (tid_new && iface_match) || (iface_match && timed_out) || (timed_out && tid_new);
 }
 
 // Returns false on OOM, no other failure modes.
@@ -1514,16 +1430,14 @@ static bool rx_session_update(canard_subscription_t* const sub,
     }
 
     // Decide admit or drop.
-    const uint64_t seqno = rx_session_solve_admission(
+    const bool admit = rx_session_solve_admission(
       ses, ts, frame->priority, frame->start, frame->toggle, frame->transfer_id, iface_index);
-    if (seqno == RX_SESSION_ADMISSION_REJECTED) {
+    if (!admit) {
         return true; // Rejection is not a failure.
     }
 
     // The frame must be accepted. If this is the start of a new transfer, we must update state.
-    if (seqno != RX_SESSION_ADMISSION_CONTINUATION) {
-        CANARD_ASSERT(seqno <= RX_SEQNO_MAX);
-        CANARD_ASSERT(frame->start);
+    if (frame->start) {
         // Animate only when a new transfer is started to manage load. Correctness-wise there is not much difference.
         enlist_tail(&sub->owner->rx.list_session_by_animation, &ses->list_animation);
         // Destroy the old slot if it exists (if we're discarding a stale transfer).
@@ -1541,7 +1455,7 @@ static bool rx_session_update(canard_subscription_t* const sub,
             }
         }
         // Register the new state only after we have a confirmation that we have memory to store the frame.
-        rx_session_record_admission(ses, frame->priority, seqno, ts, iface_index);
+        rx_session_record_admission(ses, frame->transfer_id, ts, iface_index);
     }
 
     // TODO acceptance is not yet implemented.