Skip to content

Commit

Permalink
Added new CanardRxTransfer::allocated_size
Browse files Browse the repository at this point in the history
  • Loading branch information
@serges147
serges147 committed Nov 15, 2024
1 parent e17c1f6 commit b857b90
Show file tree
Hide file tree
Showing 5 changed files with 55 additions and 20 deletions.
2 changes: 2 additions & 0 deletions libcanard/canard.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -790,6 +790,7 @@ CANARD_PRIVATE int8_t rxSessionAcceptFrame(CanardInstance* const ins,
out_transfer->timestamp_usec = rxs->transfer_timestamp_usec;
out_transfer->payload_size = rxs->payload_size;
out_transfer->payload = rxs->payload;
out_transfer->allocated_size = (rxs->payload != NULL) ? extent : 0U;

// Cut off the CRC from the payload if it's there -- we don't want to expose it to the user.
CANARD_ASSERT(rxs->total_payload_size >= rxs->payload_size);
Expand Down Expand Up @@ -983,6 +984,7 @@ CANARD_PRIVATE int8_t rxAcceptFrame(CanardInstance* const ins,
out_transfer->timestamp_usec = frame->timestamp_usec;
out_transfer->payload_size = payload_size;
out_transfer->payload = payload;
out_transfer->allocated_size = subscription->extent;
// Clang-Tidy raises an error recommending the use of memcpy_s() instead.
// We ignore it because the safe functions are poorly supported; reliance on them may limit the portability.
(void) memcpy(payload, frame->payload, payload_size); // NOLINT
Expand Down
12 changes: 10 additions & 2 deletions libcanard/canard.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -336,10 +336,18 @@ typedef struct CanardRxTransfer
/// The time system may be arbitrary as long as the clock is monotonic (steady).
CanardMicrosecond timestamp_usec;

/// Size of the payload data in bytes.
/// The value is always less than or equal to the extent specified in the subscription.
/// If the payload is empty (payload_size = 0), the payload pointer may be NULL.
/// The application is required to deallocate the payload buffer after the transfer is processed.
size_t payload_size;
void* payload;

/// The application is required to deallocate the payload buffer after the transfer is processed.
/// Allocated buffer size (`allocated_size`, not `payload_size`) should be used to deallocate the buffer.
void* payload;

/// Size of the allocated payload buffer in bytes.
/// Normally equal to the extent specified in the subscription, but may be zero if the payload pointer is NULL.
size_t allocated_size;
} CanardRxTransfer;

/// A pointer to the memory allocation function. The semantics are similar to malloc():
Expand Down
21 changes: 14 additions & 7 deletions tests/test_private_rx.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -340,10 +340,11 @@ TEST_CASE("rxSessionUpdate")
REQUIRE(transfer.metadata.remote_node_id == 55);
REQUIRE(transfer.metadata.transfer_id == 11);
REQUIRE(transfer.payload_size == 3);
REQUIRE(transfer.allocated_size == 16);
REQUIRE(0 == std::memcmp(transfer.payload, "\x01\x01\x01", 3));
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);
ins.getAllocator().deallocate(transfer.payload, 16);
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);

// Valid next transfer, wrong transport.
frame.timestamp_usec = 10'000'100;
Expand Down Expand Up @@ -376,10 +377,11 @@ TEST_CASE("rxSessionUpdate")
REQUIRE(transfer.metadata.remote_node_id == 55);
REQUIRE(transfer.metadata.transfer_id == 12);
REQUIRE(transfer.payload_size == 3);
REQUIRE(transfer.allocated_size == 16);
REQUIRE(0 == std::memcmp(transfer.payload, "\x03\x03\x03", 3));
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);
ins.getAllocator().deallocate(transfer.payload, 16);
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);

// Same TID.
frame.timestamp_usec = 10'000'200;
Expand Down Expand Up @@ -413,10 +415,11 @@ TEST_CASE("rxSessionUpdate")
REQUIRE(transfer.metadata.remote_node_id == 55);
REQUIRE(transfer.metadata.transfer_id == 12);
REQUIRE(transfer.payload_size == 3);
REQUIRE(transfer.allocated_size == 16);
REQUIRE(0 == std::memcmp(transfer.payload, "\x05\x05\x05", 3));
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);
ins.getAllocator().deallocate(transfer.payload, 16);
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);

