Skip to content

Commit 32ff7e9

Browse files
lydia-at-amazonlydia-at-amazon
authored
Update frame index to start at 0 instead of 1 (#27)
1 parent 8ba629b commit 32ff7e9

File tree

5 files changed

+60
-32
lines changed

5 files changed

+60
-32
lines changed

‎libudpard/udpard.c‎

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -429,7 +429,11 @@ UDPARD_PRIVATE int32_t txPushSingleFrame(UdpardTxQueue* const que
429429
UDPARD_ASSERT(sizeof(UdpardFrameHeader) > 0); // NOLINT
430430
const size_t frame_payload_size = payload_size + sizeof(UdpardFrameHeader) + CRC_SIZE_BYTES;
431431
UDPARD_ASSERT(frame_payload_size > payload_size);
432-
int32_t out = 0;
432+
433+
int32_t out = 0;
434+
uint32_t frame_index = 0U;
435+
bool end_of_transfer = true;
436+
433437
TxItem* const tqi =
434438
(que->size < que->capacity) ? txAllocateQueueItem(ins, specifier, deadline_usec, frame_payload_size) : NULL;
435439
if (tqi != NULL)
@@ -442,7 +446,17 @@ UDPARD_PRIVATE int32_t txPushSingleFrame(UdpardTxQueue* const que
442446
(void) memcpy(&tqi->payload_buffer[sizeof(UdpardFrameHeader)], payload, payload_size); // NOLINT
443447
}
444448
/// Create the FrameHeader
445-
txMakeFrameHeader(&tqi->base.frame.udp_cyphal_header, src_node_id, dst_node_id, port_id, transfer_kind, priority, transfer_id, true, 1);
449+
txMakeFrameHeader(
450+
&tqi->base.frame.udp_cyphal_header,
451+
src_node_id,
452+
dst_node_id,
453+
port_id,
454+
transfer_kind,
455+
priority,
456+
transfer_id,
457+
end_of_transfer,
458+
frame_index);
459+
446460
// Clang-Tidy raises an error recommending the use of memcpy_s() instead.
447461
// We ignore it because the safe functions are poorly supported; reliance on them may limit the portability.
448462
(void) memcpy(&tqi->payload_buffer[0], &tqi->base.frame.udp_cyphal_header, sizeof(UdpardFrameHeader)); // NOLINT
@@ -557,7 +571,17 @@ UDPARD_PRIVATE TxChain txGenerateMultiFrameChain(UdpardInstance* const in
557571
// Size of remaining payload without CRC
558572
size_t payload_move_size = move_size_with_crc - overrun_amount;
559573

560-
txMakeFrameHeader(&out.tail->base.frame.udp_cyphal_header, src_node_id, dst_node_id, port_id, transfer_kind, priority, transfer_id, false, ++frame_index);
574+
txMakeFrameHeader(
575+
&out.tail->base.frame.udp_cyphal_header,
576+
src_node_id,
577+
dst_node_id,
578+
port_id,
579+
transfer_kind,
580+
priority,
581+
transfer_id,
582+
false,
583+
frame_index);
584+
561585
if(initial_payload_overrun)
562586
{
563587
// Insert payload and header
@@ -593,6 +617,7 @@ UDPARD_PRIVATE TxChain txGenerateMultiFrameChain(UdpardInstance* const in
593617
(void) memcpy(&out.tail->payload_buffer[0], &out.tail->base.frame.udp_cyphal_header, sizeof(UdpardFrameHeader)); // NOLINT
594618
UDPARD_ASSERT((frame_offset) == out.tail->base.frame.payload_size);
595619
}
620+
++frame_index;
596621
}
597622
return out;
598623
}
@@ -768,7 +793,7 @@ UDPARD_PRIVATE bool rxTryParseFrame(const UdpardMicrosecond timestam
768793
out->payload = (void*) ((uint8_t*) frame->payload + sizeof(frame->udp_cyphal_header));
769794

770795
out->transfer_id = frame->udp_cyphal_header.transfer_id;
771-
out->start_of_transfer = (((frame->udp_cyphal_header.frame_index_eot) & (UDPARD_MAX_FRAME_INDEX)) == 1);
796+
out->start_of_transfer = (((frame->udp_cyphal_header.frame_index_eot) & (UDPARD_MAX_FRAME_INDEX)) == 0);
772797
out->end_of_transfer = ((frame->udp_cyphal_header.frame_index_eot >> UDPARD_END_OF_TRANSFER_OFFSET) == 1);
773798
out->frame_index = frame->udp_cyphal_header.frame_index_eot;
774799
// Make sure header version is supported
@@ -993,7 +1018,7 @@ UDPARD_PRIVATE int8_t rxSessionUpdate(UdpardInstance* const ins,
9931018
else
9941019
{
9951020
if ((!frame->start_of_transfer && frame->frame_index != rxs->last_udp_header_index + 1)
996-
|| (frame->start_of_transfer && frame->frame_index != 1U))
1021+
|| (frame->start_of_transfer && frame->frame_index != 0U))
9971022
{
9981023
// Out of order multiframe packet received
9991024
out = -UDPARD_ERROR_OUT_OF_ORDER;

‎tests/exposed.hpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,7 @@ struct TxItem final : UdpardTxQueueItem
3636

3737
[[nodiscard]] auto isStartOfTransfer() const
3838
{
39-
return (getFrameHeader()->frame_index_eot & ((1U << 31U) - 1U)) == 1;
39+
return (getFrameHeader()->frame_index_eot & ((1U << 31U) - 1U)) == 0;
4040
}
4141
[[nodiscard]] auto isEndOfTransfer() const { return (getFrameHeader()->frame_index_eot >> 31U) == 1; }
4242

‎tests/test_private_rx.cpp‎

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
#include <cstring>
99

1010
static const uint32_t CRC_INITIAL = 0xFFFFFFFFU;
11+
static const uint32_t UDPARD_MAX_FRAME_INDEX = 0x7FFFFFFFU;
1112

1213
TEST_CASE("rxTryParseFrame")
1314
{
@@ -36,7 +37,7 @@ TEST_CASE("rxTryParseFrame")
3637
header.destination_node_id = 0xFFFF;
3738
header.data_specifier = 0x0000;
3839
header.transfer_id = 0x0000000000000001;
39-
header.frame_index_eot = (1U << 31U) + 1U;
40+
header.frame_index_eot = 1U << 31U;
4041
header._opaque = 0x0000;
4142
header.cyphal_header_checksum = 0x0000;
4243

@@ -48,7 +49,7 @@ TEST_CASE("rxTryParseFrame")
4849
0xFF, 0xFF, // Destination Node ID
4950
0x00, 0x00, // Data Specifier
5051
0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // Transfer ID
51-
0x01, 0x00, 0x00, 0x80, // Frame EOT
52+
0x00, 0x00, 0x00, 0x80, // Frame EOT
5253
0x00, 0x00, // Opaque Data
5354
0x00, 0x00, // Transfer CRC
5455
};
@@ -70,7 +71,7 @@ TEST_CASE("rxTryParseFrame")
7071
0xFF, 0xFF, // Destination Node ID
7172
0x00, 0x00, // Data Specifier
7273
0x01, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // Transfer ID
73-
0x01, 0x00, 0x00, 0x80, // Frame EOT
74+
0x00, 0x00, 0x00, 0x80, // Frame EOT
7475
0x00, 0x00, // Opaque Data
7576
0x00, 0x00, // Transfer CRC
7677
0, 1, 2, 3, 4, 5, 6, 7 // Payload
@@ -82,7 +83,7 @@ TEST_CASE("rxTryParseFrame")
8283
REQUIRE(model.source_node_id == 0U);
8384
REQUIRE(model.destination_node_id == UDPARD_NODE_ID_UNSET);
8485
REQUIRE(model.transfer_id == 1U);
85-
// REQUIRE(model.frame_index == 1U);
86+
REQUIRE((model.frame_index & UDPARD_MAX_FRAME_INDEX) == 0U);
8687
REQUIRE(model.start_of_transfer);
8788
REQUIRE(model.end_of_transfer);
8889
REQUIRE(model.payload_size == 8);
@@ -105,7 +106,7 @@ TEST_CASE("rxTryParseFrame")
105106
0xFF, 0xFF, // Destination Node ID
106107
0x00, 0x00, // Data Specifier
107108
0x07, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // Transfer ID
108-
0x00, 0x00, 0x00, 0x80, // Frame EOT
109+
0x02, 0x00, 0x00, 0x80, // Frame EOT
109110
0x00, 0x00, // Opaque Data
110111
0x00, 0x00, // Transfer CRC
111112
})); // MFT FRAMES REQUIRE PAYLOAD
@@ -120,7 +121,7 @@ TEST_CASE("rxTryParseFrame")
120121
0xFF, 0xFF, // Destination Node ID
121122
0xCC, 0x0C, // Data Specifier
122123
0x17, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // Transfer ID
123-
0x01, 0x00, 0x00, 0x00, // Frame EOT
124+
0x00, 0x00, 0x00, 0x00, // Frame EOT
124125
0x00, 0x00, // Opaque Data
125126
0x00, 0x00, // Transfer CRC
126127
0, 1, 2, 3, 4, 5, 6 // Payload
@@ -132,6 +133,7 @@ TEST_CASE("rxTryParseFrame")
132133
REQUIRE(model.source_node_id == 0b0100111U);
133134
REQUIRE(model.destination_node_id == UDPARD_NODE_ID_UNSET);
134135
REQUIRE(model.transfer_id == 23U);
136+
REQUIRE((model.frame_index & UDPARD_MAX_FRAME_INDEX) == 0U);
135137
REQUIRE(model.start_of_transfer);
136138
REQUIRE(!model.end_of_transfer);
137139
REQUIRE(model.payload_size == 7);
@@ -172,7 +174,7 @@ TEST_CASE("rxTryParseFrame")
172174
0xFF, 0xFF, // Destination Node ID
173175
0xCD, 0x0C, // Data Specifier
174176
0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, // Transfer ID
175-
0x01, 0x00, 0x00, 0x80, // Frame EOT
177+
0x00, 0x00, 0x00, 0x80, // Frame EOT
176178
0x00, 0x00, // Opaque Data
177179
0x00, 0x00, // Transfer CRC
178180
}));
@@ -183,6 +185,7 @@ TEST_CASE("rxTryParseFrame")
183185
REQUIRE(model.source_node_id == UDPARD_NODE_ID_UNSET);
184186
REQUIRE(model.destination_node_id == UDPARD_NODE_ID_UNSET);
185187
REQUIRE(model.transfer_id == 0U);
188+
REQUIRE((model.frame_index & UDPARD_MAX_FRAME_INDEX) == 0U);
186189
REQUIRE(model.start_of_transfer);
187190
REQUIRE(model.end_of_transfer);
188191
REQUIRE(model.payload_size == 0);
@@ -563,7 +566,7 @@ TEST_CASE("rxSessionUpdate")
563566
frame.transfer_id = 13;
564567
frame.end_of_transfer = false;
565568
frame.payload_size = 7;
566-
frame.frame_index = 1;
569+
frame.frame_index = 0;
567570
frame.payload = reinterpret_cast<const uint8_t*>("\x06\x06\x06\x06\x06\x06\x06");
568571
REQUIRE(0 == update(0, 1'000'000, 16));
569572

