Skip to content

Commit 6867f55

Browse files
keningatamazonkeningatamazon
committed
Implement CRC for single-frame transfer
1 parent ff804bf commit 6867f55

File tree

4 files changed

+78
-63
lines changed

4 files changed

+78
-63
lines changed

‎libudpard/udpard.c‎

Lines changed: 29 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,8 @@ typedef uint32_t TransferCRC;
126126
#define CRC_XOR 0xFFFFFFFFU
127127
#define CRC_SIZE_BYTES 4U
128128

129+
#define CRC_BYTE_MASK 0xFFU
130+
129131
static const uint32_t CRCTable[256] =
130132
{0x00000000, 0xf26b8303, 0xe13b70f7, 0x1350f3f4, 0xc79a971f, 0x35f1141c, 0x26a1e7e8, 0xd4ca64eb, 0x8ad958cf,
131133
0x78b2dbcc, 0x6be22838, 0x9989ab3b, 0x4d43cfd0, 0xbf284cd3, 0xac78bf27, 0x5e133c24, 0x105ec76f, 0xe235446c,
@@ -405,10 +407,8 @@ UDPARD_PRIVATE int32_t txPushSingleFrame(UdpardTxQueue* const que
405407
UDPARD_ASSERT((payload != NULL) || (payload_size == 0));
406408
// The size of a Frame header shouldn't change, but best to check it is at least bigger than 0
407409
UDPARD_ASSERT(sizeof(UdpardFrameHeader) > 0); // NOLINT
408-
const size_t frame_payload_size = payload_size + sizeof(UdpardFrameHeader);
410+
const size_t frame_payload_size = payload_size + sizeof(UdpardFrameHeader) + CRC_SIZE_BYTES;
409411
UDPARD_ASSERT(frame_payload_size > payload_size);
410-
const size_t padding_size = frame_payload_size - payload_size - sizeof(UdpardFrameHeader);
411-
UDPARD_ASSERT((padding_size + payload_size + sizeof(UdpardFrameHeader)) == frame_payload_size);
412412
int32_t out = 0;
413413
TxItem* const tqi =
414414
(que->size < que->capacity) ? txAllocateQueueItem(ins, specifier, deadline_usec, frame_payload_size) : NULL;
@@ -421,16 +421,26 @@ UDPARD_PRIVATE int32_t txPushSingleFrame(UdpardTxQueue* const que
421421
// We ignore it because the safe functions are poorly supported; reliance on them may limit the portability.
422422
(void) memcpy(&tqi->payload_buffer[sizeof(UdpardFrameHeader)], payload, payload_size); // NOLINT
423423
}
424-
// Clang-Tidy raises an error recommending the use of memset_s() instead.
425-
// We ignore it because the safe functions are poorly supported; reliance on them may limit the portability.
426-
(void) memset(&tqi->payload_buffer[payload_size], PADDING_BYTE_VALUE, padding_size); // NOLINT
427424
/// Create the FrameHeader
428425
txMakeFrameHeader(&tqi->base.frame.udp_cyphal_header, src_node_id, dst_node_id, port_id, transfer_kind, priority, transfer_id, true, 1);
429426
// Clang-Tidy raises an error recommending the use of memcpy_s() instead.
430427
// We ignore it because the safe functions are poorly supported; reliance on them may limit the portability.
431428
(void) memcpy(&tqi->payload_buffer[0],
432429
&tqi->base.frame.udp_cyphal_header,
433430
sizeof(UdpardFrameHeader)); // NOLINT
431+
432+
// Insert CRC
433+
size_t frame_offset = payload_size + sizeof(UdpardFrameHeader);
434+
TransferCRC crc = crcValue(crcAdd(CRC_INITIAL, payload_size, payload));
435+
uint8_t crc_as_byte[] = {(uint8_t)(crc & BYTE_MAX & CRC_BYTE_MASK),
436+
(uint8_t)(crc >> 8U & CRC_BYTE_MASK),
437+
(uint8_t)(crc >> 16U & CRC_BYTE_MASK),
438+
(uint8_t)(crc >> 24U & CRC_BYTE_MASK)};
439+
for (int i = 0; i < sizeof(crc_as_byte); i++)
440+
{
441+
tqi->payload_buffer[frame_offset++] = crc_as_byte[i];
442+
}
443+
434444
// Insert the newly created TX item into the queue.
435445
const UdpardTreeNode* const res = cavlSearch(&que->root, &tqi->base.base, &txAVLPredicate, &avlTrivialFactory);
436446
(void) res;
@@ -463,7 +473,10 @@ UDPARD_PRIVATE TxChain txGenerateMultiFrameChain(UdpardInstance* const in
463473
{
464474
UDPARD_ASSERT(ins != NULL);
465475
UDPARD_ASSERT(presentation_layer_mtu > sizeof(UdpardFrameHeader));
466-
UDPARD_ASSERT(payload_size > presentation_layer_mtu - sizeof(UdpardFrameHeader)); // Otherwise, a single-frame transfer should be used.
476+
477+
// Otherwise, a single-frame transfer should be used.
478+
// CRC_SIZE_BYTES
479+
UDPARD_ASSERT(payload_size + CRC_SIZE_BYTES > presentation_layer_mtu - sizeof(UdpardFrameHeader));
467480
UDPARD_ASSERT(payload != NULL);
468481

469482
TxChain out = {NULL, NULL, 0};
@@ -472,7 +485,10 @@ UDPARD_PRIVATE TxChain txGenerateMultiFrameChain(UdpardInstance* const in
472485
TransferCRC crc = crcValue(crcAdd(CRC_INITIAL, payload_size, payload));
473486
const uint8_t* payload_ptr = (const uint8_t*) payload;
474487
uint32_t frame_index = 0U;
475-
uint8_t crc_as_byte[] = {(uint8_t)(crc & BYTE_MAX), (uint8_t)(crc >> 8U), (uint8_t)(crc >> 16U), (uint8_t)(crc >> 24U)};
488+
uint8_t crc_as_byte[] = {(uint8_t)(crc & BYTE_MAX & CRC_BYTE_MASK),
489+
(uint8_t)(crc >> 8U & CRC_BYTE_MASK),
490+
(uint8_t)(crc >> 16U & CRC_BYTE_MASK),
491+
(uint8_t)(crc >> 24U & CRC_BYTE_MASK)};
476492
size_t last_crc_index = 0U;
477493
size_t inserted_crc_amount = 0;
478494
while (offset < payload_size_with_crc)
@@ -581,7 +597,7 @@ UDPARD_PRIVATE int32_t txPushMultiFrame(UdpardTxQueue* const que,
581597

582598
UDPARD_ASSERT((ins != NULL) && (que != NULL));
583599
UDPARD_ASSERT(presentation_layer_mtu > sizeof(UdpardFrameHeader));
584-
UDPARD_ASSERT(payload_size > presentation_layer_mtu - sizeof(UdpardFrameHeader));
600+
UDPARD_ASSERT(payload_size + CRC_SIZE_BYTES > presentation_layer_mtu - sizeof(UdpardFrameHeader));
585601
const size_t payload_size_with_crc = payload_size + CRC_SIZE_BYTES;
586602
const size_t num_frames = (payload_size_with_crc
587603
+ presentation_layer_mtu
@@ -882,10 +898,8 @@ UDPARD_PRIVATE int8_t rxSessionAcceptFrame(UdpardInstance* const ins,
882898
}
883899

884900
const bool single_frame = frame->start_of_transfer && frame->end_of_transfer;
885-
if (!single_frame)
886-
{
887-
rxs->calculated_crc = crcAdd(rxs->calculated_crc, frame->payload_size, frame->payload);
888-
}
901+
902+
rxs->calculated_crc = crcAdd(rxs->calculated_crc, frame->payload_size, frame->payload);
889903

890904
int8_t out = rxSessionWritePayload(ins, rxs, extent, frame->payload_size, frame->payload);
891905
if (out < 0)
@@ -896,7 +910,7 @@ UDPARD_PRIVATE int8_t rxSessionAcceptFrame(UdpardInstance* const ins,
896910
else if (frame->end_of_transfer)
897911
{
898912
UDPARD_ASSERT(0 == out);
899-
if (single_frame || (CRC_RESIDUE == rxs->calculated_crc))
913+
if (CRC_RESIDUE == rxs->calculated_crc)
900914
{
901915

902916
out = 1; // One transfer received, notify the application.
@@ -1147,7 +1161,7 @@ int32_t udpardTxPush(UdpardTxQueue* const que,
11471161
const int32_t specifier_result = txMakeSessionSpecifier(metadata, ins->node_id, ins->local_ip_addr, &specifier);
11481162
if (specifier_result >= 0)
11491163
{
1150-
if (payload_size <= pl_mtu)
1164+
if (payload_size + CRC_SIZE_BYTES <= pl_mtu - sizeof(UdpardFrameHeader))
11511165
{
11521166
out = txPushSingleFrame(que,
11531167
ins,

‎tests/test_private_rx.cpp‎

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -439,8 +439,8 @@ TEST_CASE("rxSessionUpdate")
439439
frame.transfer_id = 11;
440440
frame.start_of_transfer = true;
441441
frame.end_of_transfer = true;
442-
frame.payload_size = 3;
443-
frame.payload = reinterpret_cast<const uint8_t*>("\x01\x01\x01");
442+
frame.payload_size = 3 + 4; // 3 payload + 4 CRC
443+
frame.payload = reinterpret_cast<const uint8_t*>("\x01\x01\x01\x70\x2A\xEC\x24"); // \x70\x2A\xEC\x24 is CRC
444444

445445
RxSession rxs;
446446
RxSession rxs_1;
@@ -491,7 +491,7 @@ TEST_CASE("rxSessionUpdate")
491491
REQUIRE(transfer.metadata.port_id == 2'222);
492492
REQUIRE(transfer.metadata.remote_node_id == 55);
493493
REQUIRE(transfer.metadata.transfer_id == 11);
494-
REQUIRE(transfer.payload_size == 3);
494+
REQUIRE(transfer.payload_size == 7);
495495
REQUIRE(0 == std::memcmp(transfer.payload, "\x01\x01\x01", 3));
496496
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
497497
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);
@@ -500,7 +500,7 @@ TEST_CASE("rxSessionUpdate")
500500
// Valid next transfer, wrong transport.
501501
frame.timestamp_usec = 10'000'100;
502502
frame.transfer_id = 12;
503-
frame.payload = reinterpret_cast<const uint8_t*>("\x02\x02\x02");
503+
frame.payload = reinterpret_cast<const uint8_t*>("\x02\x02\x02\x6E\xB1\x75\xE9");
504504
REQUIRE(0 == update(2, 1'000'000, 16));
505505
REQUIRE(rxs.transfer_timestamp_usec == 10'000'000);
506506
REQUIRE(rxs.payload_size == 0); // Handed over to the output transfer.
@@ -511,7 +511,7 @@ TEST_CASE("rxSessionUpdate")
511511

512512
// Correct transport.
513513
frame.timestamp_usec = 10'000'050;
514-
frame.payload = reinterpret_cast<const uint8_t*>("\x03\x03\x03");
514+
frame.payload = reinterpret_cast<const uint8_t*>("\x03\x03\x03\x64\x38\xFD\xAD");
515515
REQUIRE(1 == update(1, 1'000'000, 16));
516516
REQUIRE(rxs.transfer_timestamp_usec == 10'000'050);
517517
REQUIRE(rxs.payload_size == 0);
@@ -525,7 +525,7 @@ TEST_CASE("rxSessionUpdate")
525525
REQUIRE(transfer.metadata.port_id == 2'222);
526526
REQUIRE(transfer.metadata.remote_node_id == 55);
527527
REQUIRE(transfer.metadata.transfer_id == 12);
528-
REQUIRE(transfer.payload_size == 3);
528+
REQUIRE(transfer.payload_size == 7);
529529
REQUIRE(0 == std::memcmp(transfer.payload, "\x03\x03\x03", 3));
530530
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
531531
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);
@@ -534,7 +534,7 @@ TEST_CASE("rxSessionUpdate")
534534
// Same TID.
535535
frame.timestamp_usec = 10'000'200;
536536
frame.transfer_id = 12;
537-
frame.payload = reinterpret_cast<const uint8_t*>("\x04\x04\x04");
537+
frame.payload = reinterpret_cast<const uint8_t*>("\x04\x04\x04\xA3\xF1\xAA\x77");
538538
REQUIRE(0 == update(1, 1'000'200, 16));
539539
REQUIRE(rxs.transfer_timestamp_usec == 10'000'050);
540540
REQUIRE(rxs.payload_size == 0);
@@ -546,7 +546,7 @@ TEST_CASE("rxSessionUpdate")
546546
// Restart due to TID timeout, switch iface.
547547
frame.timestamp_usec = 20'000'000;
548548
frame.transfer_id = 12;
549-
frame.payload = reinterpret_cast<const uint8_t*>("\x05\x05\x05");
549+
frame.payload = reinterpret_cast<const uint8_t*>("\x05\x05\x05\xA9\x78\x22\x33");
550550
REQUIRE(1 == update(0, 1'000'000, 16));
551551
REQUIRE(rxs.transfer_timestamp_usec == 20'000'000);
552552
REQUIRE(rxs.payload_size == 0);
@@ -560,7 +560,7 @@ TEST_CASE("rxSessionUpdate")
560560
REQUIRE(transfer.metadata.port_id == 2'222);
561561
REQUIRE(transfer.metadata.remote_node_id == 55);
562562
REQUIRE(transfer.metadata.transfer_id == 12);
563-
REQUIRE(transfer.payload_size == 3);
563+
REQUIRE(transfer.payload_size == 7);
564564
REQUIRE(0 == std::memcmp(transfer.payload, "\x05\x05\x05", 3));
565565
REQUIRE(ins.getAllocator().getNumAllocatedFragments() == 1);
566566
REQUIRE(ins.getAllocator().getTotalAllocatedAmount() == 16);

0 commit comments

Comments
 (0)