// Restart due to TID timeout, switch iface.
frame.timestamp_usec = 20'000'000;
Expand All @@ -437,10 +440,11 @@ TEST_CASE("rxSessionUpdate")
REQUIRE(transfer.metadata.remote_node_id == 55);
REQUIRE(transfer.metadata.transfer_id == 11);
REQUIRE(transfer.payload_size == 3);
REQUIRE(transfer.allocated_size == 16);
REQUIRE(0 == std::memcmp(transfer.payload, "\x05\x05\x05", 3));
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);
ins.getAllocator().deallocate(transfer.payload, 16);
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);

// Multi-frame, first.
frame.timestamp_usec = 20'000'100;
Expand Down Expand Up @@ -515,10 +519,11 @@ TEST_CASE("rxSessionUpdate")
REQUIRE(transfer.metadata.remote_node_id == 55);
REQUIRE(transfer.metadata.transfer_id == 13);
REQUIRE(transfer.payload_size == 16);
REQUIRE(transfer.allocated_size == 16);
REQUIRE(0 == std::memcmp(transfer.payload, "\x06\x06\x06\x06\x06\x06\x06\x07\x07\x07\x07\x07\x07\x07\x09\x09", 16));
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);
ins.getAllocator().deallocate(transfer.payload, 16);
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);

// TID timeout does not occur until SOT; see https://github.com/OpenCyphal/libcanard/issues/212.
frame.timestamp_usec = 30'000'000;
Expand Down Expand Up @@ -616,10 +621,11 @@ TEST_CASE("rxSessionUpdate")
REQUIRE(transfer.metadata.remote_node_id == 55);
REQUIRE(transfer.metadata.transfer_id == 10);
REQUIRE(transfer.payload_size == 10);
REQUIRE(transfer.allocated_size == 16);
REQUIRE(0 == std::memcmp(transfer.payload, "\x0B\x0B\x0B\x0B\x0B\x0B\x0B\x0D\x0D\x0D", 10));
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);
ins.getAllocator().deallocate(transfer.payload, 16);
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);

// CRC SPLIT -- first frame.
frame.timestamp_usec = 30'000'000;
Expand Down Expand Up @@ -663,10 +669,11 @@ TEST_CASE("rxSessionUpdate")
REQUIRE(transfer.metadata.remote_node_id == 55);
REQUIRE(transfer.metadata.transfer_id == 0);
REQUIRE(transfer.payload_size == 7); // ONE CRC BYTE BACKTRACKED!
REQUIRE(transfer.allocated_size == 16);
REQUIRE(0 == std::memcmp(transfer.payload, "\x0E\x0E\x0E\x0E\x0E\x0E\x0E", 7));
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);
ins.getAllocator().deallocate(transfer.payload, 16);
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);

// BAD CRC -- first frame.
frame.timestamp_usec = 30'001'000;
Expand Down
3 changes: 2 additions & 1 deletion tests/test_public_roundtrip.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -191,9 +191,10 @@ TEST_CASE("RoundtripSimple")
else
{
REQUIRE(transfer.payload_size == 0U);
REQUIRE(transfer.allocated_size == 0U);
}

ins_rx.getAllocator().deallocate(transfer.payload, subscription->extent);
ins_rx.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
std::free(ref_payload); // NOLINT
}
else
Expand Down
37 changes: 27 additions & 10 deletions tests/test_public_rx.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -118,11 +118,16 @@ TEST_CASE("RxBasic0")
REQUIRE(transfer.metadata.remote_node_id == 0b0100111);
REQUIRE(transfer.metadata.transfer_id == 0);
REQUIRE(transfer.payload_size == 0);
REQUIRE(transfer.allocated_size == 16);
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.getMessageSubs().at(0)->sessions[0b0100111] != nullptr);
auto* msg_payload = transfer.payload; // Will need it later.

// Will need these later.
//
auto* const msg_payload = transfer.payload;
const auto payload_alloc_size = transfer.allocated_size;