@@ -591,7 +594,7 @@ TEST_CASE("rxSessionUpdate")
591594
frame.timestamp_usec = 20'000'200;
592595
frame.start_of_transfer = false;
593596
frame.end_of_transfer = false;
594-
frame.frame_index = 3 + static_cast<uint32_t>(1U << static_cast<uint32_t>(31U));
597+
frame.frame_index = 2 + static_cast<uint32_t>(1U << static_cast<uint32_t>(31U));
595598
frame.payload_size = 2;
596599
frame.payload = reinterpret_cast<const uint8_t*>("\x09\x09");
597600
REQUIRE(-UDPARD_ERROR_OUT_OF_ORDER == update(0, 1'000'000, 16));
@@ -618,7 +621,7 @@ TEST_CASE("rxSessionUpdate")
618621
frame.start_of_transfer = true;
619622
frame.end_of_transfer = false;
620623
frame.payload_size = 7;
621-
frame.frame_index = 1;
624+
frame.frame_index = 0;
622625
frame.payload = reinterpret_cast<const uint8_t*>("\x06\x06\x06\x06\x06\x06\x06");
623626
REQUIRE(0 == update(0, 1'000'000, 16));
624627

@@ -645,7 +648,7 @@ TEST_CASE("rxSessionUpdate")
645648
// Multi-frame, middle.
646649
frame.start_of_transfer = false;
647650
frame.end_of_transfer = false;
648-
frame.frame_index = 2;
651+
frame.frame_index = 1;
649652
frame.payload_size = 7;
650653
frame.payload = reinterpret_cast<const uint8_t*>("\x07\x07\x07\x07\x07\x07\x07");
651654
REQUIRE(0 == update(0, 1'000'000, 16));
@@ -672,7 +675,7 @@ TEST_CASE("rxSessionUpdate")
672675
// Multi-frame, last.
673676
frame.start_of_transfer = false;
674677
frame.end_of_transfer = true;
675-
frame.frame_index = 3 + static_cast<uint32_t>(1U << static_cast<uint32_t>(31U));
678+
frame.frame_index = 2 + static_cast<uint32_t>(1U << static_cast<uint32_t>(31U));
676679
frame.payload_size = 8; // The payload is IMPLICITLY TRUNCATED, and the CRC IS STILL VALIDATED.
677680
frame.payload = reinterpret_cast<const uint8_t*>("\x09\x09\x09\x09\x32\x98\x04\x7B");
678681
REQUIRE(1 == update(0, 1'000'000, 16));

