Skip to content

Commit a1bcb1d

Browse files
SchoberMJSchoberMJ
committed
Fix up warnings and clean up the code
Make defines static const, merge in some changes that remove session specifier where not needed, fix up unit tests, fix up warnings, add -werror to cmakelists
1 parent bb86d0a commit a1bcb1d

9 files changed

Lines changed: 94 additions & 110 deletions

File tree

‎libudpard/udpard.c‎

Lines changed: 60 additions & 68 deletions
Large diffs are not rendered by default.

‎libudpard/udpard.h‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,8 @@
1414
/// Author: Pavel Kirienko <pavel@opencyphal.org>
1515
/// Copyright 2022 Amazon.com, Inc. or its affiliates. All Rights Reserved.
1616

17+
/// @todo Replace with subrepo of official libudpard
18+
1719
#ifndef UDPARD_H_INCLUDED
1820
#define UDPARD_H_INCLUDED
1921

@@ -51,10 +53,9 @@ extern "C" {
5153
#define UDPARD_SUCCESS 0
5254

5355
/// MTU values for the supported protocols.
54-
#define UDPARD_MTU_UDP_IPV4 576U
55-
#define UDPARD_MTU_UDP_IPV6 1280U
56-
#define UDPARD_MTU_UDP_IPV4_TEMP 64U
57-
#define UDPARD_MTU_MAX UDPARD_MTU_UDP_IPV6 /// We may want to set this to 1400/1500 for Ethernet MTU
56+
#define UDPARD_MTU_MAX 1400U /// Can update up to 1500.
57+
#define UDPARD_MTU_UDP_IPV4 UDPARD_MTU_MAX // Minimum MTU for IPv4 for the internet is 576
58+
#define UDPARD_MTU_UDP_IPV6 UDPARD_MTU_MAX
5859

5960
/// Parameter ranges are inclusive; the lower bound is zero for all. See Cyphal/UDP Specification for background.
6061
#define UDPARD_SUBJECT_ID_MAX 32767U /// 15 bits subject ID
@@ -541,7 +542,6 @@ int8_t udpardRxAccept(UdpardInstance* const ins,
541542
const UdpardMicrosecond timestamp_usec,
542543
UdpardFrame* const frame,
543544
const uint8_t redundant_transport_index,
544-
UdpardSessionSpecifier* const specifier,
545545
UdpardRxTransfer* const out_transfer,
546546
UdpardRxSubscription** const out_subscription);
547547

‎tests/CMakeLists.txt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ set(CMAKE_CXX_STANDARD 17)
3737
set(CXX_EXTENSIONS OFF)
3838
add_compile_options(
3939
-Wall -Wextra -pedantic -fstrict-aliasing -Wdouble-promotion -Wswitch-enum -Wfloat-equal -Wundef
40-
# -Werror
40+
-Werror
4141
-Wconversion -Wtype-limits -Wsign-conversion -Wcast-align
4242
)
4343
set(CMAKE_CXX_FLAGS "${CMAKE_CXX_FLAGS} -Woverloaded-virtual -Wnon-virtual-dtor -Wsign-promo")

‎tests/exposed.hpp‎

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,6 @@ void txMakeFrameHeader(UdpardFrameHeader* const header,
110110
auto txRoundFramePayloadSizeUp(const std::size_t x) -> std::size_t;
111111

112112
auto rxTryParseFrame(const UdpardMicrosecond timestamp_usec,
113-
const UdpardSessionSpecifier* const specifier,
114113
UdpardFrame* const frame,
115114
RxFrameModel* const out) -> bool;
116115

‎tests/helpers.hpp‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -74,10 +74,10 @@ class TestAllocator
7474
virtual ~TestAllocator()
7575
{
7676
std::unique_lock locker(lock_);
77-
for (auto [ptr, _] : allocated_)
77+
for (const auto& pair : allocated_)
7878
{
7979
// Clang-tidy complains about manual memory management. Suppressed because we need it for testing purposes.
80-
std::free(ptr - canary_.size()); // NOLINT
80+
std::free(pair.first - canary_.size()); // NOLINT
8181
}
8282
}
8383

@@ -139,9 +139,9 @@ class TestAllocator
139139
{
140140
std::unique_lock locker(lock_);
141141
std::size_t out = 0U;
142-
for (auto [_, size] : allocated_)
142+
for (const auto& pair : allocated_)
143143
{
144-
out += size;
144+
out += pair.second;
145145
}
146146
return out;
147147
}
@@ -180,15 +180,14 @@ class Instance
180180
[[nodiscard]] auto rxAccept(const UdpardMicrosecond timestamp_usec,
181181
UdpardFrame& frame,
182182
const uint8_t redundant_transport_index,
183-
UdpardSessionSpecifier& specifier,
183+
UdpardSessionSpecifier& /*specifier*/,
184184
UdpardRxTransfer& out_transfer,
185185
UdpardRxSubscription** const out_subscription)
186186
{
187187
return udpardRxAccept(&udpard_,
188188
timestamp_usec,
189189
&frame,
190190
redundant_transport_index,
191-
&specifier,
192191
&out_transfer,
193192
out_subscription);
194193
}
@@ -277,7 +276,8 @@ class TxQueue
277276
checkInvariants();
278277
const auto size_before = que_.size;
279278
const auto ret = udpardTxPush(&que_, ins, transmission_deadline_usec, &metadata, payload_size, payload);
280-
enforce((ret < 0) || ((size_before + static_cast<std::size_t>(ret)) == que_.size),
279+
const auto num_added = static_cast<std::size_t>(ret);
280+
enforce((ret < 0) || ((size_before + num_added) == que_.size),
281281
"Unexpected size change after push");
282282
checkInvariants();
283283
return ret;

‎tests/test_private_rx.cpp‎

Lines changed: 8 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -17,15 +17,14 @@ TEST_CASE("rxTryParseFrame")
1717
UdpardFrameHeader header{};
1818

1919
const auto parse = [&](const UdpardMicrosecond timestamp_usec,
20-
UdpardSessionSpecifier session_specifier,
2120
const std::vector<std::uint8_t>& payload) {
2221
static std::vector<std::uint8_t> payload_storage;
2322
payload_storage = payload;
2423
UdpardFrame frame{};
2524
frame.payload_size = std::size(payload);
2625
frame.payload = payload_storage.data();
2726
model = RxFrameModel{};
28-
return rxTryParseFrame(timestamp_usec, &session_specifier, &frame, &model);
27+
return rxTryParseFrame(timestamp_usec, &frame, &model);
2928
};
3029

3130
// Some initial header setup and payload test
@@ -55,14 +54,13 @@ TEST_CASE("rxTryParseFrame")
5554
REQUIRE(sizeof(header) == 24U);
5655
REQUIRE(std::size(test_payload) == 24U);
5756

58-
auto test_header_ptr = reinterpret_cast<std::uint8_t*>(&header);
57+
auto *test_header_ptr = reinterpret_cast<std::uint8_t*>(&header);
5958
auto test_payload_storage = std::vector<std::uint8_t>(test_header_ptr, test_header_ptr + sizeof(header));
6059
REQUIRE(test_payload_storage == test_payload);
6160

6261
// MESSAGE
6362
REQUIRE(0 == exposed::txMakeMessageSessionSpecifier(0b0, 0b0, 0xc0a80000, &specifier));
6463
REQUIRE(parse(543210U,
65-
specifier,
6664
{
6765
0x01, // Version
6866
0x00, // Priority
@@ -98,7 +96,6 @@ TEST_CASE("rxTryParseFrame")
9896
// SIMILAR BUT INVALID
9997
REQUIRE(0 == exposed::txMakeMessageSessionSpecifier(0b0, 0b0, 0xc0a80000, &specifier));
10098
REQUIRE(!parse(543210U,
101-
specifier,
10299
{
103100
0x01, // Version
104101
0x00, // Priority
@@ -114,7 +111,6 @@ TEST_CASE("rxTryParseFrame")
114111
// MESSAGE
115112
REQUIRE(0 == exposed::txMakeMessageSessionSpecifier(0b0110011001100, 0b0100111, 0xc0a80000, &specifier));
116113
REQUIRE(parse(123456U,
117-
specifier,
118114
{
119115
0x01, // Version
120116
0x01, // Priority
@@ -147,11 +143,10 @@ TEST_CASE("rxTryParseFrame")
147143
// SIMILAR BUT INVALID
148144
REQUIRE(0 == exposed::txMakeMessageSessionSpecifier(0b0110011001100, 0b0100111, 0xc0a80000, &specifier));
149145
// NO HEADER
150-
REQUIRE(!parse(123456U, specifier, {}));
146+
REQUIRE(!parse(123456U, {}));
151147
// ANON NOT SINGLE FRAME
152148
REQUIRE(0 == exposed::txMakeMessageSessionSpecifier(0b0110011001100, 0b1111111111111111, 0xc0a80000, &specifier));
153149
REQUIRE(!parse(123456U,
154-
specifier,
155150
{
156151
0x01, // Version
157152
0x01, // Priority
@@ -168,7 +163,6 @@ TEST_CASE("rxTryParseFrame")
168163
// ANONYMOUS MESSAGE
169164
REQUIRE(0 == exposed::txMakeMessageSessionSpecifier(0b0110011001101, 0b1111111111111111, 0xc0a80000, &specifier));
170165
REQUIRE(parse(12345U,
171-
specifier,
172166
{
173167
0x01, // Version
174168
0x02, // Priority
@@ -191,13 +185,12 @@ TEST_CASE("rxTryParseFrame")
191185
REQUIRE(model.end_of_transfer);
192186
REQUIRE(model.payload_size == 0);
193187
// SIMILAR BUT INVALID
194-
REQUIRE(!parse(12345U, specifier, {})); // NO HEADER
188+
REQUIRE(!parse(12345U, {})); // NO HEADER
195189

196190
// REQUEST
197191
REQUIRE(0 ==
198192
exposed::txMakeServiceSessionSpecifier(0b0000110011, 0b0100111, 0xc0a80000, &specifier));
199193
REQUIRE(parse(999'999U,
200-
specifier,
201194
{
202195
0x01, // Version
203196
0x03, // Priority
@@ -226,11 +219,10 @@ TEST_CASE("rxTryParseFrame")
226219
REQUIRE(model.payload[2] == 2);
227220
REQUIRE(model.payload[3] == 3);
228221
// SIMILAR BUT INVALID (Source Node ID cant be equal to Destination Node ID)
229-
REQUIRE(!parse(999'999U, specifier, {})); // NO HEADER
222+
REQUIRE(!parse(999'999U, {})); // NO HEADER
230223
REQUIRE(0 ==
231224
exposed::txMakeServiceSessionSpecifier(0b0000110011, 0b0100111, 0xc0a80000, &specifier));
232225
REQUIRE(!parse(999'999U,
233-
specifier,
234226
{
235227
0x01, // Version
236228
0x03, // Priority
@@ -248,7 +240,6 @@ TEST_CASE("rxTryParseFrame")
248240
REQUIRE(0 ==
249241
exposed::txMakeServiceSessionSpecifier(0b0000110011, 0b00011010, 0xc0a80000, &specifier));
250242
REQUIRE(parse(888'888,
251-
specifier,
252243
{
253244
0x01, // Version
254245
0x04, // Priority
@@ -273,12 +264,11 @@ TEST_CASE("rxTryParseFrame")
273264
REQUIRE(model.payload_size == 1);
274265
REQUIRE(model.payload[0] == 255);
275266
// SIMILAR BUT INVALID (Source Node ID cant be equal to Destination Node ID)
276-
REQUIRE(!parse(888'888U, specifier, {})); // NO TAIL BYTE
267+
REQUIRE(!parse(888'888U, {})); // NO TAIL BYTE
277268
REQUIRE(
278269
0 ==
279270
exposed::txMakeServiceSessionSpecifier(0b0000110011, 0b00011010, 0xc0a80000, &specifier));
280271
REQUIRE(!parse(888'888,
281-
specifier,
282272
{
283273
0x01, // Version
284274
0x04, // Priority
@@ -599,7 +589,7 @@ TEST_CASE("rxSessionUpdate")
599589
frame.timestamp_usec = 20'000'200;
600590
frame.start_of_transfer = false;
601591
frame.end_of_transfer = false;
602-
frame.frame_index = 3 + (uint32_t) (1U << (uint32_t) 31U);
592+
frame.frame_index = 3 + static_cast<uint32_t>(1U << static_cast<uint32_t>(31U));
603593
frame.payload_size = 2;
604594
frame.payload = reinterpret_cast<const uint8_t*>("\x09\x09");
605595
REQUIRE(-UDPARD_ERROR_OUT_OF_ORDER == update(0, 1'000'000, 16));
@@ -638,7 +628,7 @@ TEST_CASE("rxSessionUpdate")
638628
frame.timestamp_usec = 20'000'400;
639629
frame.start_of_transfer = false;
640630
frame.end_of_transfer = true;
641-
frame.frame_index = 3 + (uint32_t) (1U << (uint32_t) 31U);
631+
frame.frame_index = 3 + static_cast<uint32_t>(1U << static_cast<uint32_t>(31U));
642632
frame.payload_size = 8; // The payload is IMPLICITLY TRUNCATED, and the CRC IS STILL VALIDATED.
643633
frame.payload = reinterpret_cast<const uint8_t*>("\x09\x09\x09\x09\x32\x98\x04\x7B");
644634
REQUIRE(1 == update(0, 1'000'000, 16));

‎tests/test_public_roundtrip.cpp‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -258,10 +258,14 @@ TEST_CASE("RoundtripSimple")
258258
std::size_t i = 0;
259259
for (const auto& [k, v] : pending_transfers)
260260
{
261-
const auto [ref_meta, ref_payload_size, ref_payload] = v;
261+
262+
UdpardTransferMetadata ref_meta;
263+
uint64_t ref_payload_size {};
264+
std::tie(ref_meta, ref_payload_size, std::ignore) = v;
262265
std::cout << "#" << i++ << "/" << std::size(pending_transfers) << ":" //
263266
<< " ts=" << k //
264267
<< " prio=" << static_cast<std::uint16_t>(ref_meta.priority) //
268+
<< " prio=" << static_cast<std::uint16_t>(ref_meta.priority) //
265269
<< " kind=" << static_cast<std::uint16_t>(ref_meta.transfer_kind) //
266270
<< " port=" << ref_meta.port_id //
267271
<< " nid=" << static_cast<std::uint16_t>(ref_meta.remote_node_id) //

‎tests/test_public_rx.cpp‎

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ TEST_CASE("RxBasic0")
3434
UdpardFrameHeader frame_header,
3535
UdpardSessionSpecifier session_specifier,
3636
const std::vector<std::uint8_t>& payload) {
37-
auto header_ptr = reinterpret_cast<std::uint8_t*>(&frame_header);
37+
auto *header_ptr = reinterpret_cast<std::uint8_t*>(&frame_header);
3838
auto payload_storage = std::vector<std::uint8_t>(header_ptr, header_ptr + sizeof(frame_header));
3939
static std::vector<std::uint8_t> payload_buffer;
4040
payload_buffer = payload;
@@ -318,7 +318,7 @@ TEST_CASE("RxAnonymous")
318318
UdpardFrameHeader frame_header,
319319
UdpardSessionSpecifier session_specifier,
320320
const std::vector<std::uint8_t>& payload) {
321-
auto header_ptr = reinterpret_cast<std::uint8_t*>(&frame_header);
321+
auto *header_ptr = reinterpret_cast<std::uint8_t*>(&frame_header);
322322
auto payload_storage = std::vector<std::uint8_t>(header_ptr, header_ptr + sizeof(frame_header));
323323
static std::vector<std::uint8_t> payload_buffer;
324324
payload_buffer = payload;
@@ -466,15 +466,14 @@ TEST_CASE("RxSubscriptionErrors")
466466
REQUIRE(-UDPARD_ERROR_INVALID_ARGUMENT == udpardRxUnsubscribe(&ins.getInstance(), kind.value, 0));
467467

468468
UdpardFrame frame{};
469-
UdpardSessionSpecifier specifier{};
470469
frame.payload_size = 1U;
471470
UdpardRxTransfer transfer{};
472471
REQUIRE(-UDPARD_ERROR_INVALID_ARGUMENT ==
473-
udpardRxAccept(&ins.getInstance(), 0, &frame, 0, &specifier, &transfer, nullptr));
474-
REQUIRE(-UDPARD_ERROR_INVALID_ARGUMENT == udpardRxAccept(nullptr, 0, &frame, 0, &specifier, &transfer, nullptr));
472+
udpardRxAccept(&ins.getInstance(), 0, &frame, 0, &transfer, nullptr));
473+
REQUIRE(-UDPARD_ERROR_INVALID_ARGUMENT == udpardRxAccept(nullptr, 0, &frame, 0, &transfer, nullptr));
475474
REQUIRE(-UDPARD_ERROR_INVALID_ARGUMENT ==
476-
udpardRxAccept(&ins.getInstance(), 0, nullptr, 0, &specifier, &transfer, nullptr));
475+
udpardRxAccept(&ins.getInstance(), 0, nullptr, 0, &transfer, nullptr));
477476
REQUIRE(-UDPARD_ERROR_INVALID_ARGUMENT ==
478-
udpardRxAccept(&ins.getInstance(), 0, &frame, 0, &specifier, nullptr, nullptr));
479-
REQUIRE(-UDPARD_ERROR_INVALID_ARGUMENT == udpardRxAccept(nullptr, 0, nullptr, 0, nullptr, nullptr, nullptr));
477+
udpardRxAccept(&ins.getInstance(), 0, &frame, 0, nullptr, nullptr));
478+
REQUIRE(-UDPARD_ERROR_INVALID_ARGUMENT == udpardRxAccept(nullptr, 0, nullptr, 0, nullptr, nullptr));
480479
}

‎tests/test_public_tx.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -226,7 +226,7 @@ REQUIRE(que.peek()->getPayloadByte(3) == 3);
226226
// Multi-frame transfer
227227
meta.priority = UdpardPriorityLow;
228228
meta.transfer_id = 22;
229-
que.setMTU(UDPARD_MTU_UDP_IPV4_TEMP);
229+
que.setMTU(64U);
230230
ins.setNodeID(42);
231231
REQUIRE(64U == que.getMTU());
232232
REQUIRE(2 == que.push(&ins.getInstance(), 1'000'000'000'100ULL, meta, 68, payload.data())); // 68 bytes --> 2 frames

0 commit comments

Comments
 (0)