// Provide the space for an extra session and its payload.
ins.getAllocator().setAllocationCeiling(sizeof(RxSession) * 2 + 16 + 20);
Expand Down Expand Up @@ -150,6 +155,7 @@ TEST_CASE("RxBasic0")
REQUIRE(transfer.metadata.remote_node_id == 0b0100101);
REQUIRE(transfer.metadata.transfer_id == 4);
REQUIRE(transfer.payload_size == 3);
REQUIRE(transfer.allocated_size == 20);
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));
Expand Down Expand Up @@ -188,7 +194,7 @@ TEST_CASE("RxBasic0")
REQUIRE(nullptr == ins.rxGetSubscription(CanardTransferKindMessage, 0b0110011001100));
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 4);
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (2 * sizeof(RxSession) + 16 + 20));
ins.getAllocator().deallocate(msg_payload, 16);
ins.getAllocator().deallocate(msg_payload, payload_alloc_size);
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 3);
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (2 * sizeof(RxSession) + 20));

Expand All @@ -204,6 +210,7 @@ TEST_CASE("RxBasic0")
REQUIRE(transfer.metadata.remote_node_id == 0b0011011);
REQUIRE(transfer.metadata.transfer_id == 3);
REQUIRE(transfer.payload_size == 1);
REQUIRE(transfer.allocated_size == 10);
REQUIRE(0 == std::memcmp(transfer.payload, "\x05", 1));
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 4);
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (2 * sizeof(RxSession) + 10 + 20));
Expand Down Expand Up @@ -298,6 +305,7 @@ TEST_CASE("RxAnonymous")
REQUIRE(transfer.metadata.remote_node_id == CANARD_NODE_ID_UNSET);
REQUIRE(transfer.metadata.transfer_id == 0);
REQUIRE(transfer.payload_size == 16); // Truncated.
REQUIRE(transfer.allocated_size == 16);
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);
Expand All @@ -315,13 +323,14 @@ TEST_CASE("RxAnonymous")
REQUIRE(transfer.metadata.remote_node_id == CANARD_NODE_ID_UNSET);
REQUIRE(transfer.metadata.transfer_id == 0);
REQUIRE(transfer.payload_size == 16); // Truncated.
REQUIRE(transfer.allocated_size == 16);
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.getMessageSubs().at(0)->sessions)); // No RX states!

// Release the memory.
ins.getAllocator().deallocate(transfer.payload, subscription->extent);
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 0);
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 0);

Expand All @@ -337,6 +346,7 @@ TEST_CASE("RxAnonymous")
REQUIRE(transfer.metadata.remote_node_id == CANARD_NODE_ID_UNSET);
REQUIRE(transfer.metadata.transfer_id == 0);
REQUIRE(transfer.payload_size == 6); // NOT truncated.
REQUIRE(transfer.allocated_size == 16);
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.
Expand Down Expand Up @@ -436,11 +446,12 @@ TEST_CASE("Issue189") // https://github.com/OpenCyphal/libcanard/issues/189
REQUIRE(transfer.metadata.remote_node_id == 0b0100111);
REQUIRE(transfer.metadata.transfer_id == 0);
REQUIRE(transfer.payload_size == 1);
REQUIRE(transfer.allocated_size == 50);
REQUIRE(0 == std::memcmp(transfer.payload, "\x42", 1));
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 2); // The SESSION and the PAYLOAD BUFFER.
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (sizeof(RxSession) + 50));
REQUIRE(ins.getMessageSubs().at(0)->sessions[0b0100111] != nullptr);
ins.getAllocator().deallocate(transfer.payload, subscription->extent);
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); // The payload buffer is gone.
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == sizeof(RxSession));

Expand Down Expand Up @@ -484,14 +495,15 @@ TEST_CASE("Issue189") // https://github.com/OpenCyphal/libcanard/issues/189
REQUIRE(transfer.metadata.remote_node_id == 0b0100111);
REQUIRE(transfer.metadata.transfer_id == 2);
REQUIRE(transfer.payload_size == 11);
REQUIRE(transfer.allocated_size == 50);
REQUIRE(0 == std::memcmp(transfer.payload,
"\x01\x02\x03\x04\x05\x06\x07"
"DUCK",
11));
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 2); // The SESSION and the PAYLOAD BUFFER.
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (sizeof(RxSession) + 50));
REQUIRE(ins.getMessageSubs().at(0)->sessions[0b0100111] != nullptr);
ins.getAllocator().deallocate(transfer.payload, subscription->extent);
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); // The payload buffer is gone.
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == sizeof(RxSession));
}
Expand Down Expand Up @@ -561,11 +573,12 @@ TEST_CASE("Issue212")
REQUIRE(transfer.metadata.remote_node_id == 0b0100111);
REQUIRE(transfer.metadata.transfer_id == 2);
REQUIRE(transfer.payload_size == 14);
REQUIRE(transfer.allocated_size == 50);
REQUIRE(0 == std::memcmp(transfer.payload, "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0A\x0B\x0C\x0D\x0E", 14));
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 2); // The SESSION and the PAYLOAD BUFFER.
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (sizeof(RxSession) + 50));
REQUIRE(ins.getMessageSubs().at(0)->sessions[0b0100111] != nullptr);
ins.getAllocator().deallocate(transfer.payload, subscription->extent);
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); // The payload buffer is gone.
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == sizeof(RxSession));

