From a1efa3a1f19baef54764ef7c4cecb7b68db88f99 Mon Sep 17 00:00:00 2001 From: Peter van der Perk <57130844+PetervdPerk-NXP@users.noreply.github.com> Date: Thu, 20 May 2021 15:33:24 +0200 Subject: [PATCH] Expose CanardRxSubscription fields for read-only use (#167) --- libcanard/canard.c | 54 +++++++++++------------ libcanard/canard.h | 14 +++--- tests/test_public_rx.cpp | 94 ++++++++++++++++++++-------------------- 3 files changed, 82 insertions(+), 80 deletions(-) diff --git a/libcanard/canard.c b/libcanard/canard.c index bc6bf57..79a88e0 100644 --- a/libcanard/canard.c +++ b/libcanard/canard.c @@ -792,7 +792,7 @@ CANARD_PRIVATE int8_t rxAcceptFrame(CanardInstance* const ins, { CANARD_ASSERT(ins != NULL); CANARD_ASSERT(subscription != NULL); - CANARD_ASSERT(subscription->_port_id == frame->port_id); + CANARD_ASSERT(subscription->port_id == frame->port_id); CANARD_ASSERT(frame != NULL); CANARD_ASSERT(frame->payload != NULL); CANARD_ASSERT(frame->transfer_id <= CANARD_TRANSFER_ID_MAX); @@ -833,8 +833,8 @@ CANARD_PRIVATE int8_t rxAcceptFrame(CanardInstance* const ins, subscription->_sessions[frame->source_node_id], frame, redundant_transport_index, - subscription->_transfer_id_timeout_usec, - subscription->_extent, + subscription->transfer_id_timeout_usec, + subscription->extent, out_transfer); } } @@ -845,7 +845,7 @@ CANARD_PRIVATE int8_t rxAcceptFrame(CanardInstance* const ins, // We have to copy the data into an allocated storage because the API expects it: the lifetime shall be // independent of the input data and the memory shall be free-able. const size_t payload_size = - (subscription->_extent < frame->payload_size) ? subscription->_extent : frame->payload_size; + (subscription->extent < frame->payload_size) ? subscription->extent : frame->payload_size; void* const payload = ins->memory_allocate(ins, payload_size); if (payload != NULL) { @@ -884,13 +884,13 @@ CanardInstance canardInit(const CanardMemoryAllocate memory_allocate, const Cana CANARD_ASSERT(memory_allocate != NULL); CANARD_ASSERT(memory_free != NULL); const CanardInstance out = { - .user_reference = NULL, - .mtu_bytes = CANARD_MTU_CAN_FD, - .node_id = CANARD_NODE_ID_UNSET, - .memory_allocate = memory_allocate, - .memory_free = memory_free, - ._rx_subscriptions = {NULL, NULL, NULL}, - ._tx_queue = NULL, + .user_reference = NULL, + .mtu_bytes = CANARD_MTU_CAN_FD, + .node_id = CANARD_NODE_ID_UNSET, + .memory_allocate = memory_allocate, + .memory_free = memory_free, + .rx_subscriptions = {NULL, NULL, NULL}, + ._tx_queue = NULL, }; return out; } @@ -977,10 +977,10 @@ int8_t canardRxAccept2(CanardInstance* const ins, // 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 logic contains neither // loops nor recursion. - CanardRxSubscription* sub = ins->_rx_subscriptions[(size_t) model.transfer_kind]; - while ((sub != NULL) && (sub->_port_id != model.port_id)) + CanardRxSubscription* sub = ins->rx_subscriptions[(size_t) model.transfer_kind]; + while ((sub != NULL) && (sub->port_id != model.port_id)) { - sub = sub->_next; + sub = sub->next; } if (out_subscription != NULL) @@ -990,7 +990,7 @@ int8_t canardRxAccept2(CanardInstance* const ins, if (sub != NULL) { - CANARD_ASSERT(sub->_port_id == model.port_id); + CANARD_ASSERT(sub->port_id == model.port_id); out = rxAcceptFrame(ins, sub, &model, redundant_transport_index, out_transfer); } else @@ -1044,12 +1044,12 @@ int8_t canardRxSubscribe(CanardInstance* const ins, // We could accept an extra argument that would instruct us to pre-allocate sessions here? out_subscription->_sessions[i] = NULL; } - out_subscription->_transfer_id_timeout_usec = transfer_id_timeout_usec; - out_subscription->_extent = extent; - out_subscription->_port_id = port_id; - out_subscription->_next = ins->_rx_subscriptions[tk]; - ins->_rx_subscriptions[tk] = out_subscription; - out = (out > 0) ? 0 : 1; + out_subscription->transfer_id_timeout_usec = transfer_id_timeout_usec; + out_subscription->extent = extent; + out_subscription->port_id = port_id; + out_subscription->next = ins->rx_subscriptions[tk]; + ins->rx_subscriptions[tk] = out_subscription; + out = (out > 0) ? 0 : 1; } } return out; @@ -1064,25 +1064,25 @@ int8_t canardRxUnsubscribe(CanardInstance* const ins, if ((ins != NULL) && (tk < CANARD_NUM_TRANSFER_KINDS)) { CanardRxSubscription* prv = NULL; - CanardRxSubscription* sub = ins->_rx_subscriptions[tk]; - while ((sub != NULL) && (sub->_port_id != port_id)) + CanardRxSubscription* sub = ins->rx_subscriptions[tk]; + while ((sub != NULL) && (sub->port_id != port_id)) { prv = sub; - sub = sub->_next; + sub = sub->next; } if (sub != NULL) { - CANARD_ASSERT(sub->_port_id == port_id); + CANARD_ASSERT(sub->port_id == port_id); out = 1; if (prv != NULL) { - prv->_next = sub->_next; + prv->next = sub->next; } else { - ins->_rx_subscriptions[tk] = sub->_next; + ins->rx_subscriptions[tk] = sub->next; } for (size_t i = 0; i < RX_SESSIONS_PER_SUBSCRIPTION; i++) diff --git a/libcanard/canard.h b/libcanard/canard.h index 97cd9d9..8d1916f 100644 --- a/libcanard/canard.h +++ b/libcanard/canard.h @@ -268,7 +268,7 @@ typedef struct /// This is an intentional time-memory trade-off: use a large look-up table to ensure predictable temporal properties. typedef struct CanardRxSubscription { - struct CanardRxSubscription* _next; ///< Internal use only. + struct CanardRxSubscription* next; ///< Read-only DO NOT MODIFY THIS /// The current architecture is an acceptable middle ground between worst-case execution time and memory /// consumption. Instead of statically pre-allocating a dedicated RX session for each remote node-ID here in @@ -285,9 +285,9 @@ typedef struct CanardRxSubscription /// but more memory-efficient approach. struct CanardInternalRxSession* _sessions[CANARD_NODE_ID_MAX + 1U]; - CanardMicrosecond _transfer_id_timeout_usec; ///< Internal use only. - size_t _extent; ///< Internal use only. - CanardPortID _port_id; ///< Internal use only. + CanardMicrosecond transfer_id_timeout_usec; ///< Read-only DO NOT MODIFY THIS + size_t extent; ///< Read-only DO NOT MODIFY THIS + CanardPortID port_id; ///< Read-only DO NOT MODIFY THIS /// 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. @@ -347,8 +347,10 @@ struct CanardInstance CanardMemoryAllocate memory_allocate; CanardMemoryFree memory_free; - /// These fields are for internal use only. Do not access from the application. - CanardRxSubscription* _rx_subscriptions[CANARD_NUM_TRANSFER_KINDS]; + /// Read-only DO NOT MODIFY THIS + CanardRxSubscription* rx_subscriptions[CANARD_NUM_TRANSFER_KINDS]; + + /// This field is for internal use only. Do not access from the application. struct CanardInternalTxQueueItem* _tx_queue; }; diff --git a/tests/test_public_rx.cpp b/tests/test_public_rx.cpp index ec77792..fc2b016 100644 --- a/tests/test_public_rx.cpp +++ b/tests/test_public_rx.cpp @@ -38,9 +38,9 @@ TEST_CASE("RxBasic0") ins.getAllocator().setAllocationCeiling(sizeof(RxSession) + 16); // A session and a 16-byte payload buffer. // No subscriptions by default. - REQUIRE(ins.getInstance()._rx_subscriptions[0] == nullptr); - REQUIRE(ins.getInstance()._rx_subscriptions[1] == nullptr); - REQUIRE(ins.getInstance()._rx_subscriptions[2] == nullptr); + REQUIRE(ins.getInstance().rx_subscriptions[0] == nullptr); + REQUIRE(ins.getInstance().rx_subscriptions[1] == nullptr); + REQUIRE(ins.getInstance().rx_subscriptions[2] == nullptr); // A valid single-frame transfer for which there is no subscription. subscription = nullptr; @@ -51,56 +51,56 @@ TEST_CASE("RxBasic0") CanardRxSubscription sub_msg{}; REQUIRE(1 == ins.rxSubscribe(CanardTransferKindMessage, 0b0110011001100, 32, 2'000'000, sub_msg)); // New. REQUIRE(0 == ins.rxSubscribe(CanardTransferKindMessage, 0b0110011001100, 16, 1'000'000, sub_msg)); // Replaced. - REQUIRE(ins.getInstance()._rx_subscriptions[0] == &sub_msg); - REQUIRE(ins.getInstance()._rx_subscriptions[0]->_next == nullptr); - REQUIRE(ins.getInstance()._rx_subscriptions[0]->_port_id == 0b0110011001100); - REQUIRE(ins.getInstance()._rx_subscriptions[0]->_extent == 16); - REQUIRE(ins.getInstance()._rx_subscriptions[0]->_transfer_id_timeout_usec == 1'000'000); - REQUIRE(ensureAllNullptr(ins.getInstance()._rx_subscriptions[0]->_sessions)); - REQUIRE(ins.getInstance()._rx_subscriptions[1] == nullptr); - REQUIRE(ins.getInstance()._rx_subscriptions[2] == nullptr); + REQUIRE(ins.getInstance().rx_subscriptions[0] == &sub_msg); + REQUIRE(ins.getInstance().rx_subscriptions[0]->next == nullptr); + REQUIRE(ins.getInstance().rx_subscriptions[0]->port_id == 0b0110011001100); + REQUIRE(ins.getInstance().rx_subscriptions[0]->extent == 16); + REQUIRE(ins.getInstance().rx_subscriptions[0]->transfer_id_timeout_usec == 1'000'000); + REQUIRE(ensureAllNullptr(ins.getInstance().rx_subscriptions[0]->_sessions)); + REQUIRE(ins.getInstance().rx_subscriptions[1] == nullptr); + REQUIRE(ins.getInstance().rx_subscriptions[2] == nullptr); // Create a request subscription. CanardRxSubscription sub_req{}; REQUIRE(1 == ins.rxSubscribe(CanardTransferKindRequest, 0b0000110011, 20, 3'000'000, sub_req)); - REQUIRE(ins.getInstance()._rx_subscriptions[0] == &sub_msg); - REQUIRE(ins.getInstance()._rx_subscriptions[1] == nullptr); - REQUIRE(ins.getInstance()._rx_subscriptions[2] == &sub_req); - REQUIRE(ins.getInstance()._rx_subscriptions[2]->_next == nullptr); - REQUIRE(ins.getInstance()._rx_subscriptions[2]->_port_id == 0b0000110011); - REQUIRE(ins.getInstance()._rx_subscriptions[2]->_extent == 20); - REQUIRE(ins.getInstance()._rx_subscriptions[2]->_transfer_id_timeout_usec == 3'000'000); - REQUIRE(ensureAllNullptr(ins.getInstance()._rx_subscriptions[2]->_sessions)); + REQUIRE(ins.getInstance().rx_subscriptions[0] == &sub_msg); + REQUIRE(ins.getInstance().rx_subscriptions[1] == nullptr); + REQUIRE(ins.getInstance().rx_subscriptions[2] == &sub_req); + REQUIRE(ins.getInstance().rx_subscriptions[2]->next == nullptr); + REQUIRE(ins.getInstance().rx_subscriptions[2]->port_id == 0b0000110011); + REQUIRE(ins.getInstance().rx_subscriptions[2]->extent == 20); + REQUIRE(ins.getInstance().rx_subscriptions[2]->transfer_id_timeout_usec == 3'000'000); + REQUIRE(ensureAllNullptr(ins.getInstance().rx_subscriptions[2]->_sessions)); // Create a response subscription. CanardRxSubscription sub_res{}; REQUIRE(1 == ins.rxSubscribe(CanardTransferKindResponse, 0b0000111100, 10, 100'000, sub_res)); - REQUIRE(ins.getInstance()._rx_subscriptions[0] == &sub_msg); - REQUIRE(ins.getInstance()._rx_subscriptions[1] == &sub_res); - REQUIRE(ins.getInstance()._rx_subscriptions[1]->_next == nullptr); - REQUIRE(ins.getInstance()._rx_subscriptions[1]->_port_id == 0b0000111100); - REQUIRE(ins.getInstance()._rx_subscriptions[1]->_extent == 10); - REQUIRE(ins.getInstance()._rx_subscriptions[1]->_transfer_id_timeout_usec == 100'000); - REQUIRE(ensureAllNullptr(ins.getInstance()._rx_subscriptions[1]->_sessions)); - REQUIRE(ins.getInstance()._rx_subscriptions[2] == &sub_req); + REQUIRE(ins.getInstance().rx_subscriptions[0] == &sub_msg); + REQUIRE(ins.getInstance().rx_subscriptions[1] == &sub_res); + REQUIRE(ins.getInstance().rx_subscriptions[1]->next == nullptr); + REQUIRE(ins.getInstance().rx_subscriptions[1]->port_id == 0b0000111100); + REQUIRE(ins.getInstance().rx_subscriptions[1]->extent == 10); + REQUIRE(ins.getInstance().rx_subscriptions[1]->transfer_id_timeout_usec == 100'000); + REQUIRE(ensureAllNullptr(ins.getInstance().rx_subscriptions[1]->_sessions)); + REQUIRE(ins.getInstance().rx_subscriptions[2] == &sub_req); // Create a second response subscription. CanardRxSubscription sub_res2{}; REQUIRE(1 == ins.rxSubscribe(CanardTransferKindResponse, 0b0000000000, 10, 1'000, sub_res2)); - REQUIRE(ins.getInstance()._rx_subscriptions[0] == &sub_msg); - REQUIRE(ins.getInstance()._rx_subscriptions[1] == &sub_res2); - REQUIRE(ins.getInstance()._rx_subscriptions[1]->_next == &sub_res); - REQUIRE(ins.getInstance()._rx_subscriptions[1]->_port_id == 0b0000000000); - REQUIRE(ins.getInstance()._rx_subscriptions[1]->_extent == 10); - REQUIRE(ins.getInstance()._rx_subscriptions[1]->_transfer_id_timeout_usec == 1'000); - REQUIRE(ensureAllNullptr(ins.getInstance()._rx_subscriptions[1]->_sessions)); - REQUIRE(ins.getInstance()._rx_subscriptions[2] == &sub_req); + REQUIRE(ins.getInstance().rx_subscriptions[0] == &sub_msg); + REQUIRE(ins.getInstance().rx_subscriptions[1] == &sub_res2); + REQUIRE(ins.getInstance().rx_subscriptions[1]->next == &sub_res); + REQUIRE(ins.getInstance().rx_subscriptions[1]->port_id == 0b0000000000); + REQUIRE(ins.getInstance().rx_subscriptions[1]->extent == 10); + REQUIRE(ins.getInstance().rx_subscriptions[1]->transfer_id_timeout_usec == 1'000); + REQUIRE(ensureAllNullptr(ins.getInstance().rx_subscriptions[1]->_sessions)); + REQUIRE(ins.getInstance().rx_subscriptions[2] == &sub_req); // Accepted message. subscription = nullptr; REQUIRE(1 == accept(0, 100'000'001, 0b001'00'0'11'0110011001100'0'0100111, {0b111'00000})); REQUIRE(subscription != nullptr); - REQUIRE(subscription->_port_id == 0b0110011001100); + REQUIRE(subscription->port_id == 0b0110011001100); REQUIRE(transfer.timestamp_usec == 100'000'001); REQUIRE(transfer.priority == CanardPriorityImmediate); REQUIRE(transfer.transfer_kind == CanardTransferKindMessage); @@ -111,7 +111,7 @@ TEST_CASE("RxBasic0") REQUIRE(0 == std::memcmp(transfer.payload, "", 0)); REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 2); // The SESSION and the PAYLOAD BUFFER. REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (sizeof(RxSession) + 16)); - REQUIRE(ins.getInstance()._rx_subscriptions[0]->_sessions[0b0100111] != nullptr); + REQUIRE(ins.getInstance().rx_subscriptions[0]->_sessions[0b0100111] != nullptr); const auto* msg_payload = transfer.payload; // Will need it later. // Provide the space for an extra session and its payload. @@ -132,7 +132,7 @@ TEST_CASE("RxBasic0") subscription = nullptr; REQUIRE(1 == accept(0, 100'000'002, 0b011'11'0000110011'0011010'0100101, {1, 2, 3, 0b111'00100})); REQUIRE(subscription != nullptr); - REQUIRE(subscription->_port_id == 0b0000110011); + REQUIRE(subscription->port_id == 0b0000110011); REQUIRE(transfer.timestamp_usec == 100'000'002); REQUIRE(transfer.priority == CanardPriorityHigh); REQUIRE(transfer.transfer_kind == CanardTransferKindRequest); @@ -143,7 +143,7 @@ TEST_CASE("RxBasic0") REQUIRE(0 == std::memcmp(transfer.payload, "\x01\x02\x03", 3)); REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 4); // Two SESSIONS and two PAYLOAD BUFFERS. REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (2 * sizeof(RxSession) + 16 + 20)); - REQUIRE(ins.getInstance()._rx_subscriptions[2]->_sessions[0b0100101] != nullptr); + REQUIRE(ins.getInstance().rx_subscriptions[2]->_sessions[0b0100101] != nullptr); // Response transfer not accepted because the local node has a different node-ID. // There is no dynamic memory available, but it doesn't matter because a rejection does not require allocation. @@ -181,7 +181,7 @@ TEST_CASE("RxBasic0") subscription = nullptr; REQUIRE(1 == accept(0, 100'000'003, 0b100'10'0000111100'0011010'0011011, {5, 0b111'00011})); REQUIRE(subscription != nullptr); - REQUIRE(subscription->_port_id == 0b0000111100); + REQUIRE(subscription->port_id == 0b0000111100); REQUIRE(transfer.timestamp_usec == 100'000'003); REQUIRE(transfer.priority == CanardPriorityNominal); REQUIRE(transfer.transfer_kind == CanardTransferKindResponse); @@ -192,7 +192,7 @@ TEST_CASE("RxBasic0") REQUIRE(0 == std::memcmp(transfer.payload, "\x05", 1)); REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 4); REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (2 * sizeof(RxSession) + 10 + 20)); - REQUIRE(ins.getInstance()._rx_subscriptions[1]->_next->_sessions[0b0011011] != nullptr); + REQUIRE(ins.getInstance().rx_subscriptions[1]->next->_sessions[0b0011011] != nullptr); // Bad frames shall be rejected silently. subscription = nullptr; @@ -252,7 +252,7 @@ TEST_CASE("RxAnonymous") 0b001'01'0'11'0110011001100'0'0100111, // {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 0b111'00000})); REQUIRE(subscription != nullptr); - REQUIRE(subscription->_port_id == 0b0110011001100); + REQUIRE(subscription->port_id == 0b0110011001100); REQUIRE(transfer.timestamp_usec == 100'000'001); REQUIRE(transfer.priority == CanardPriorityImmediate); REQUIRE(transfer.transfer_kind == CanardTransferKindMessage); @@ -263,13 +263,13 @@ TEST_CASE("RxAnonymous") REQUIRE(0 == std::memcmp(transfer.payload, "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0A\x0B\x0C\x0D\x0E\x0F\x10", 0)); REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); // The PAYLOAD BUFFER only! No session for anons. REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16); - REQUIRE(ensureAllNullptr(ins.getInstance()._rx_subscriptions[0]->_sessions)); // No RX states! + REQUIRE(ensureAllNullptr(ins.getInstance().rx_subscriptions[0]->_sessions)); // No RX states! // Anonymous message not accepted because OOM. The transfer shall remain unmodified by the call, so we re-check it. REQUIRE(-CANARD_ERROR_OUT_OF_MEMORY == accept(0, 100'000'001, 0b001'01'0'11'0110011001100'0'0100111, {3, 2, 1, 0b111'00000})); REQUIRE(subscription != nullptr); - REQUIRE(subscription->_port_id == 0b0110011001100); + REQUIRE(subscription->port_id == 0b0110011001100); REQUIRE(transfer.timestamp_usec == 100'000'001); REQUIRE(transfer.priority == CanardPriorityImmediate); REQUIRE(transfer.transfer_kind == CanardTransferKindMessage); @@ -280,7 +280,7 @@ TEST_CASE("RxAnonymous") REQUIRE(0 == std::memcmp(transfer.payload, "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0A\x0B\x0C\x0D\x0E\x0F\x10", 0)); REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); // The PAYLOAD BUFFER only! No session for anons. REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16); - REQUIRE(ensureAllNullptr(ins.getInstance()._rx_subscriptions[0]->_sessions)); // No RX states! + REQUIRE(ensureAllNullptr(ins.getInstance().rx_subscriptions[0]->_sessions)); // No RX states! // Release the memory. ins.getAllocator().deallocate(transfer.payload); @@ -291,7 +291,7 @@ TEST_CASE("RxAnonymous") subscription = nullptr; REQUIRE(1 == accept(0, 100'000'001, 0b001'01'0'11'0110011001100'0'0100111, {1, 2, 3, 4, 5, 6, 0b111'00000})); REQUIRE(subscription != nullptr); - REQUIRE(subscription->_port_id == 0b0110011001100); + REQUIRE(subscription->port_id == 0b0110011001100); REQUIRE(transfer.timestamp_usec == 100'000'001); REQUIRE(transfer.priority == CanardPriorityImmediate); REQUIRE(transfer.transfer_kind == CanardTransferKindMessage); @@ -302,7 +302,7 @@ TEST_CASE("RxAnonymous") REQUIRE(0 == std::memcmp(transfer.payload, "\x01\x02\x03\x04\x05\x06", 0)); REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); // The PAYLOAD BUFFER only! No session for anons. REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 6); // Smaller allocation. - REQUIRE(ensureAllNullptr(ins.getInstance()._rx_subscriptions[0]->_sessions)); // No RX states! + REQUIRE(ensureAllNullptr(ins.getInstance().rx_subscriptions[0]->_sessions)); // No RX states! } TEST_CASE("RxSubscriptionErrors")