‎tests/test_public_rx.cpp‎

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -64,7 +64,7 @@ TEST_CASE("RxBasic0")
6464
header.destination_node_id = 0b1111111111111111;
6565
header.data_specifier = 0b0000110011001100;
6666
header.transfer_id = 1;
67-
header.frame_index_eot = (1U << 31U) + 1U;
67+
header.frame_index_eot = 1U << 31U;
6868
specifier.data_specifier = UDPARD_UDP_PORT;
6969
specifier.destination_route_specifier = 0b11101111'00'01000'0'0'000110011001100;
7070
specifier.source_route_specifier = 0b11000000'10101000'00000000'00100111;
@@ -119,7 +119,7 @@ TEST_CASE("RxBasic0")
119119

120120
// Accepted message.
121121
subscription = nullptr;
122-
header.frame_index_eot = (1U << 31U) + 1U;
122+
header.frame_index_eot = 1U << 31U;
123123
header.priority = 0b001;
124124
header.transfer_id = 0;
125125
header.data_specifier = 0b0000110011001100; // Subject ID = 3276
@@ -148,7 +148,7 @@ TEST_CASE("RxBasic0")
148148

149149
// Dropped request because the local node does not have a node-ID.
150150
subscription = nullptr;
151-
header.frame_index_eot = (1U << 31U) + 1U;
151+
header.frame_index_eot = 1U << 31U;
152152
header.priority = 0b011;
153153
header.transfer_id = 1;
154154
header.source_node_id = 0b00000000'00100111;
@@ -163,7 +163,7 @@ TEST_CASE("RxBasic0")
163163
// Dropped request because the local node has a different node-ID.
164164
ins.setNodeID(0b0011010);
165165
subscription = nullptr;
166-
header.frame_index_eot = (1U << 31U) + 1U;
166+
header.frame_index_eot = 1U << 31U;
167167
header.priority = 0b011;
168168
header.transfer_id = 1;
169169
header.source_node_id = 0b00000000'00100111;
@@ -176,7 +176,7 @@ TEST_CASE("RxBasic0")
176176