Expand Down Expand Up @@ -595,11 +608,12 @@ TEST_CASE("Issue212")
REQUIRE(transfer.metadata.remote_node_id == 0b0100111);
REQUIRE(transfer.metadata.transfer_id == 3);
REQUIRE(transfer.payload_size == 14);
REQUIRE(transfer.allocated_size == 50);
REQUIRE(0 == std::memcmp(transfer.payload, "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0A\x0B\x0C\x0D\x0E", 14));
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 2); // The SESSION and the PAYLOAD BUFFER.
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (sizeof(RxSession) + 50));
REQUIRE(ins.getMessageSubs().at(0)->sessions[0b0100111] != nullptr);
ins.getAllocator().deallocate(transfer.payload, subscription->extent);
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); // The payload buffer is gone.
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == sizeof(RxSession));
}
Expand Down Expand Up @@ -664,11 +678,12 @@ TEST_CASE("RxFixedTIDWithSmallTimeout")
REQUIRE(transfer.metadata.remote_node_id == 0b0100111);
REQUIRE(transfer.metadata.transfer_id == 0);
REQUIRE(transfer.payload_size == 14);
REQUIRE(transfer.allocated_size == 50);
REQUIRE(0 == std::memcmp(transfer.payload, "\x01\x02\x03\x04\x05\x06\x07\x08\x09\x0A\x0B\x0C\x0D\x0E", 14));
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 2); // The SESSION and the PAYLOAD BUFFER.
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (sizeof(RxSession) + 50));
REQUIRE(ins.getMessageSubs().at(0)->sessions[0b0100111] != nullptr);
ins.getAllocator().deallocate(transfer.payload, subscription->extent);
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); // The payload buffer is gone.
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == sizeof(RxSession));

Expand All @@ -688,11 +703,12 @@ TEST_CASE("RxFixedTIDWithSmallTimeout")
REQUIRE(transfer.metadata.remote_node_id == 0b0100111);
REQUIRE(transfer.metadata.transfer_id == 0); // same
REQUIRE(transfer.payload_size == 8);
REQUIRE(transfer.allocated_size == 50);
REQUIRE(0 == std::memcmp(transfer.payload, "\x01\x02\x03\x04\x05\x06\x07\x08", 8));
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 2); // The SESSION and the PAYLOAD BUFFER.
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (sizeof(RxSession) + 50));
REQUIRE(ins.getMessageSubs().at(0)->sessions[0b0100111] != nullptr);
ins.getAllocator().deallocate(transfer.payload, subscription->extent);
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); // The payload buffer is gone.
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == sizeof(RxSession));

Expand All @@ -709,11 +725,12 @@ TEST_CASE("RxFixedTIDWithSmallTimeout")
REQUIRE(transfer.metadata.remote_node_id == 0b0100111);
REQUIRE(transfer.metadata.transfer_id == 0); // same
REQUIRE(transfer.payload_size == 7);
REQUIRE(transfer.allocated_size == 50);
REQUIRE(0 == std::memcmp(transfer.payload, "\x01\x02\x03\x04\x05\x06\x07", 7));
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 2); // The SESSION and the PAYLOAD BUFFER.
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == (sizeof(RxSession) + 50));
REQUIRE(ins.getMessageSubs().at(0)->sessions[0b0100111] != nullptr);
ins.getAllocator().deallocate(transfer.payload, subscription->extent);
ins.getAllocator().deallocate(transfer.payload, transfer.allocated_size);
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1); // The payload buffer is gone.
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == sizeof(RxSession));
}

0 comments on commit b857b90

Please sign in to comment.