Allow changing cy_subscription_t::extent at any time

Author: pavel-kirienko

.clang-tidy

@@ -36,6 +36,7 @@ Checks: >-
   -*-avoid-c-arrays,
   -*-nested-conditional-operator,
   -*DeprecatedOrUnsafeBufferHandling,
+  -*-implicit-bool-conversion,
 CheckOptions:
   - key:   readability-function-cognitive-complexity.Threshold
     value: '99'

libcanard/canard.c

@@ -1190,6 +1190,7 @@ static byte_t rx_parse(const uint32_t       can_id,
 typedef struct
 {
     canard_us_t start_ts;
+    size_t      extent;     // Captured from sub->extent at slot creation time.
     uint32_t    total_size; // The raw payload size seen before the implicit truncation and CRC removal.
     uint16_t    crc;
     byte_t      transfer_id : CANARD_TRANSFER_ID_BITS;
@@ -1198,7 +1199,6 @@ typedef struct
     byte_t      payload[]; // Extent-sized.
 } rx_slot_t;
 #define RX_SLOT_OVERHEAD (offsetof(rx_slot_t, payload))
-static_assert(RX_SLOT_OVERHEAD <= 16, "unexpected layout");
 
 static rx_slot_t* rx_slot_new(const canard_subscription_t* const sub,
                               const canard_us_t                  start_ts,
@@ -1209,6 +1209,7 @@ static rx_slot_t* rx_slot_new(const canard_subscription_t* const sub,
     if (slot != NULL) {
         memset(slot, 0, RX_SLOT_OVERHEAD);
         slot->start_ts        = start_ts;
+        slot->extent          = sub->extent;
         slot->crc             = sub->crc_seed;
         slot->transfer_id     = transfer_id & CANARD_TRANSFER_ID_MAX;
         slot->expected_toggle = canard_kind_version(sub->kind) & 1U;
@@ -1219,13 +1220,15 @@ static rx_slot_t* rx_slot_new(const canard_subscription_t* const sub,
 
 static void rx_slot_destroy(const canard_subscription_t* const sub, rx_slot_t* const slot)
 {
-    mem_free(sub->owner->mem.rx_payload, RX_SLOT_OVERHEAD + sub->extent, slot);
+    if (slot != NULL) {
+        mem_free(sub->owner->mem.rx_payload, RX_SLOT_OVERHEAD + slot->extent, slot);
+    }
 }
 
-static void rx_slot_advance(rx_slot_t* const slot, const size_t extent, const canard_bytes_t payload)
+static void rx_slot_advance(rx_slot_t* const slot, const canard_bytes_t payload)
 {
-    if (slot->total_size < extent) {
-        const size_t copy_size = smaller(payload.size, (size_t)(extent - slot->total_size)); // NOLINT(*-casting)
+    if (slot->total_size < slot->extent) {
+        const size_t copy_size = smaller(payload.size, (size_t)(slot->extent - slot->total_size)); // NOLINT(*-casting)
         (void)memcpy(&slot->payload[slot->total_size], payload.data, copy_size);
     }
     slot->total_size = (uint32_t)(slot->total_size + payload.size); // Before truncation.
@@ -1358,12 +1361,12 @@ static void rx_session_complete_slot(rx_session_t* const ses, const frame_t* con
     ses->slots[fr->priority] = NULL; // Slot memory ownership transferred to the application, or destroyed.
     const bool     v1        = canard_kind_version(sub->kind) == 1;
     const uint16_t crc_ref   = v1 ? CRC_RESIDUE : (uint16_t)(slot->payload[0] | (((unsigned)slot->payload[1]) << 8U));
-    CANARD_ASSERT(v1 || (sub->extent >= CRC_BYTES)); // In v0, the CRC size is included in the extent.
+    CANARD_ASSERT(v1 || (slot->extent >= CRC_BYTES)); // In v0, the CRC size is included in the extent.
     if (slot->crc == crc_ref) {
-        const size_t           size    = smaller(slot->total_size - CRC_BYTES, sub->extent - (v1 ? 0 : CRC_BYTES));
+        const size_t           size    = smaller(slot->total_size - CRC_BYTES, slot->extent - (v1 ? 0 : CRC_BYTES));
         const canard_payload_t payload = {
             .view   = { .data = v1 ? slot->payload : &slot->payload[CRC_BYTES], .size = size },
-            .origin = { .data = slot, .size = RX_SLOT_OVERHEAD + sub->extent },
+            .origin = { .data = slot, .size = RX_SLOT_OVERHEAD + slot->extent },
         };
         sub->vtable->on_message(sub, slot->start_ts, fr->priority, fr->src, fr->transfer_id, payload);
     } else {
@@ -1380,7 +1383,7 @@ static void rx_session_accept(rx_session_t* const ses, const canard_us_t ts_fram
     if (slot != NULL) {
         CANARD_ASSERT((!fr->start || !fr->end) && (slot->expected_toggle == fr->toggle));
         CANARD_ASSERT((slot->transfer_id == fr->transfer_id) && (slot->iface_index == ses->iface_index));
-        rx_slot_advance(slot, sub->extent, fr->payload);
+        rx_slot_advance(slot, fr->payload);
         // Multi-frame transfers place CRC differently in v1 and v0.
         // The v1 handling is trivial: simply compute the full payload CRC and ensure the residue is correct.
         // The payload may be truncated to the subscription extent, but the CRC is computed over the full payload.

libcanard/canard.h

@@ -238,13 +238,13 @@ struct canard_subscription_vtable_t
 /// Subscription instances must not be moved while in use.
 /// Each subscription is indexed by its port-ID inside the canard instance, and in turn contains a tree of sessions
 /// indexed by remote node-ID. Two log-time lookups are thus required to handle an incoming frame.
-/// None of the fields may be mutated by the application after initialization except for the user context.
+/// None of the fields may be mutated by the application after initialization except for the user context and extent.
 struct canard_subscription_t
 {
     canard_tree_t index_port_id; ///< Must be the first member.
 
     canard_us_t   transfer_id_timeout;
-    size_t        extent;   ///< Must not be altered after initialization! In v0 includes the CRC.
+    size_t        extent;   ///< May be changed at any time, takes effect with next transfer. In v0 must include CRC!
     uint16_t      port_id;  ///< Represents subjects, services, and legacy message- and service data type IDs.
     uint16_t      crc_seed; ///< For v0 this is set at subscription time, for v1 this is always 0xFFFF.
     canard_kind_t kind;

tests/src/test_api_rx_edge.cpp

@@ -1301,6 +1301,192 @@ static void test_rx_v0_extent_truncation()
     canard_destroy(&self);
 }
 
+// -------------------------------------------  Dynamic extent
+// ----------------------------------------------------------
+
+/// Change extent between two complete multiframe transfers. The second transfer must use the new extent.
+static void test_rx_extent_change_between_transfers()
+{
+    canard_t    self    = {};
+    canard_us_t now_val = 0;
+    init_canard(&self, &now_val, 42U);
+
+    rx_capture_t          cap = {};
+    canard_subscription_t sub = {};
+    TEST_ASSERT_TRUE(canard_subscribe_16b(&self, &sub, 1500U, 5U, 2000000, &capture_sub_vtable));
+    sub.user_context = ∩
+
+    const uint_least8_t payload[8] = { 0xA0, 0xA1, 0xA2, 0xA3, 0xA4, 0xA5, 0xA6, 0xA7 };
+    const uint32_t      can_id     = make_v1v1_msg_can_id(canard_prio_nominal, 1500U, 10U);
+
+    // First transfer with extent=5. CRC over 8 payload + 4 padding.
+    const uint_least8_t pad[4] = {};
+    uint16_t            crc    = crc16_ccitt(0xFFFFU, payload, 8U);
+    crc                        = crc16_ccitt(crc, pad, 4U);
+    const auto crc_hi          = static_cast<uint_least8_t>((static_cast<unsigned>(crc) >> 8U) & 0xFFU);
+    const auto crc_lo          = static_cast<uint_least8_t>(crc & 0xFFU);
+
+    uint_least8_t frame1[8];
+    std::memcpy(frame1, payload, 7U);
+    frame1[7] = make_v1_tail(true, false, true, 2U);
+
+    uint_least8_t frame2[8];
+    frame2[0] = payload[7];
+    frame2[1] = 0x00;
+    frame2[2] = 0x00;
+    frame2[3] = 0x00;
+    frame2[4] = 0x00;
+    frame2[5] = crc_hi;
+    frame2[6] = crc_lo;
+    frame2[7] = make_v1_tail(false, true, false, 2U);
+
+    now_val = 100;
+    TEST_ASSERT_TRUE(
+      canard_ingest_frame(&self, 100, 0U, can_id, canard_bytes_t{ .size = sizeof(frame1), .data = frame1 }));
+    TEST_ASSERT_TRUE(
+      canard_ingest_frame(&self, 200, 0U, can_id, canard_bytes_t{ .size = sizeof(frame2), .data = frame2 }));
+    TEST_ASSERT_EQUAL_size_t(1U, cap.count);
+    TEST_ASSERT_EQUAL_size_t(5U, cap.payload_size); // Truncated to extent=5.
+
+    // Change extent to 100 and send a second transfer (TID=3).
+    sub.extent = 100;
+    cap        = {};
+
+    uint_least8_t frame3[8];
+    std::memcpy(frame3, payload, 7U);
+    frame3[7] = make_v1_tail(true, false, true, 3U);
+
+    uint_least8_t frame4[8];
+    frame4[0] = payload[7];
+    frame4[1] = 0x00;
+    frame4[2] = 0x00;
+    frame4[3] = 0x00;
+    frame4[4] = 0x00;
+    frame4[5] = crc_hi; // Same CRC: same payload+padding.
+    frame4[6] = crc_lo;
+    frame4[7] = make_v1_tail(false, true, false, 3U);
+
+    now_val = 1000;
+    TEST_ASSERT_TRUE(
+      canard_ingest_frame(&self, 1000, 0U, can_id, canard_bytes_t{ .size = sizeof(frame3), .data = frame3 }));
+    TEST_ASSERT_TRUE(
+      canard_ingest_frame(&self, 1100, 0U, can_id, canard_bytes_t{ .size = sizeof(frame4), .data = frame4 }));
+    TEST_ASSERT_EQUAL_size_t(1U, cap.count);
+    // With extent=100, all 12 bytes (8 payload + 4 padding) are delivered: min(14-2, 100) = 12.
+    TEST_ASSERT_EQUAL_size_t(12U, cap.payload_size);
+    TEST_ASSERT_EQUAL_UINT8_ARRAY(payload, cap.payload_buf, 8U);
+
+    canard_unsubscribe(&self, &sub);
+    canard_destroy(&self);
+}
+
+/// Shrink extent while a multiframe transfer is in flight. The in-flight slot must use the original extent.
+static void test_rx_extent_shrink_during_inflight()
+{
+    canard_t    self    = {};
+    canard_us_t now_val = 0;
+    init_canard(&self, &now_val, 42U);
+
+    rx_capture_t          cap = {};
+    canard_subscription_t sub = {};
+    TEST_ASSERT_TRUE(canard_subscribe_16b(&self, &sub, 1600U, 100U, 2000000, &capture_sub_vtable));
+    sub.user_context = &cap;
+
+    const uint_least8_t payload[8] = { 0xB0, 0xB1, 0xB2, 0xB3, 0xB4, 0xB5, 0xB6, 0xB7 };
+    const uint32_t      can_id     = make_v1v1_msg_can_id(canard_prio_nominal, 1600U, 10U);
+
+    const uint_least8_t pad[4] = {};
+    uint16_t            crc    = crc16_ccitt(0xFFFFU, payload, 8U);
+    crc                        = crc16_ccitt(crc, pad, 4U);
+    const auto crc_hi          = static_cast<uint_least8_t>((static_cast<unsigned>(crc) >> 8U) & 0xFFU);
+    const auto crc_lo          = static_cast<uint_least8_t>(crc & 0xFFU);
+
+    uint_least8_t frame1[8];
+    std::memcpy(frame1, payload, 7U);
+    frame1[7] = make_v1_tail(true, false, true, 4U);
+
+    // Feed frame 1, then shrink extent.
+    now_val = 100;
+    TEST_ASSERT_TRUE(
+      canard_ingest_frame(&self, 100, 0U, can_id, canard_bytes_t{ .size = sizeof(frame1), .data = frame1 }));
+
+    sub.extent = 4; // Shrink from 100 to 4.
+
+    uint_least8_t frame2[8];
+    frame2[0] = payload[7];
+    frame2[1] = 0x00;
+    frame2[2] = 0x00;
+    frame2[3] = 0x00;
+    frame2[4] = 0x00;
+    frame2[5] = crc_hi;
+    frame2[6] = crc_lo;
+    frame2[7] = make_v1_tail(false, true, false, 4U);
+
+    TEST_ASSERT_TRUE(
+      canard_ingest_frame(&self, 200, 0U, can_id, canard_bytes_t{ .size = sizeof(frame2), .data = frame2 }));
+    TEST_ASSERT_EQUAL_size_t(1U, cap.count);
+    // The slot was allocated with extent=100. Truncation: min(14-2, 100) = 12.
+    TEST_ASSERT_EQUAL_size_t(12U, cap.payload_size);
+    TEST_ASSERT_EQUAL_UINT8_ARRAY(payload, cap.payload_buf, 8U);
+
+    canard_unsubscribe(&self, &sub);
+    canard_destroy(&self);
+}
+
+/// Grow extent while a multiframe transfer is in flight. The in-flight slot must use the original (smaller) extent.
+static void test_rx_extent_grow_during_inflight()
+{
+    canard_t    self    = {};
+    canard_us_t now_val = 0;
+    init_canard(&self, &now_val, 42U);
+
+    rx_capture_t          cap = {};
+    canard_subscription_t sub = {};
+    TEST_ASSERT_TRUE(canard_subscribe_16b(&self, &sub, 1700U, 5U, 2000000, &capture_sub_vtable));
+    sub.user_context = &cap;
+
+    const uint_least8_t payload[8] = { 0xC0, 0xC1, 0xC2, 0xC3, 0xC4, 0xC5, 0xC6, 0xC7 };
+    const uint32_t      can_id     = make_v1v1_msg_can_id(canard_prio_nominal, 1700U, 10U);
+
+    const uint_least8_t pad[4] = {};
+    uint16_t            crc    = crc16_ccitt(0xFFFFU, payload, 8U);
+    crc                        = crc16_ccitt(crc, pad, 4U);
+    const auto crc_hi          = static_cast<uint_least8_t>((static_cast<unsigned>(crc) >> 8U) & 0xFFU);
+    const auto crc_lo          = static_cast<uint_least8_t>(crc & 0xFFU);
+
+    uint_least8_t frame1[8];
+    std::memcpy(frame1, payload, 7U);
+    frame1[7] = make_v1_tail(true, false, true, 5U);
+
+    // Feed frame 1, then grow extent.
+    now_val = 100;
+    TEST_ASSERT_TRUE(
+      canard_ingest_frame(&self, 100, 0U, can_id, canard_bytes_t{ .size = sizeof(frame1), .data = frame1 }));
+
+    sub.extent = 256; // Grow from 5 to 256.
+
+    uint_least8_t frame2[8];
+    frame2[0] = payload[7];
+    frame2[1] = 0x00;
+    frame2[2] = 0x00;
+    frame2[3] = 0x00;
+    frame2[4] = 0x00;
+    frame2[5] = crc_hi;
+    frame2[6] = crc_lo;
+    frame2[7] = make_v1_tail(false, true, false, 5U);
+
+    TEST_ASSERT_TRUE(
+      canard_ingest_frame(&self, 200, 0U, can_id, canard_bytes_t{ .size = sizeof(frame2), .data = frame2 }));
+    TEST_ASSERT_EQUAL_size_t(1U, cap.count);
+    // The slot was allocated with extent=5. Truncation: min(14-2, 5) = 5.
+    TEST_ASSERT_EQUAL_size_t(5U, cap.payload_size);
+    TEST_ASSERT_EQUAL_UINT8(0xC0, cap.payload_buf[0]);
+    TEST_ASSERT_EQUAL_UINT8(0xC4, cap.payload_buf[4]);
+
+    canard_unsubscribe(&self, &sub);
+    canard_destroy(&self);
+}
+
 // -------------------------------------------  Harness  ---------------------------------------------------------------
 
 extern "C" void setUp() {}
@@ -1354,5 +1540,10 @@ int main()
     RUN_TEST(test_rx_v0_extent_excludes_crc);
     RUN_TEST(test_rx_v0_extent_truncation);
 
+    // Dynamic extent.
+    RUN_TEST(test_rx_extent_change_between_transfers);
+    RUN_TEST(test_rx_extent_shrink_during_inflight);
+    RUN_TEST(test_rx_extent_grow_during_inflight);
+
     return UNITY_END();
 }