177177
// Same request accepted now.
178178
subscription = nullptr;
179-
header.frame_index_eot = (1U << 31U) + 1U;
179+
header.frame_index_eot = 1U << 31U;
180180
header.priority = 0b011;
181181
header.transfer_id = 4;
182182
header.destination_node_id = 0b00000000'00011010;
@@ -202,7 +202,7 @@ TEST_CASE("RxBasic0")
202202
// Response transfer not accepted because the local node has a different node-ID.
203203
// There is no dynamic memory available, but it doesn't matter because a rejection does not require allocation.
204204
subscription = nullptr;
205-
header.frame_index_eot = (1U << 31U) + 1U;
205+
header.frame_index_eot = 1U << 31U;
206206
header.priority = 0b100;
207207
header.transfer_id = 1;
208208
header.source_node_id = 0b00000000'00011011;
@@ -216,7 +216,7 @@ TEST_CASE("RxBasic0")
216216

217217
// Response transfer not accepted due to OOM -- can't allocate RX session.
218218
subscription = nullptr;
219-
header.frame_index_eot = (1U << 31U) + 1U;
219+
header.frame_index_eot = 1U << 31U;
220220
header.priority = 0b100;
221221
header.transfer_id = 1;
222222
header.source_node_id = 0b00000000'00011011;
@@ -233,7 +233,7 @@ TEST_CASE("RxBasic0")
233233
// Response transfer not accepted due to OOM -- can't allocate the buffer (RX session is allocated OK).
234234
ins.getAllocator().setAllocationCeiling(3 * sizeof(RxSession) + 16 + 20);
235235
subscription = nullptr;
236-
header.frame_index_eot = (1U << 31U) + 1U;
236+
header.frame_index_eot = 1U << 31U;
237237
header.priority = 0b100;
238238
header.transfer_id = 1;
239239
header.source_node_id = 0b00000000'00011011;
@@ -258,7 +258,7 @@ TEST_CASE("RxBasic0")
258258

259259
// Same response accepted now. We have to keep incrementing the transfer-ID though because it's tracked.
260260
subscription = nullptr;
261-
header.frame_index_eot = (1U << 31U) + 1U;
261+
header.frame_index_eot = 1U << 31U;
262262
header.priority = 0b100;
263263
header.transfer_id = 5;
264264
header.source_node_id = 0b00000000'00011011;
@@ -342,7 +342,7 @@ TEST_CASE("RxAnonymous")
342342
header.destination_node_id = 0b1111111111111111;
343343
header.data_specifier = 0b0000110011001100;
344344
header.transfer_id = 1;
345-
header.frame_index_eot = (1U << 31U) + 1U;
345+
header.frame_index_eot = 1U << 31U;
346346
specifier.data_specifier = UDPARD_UDP_PORT;
347347
specifier.destination_route_specifier = 0b11101111'00'01000'0'0'000110011001100;
348348
specifier.source_route_specifier = 0b11000000'10101000'00000000'00000000;
@@ -358,7 +358,7 @@ TEST_CASE("RxAnonymous")
358358

359359
// Accepted anonymous message.
360360
subscription = nullptr;
361-
header.frame_index_eot = (1U << 31U) + 1U;
361+
header.frame_index_eot = 1U << 31U;
362362
header.priority = 0b001;
363363
header.source_node_id = 0b1111111111111111;
364364
header.destination_node_id = 0b1111111111111111;
@@ -388,7 +388,7 @@ TEST_CASE("RxAnonymous")
388388
REQUIRE(ensureAllNullptr(ins.getMessageSubs().at(0)->sessions)); // No RX states!
389389

390390
// Anonymous message not accepted because OOM. The transfer shall remain unmodified by the call, so we re-check it.
391-
header.frame_index_eot = (1U << 31U) + 1U;
391+
header.frame_index_eot = 1U << 31U;
392392
header.priority = 0b001;
393393
header.transfer_id = 1;
394394
specifier.data_specifier = UDPARD_UDP_PORT;
@@ -417,7 +417,7 @@ TEST_CASE("RxAnonymous")
417417

418418
// Accepted anonymous message with small payload.
419419
subscription = nullptr;
420-
header.frame_index_eot = (1U << 31U) + 1U;
420+
header.frame_index_eot = 1U << 31U;
421421
header.priority = 0b001;
422422
header.transfer_id = 0;
423423
specifier.data_specifier = UDPARD_SUBJECT_ID_PORT;

‎tests/test_public_tx.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -215,7 +215,7 @@ TEST_CASE("TxBasic1")
215215
REQUIRE(que.peek()->getPayloadByte(0) == 0); // Payload start. (starts after header)
216216
REQUIRE(que.peek()->getPayloadByte(1) == 1);
217217
REQUIRE(que.peek()->getPayloadByte(2) == 2);
218-
REQUIRE(que.peek()->getPayloadByte(3) == 3);
218+
REQUIRE(que.peek()->getPayloadByte(3) == 3);
219219
REQUIRE(que.peek()->getPayloadByte(4) == 4);
220220
REQUIRE(que.peek()->getPayloadByte(5) == 5);
221221
REQUIRE(que.peek()->getPayloadByte(6) == 6);

0 commit comments

Comments
 (0)