Transmission pipeline and cosmetic adjustments to the API #34

pavel-kirienko · 2023-07-01T22:35:42Z

Please focus on udpard.c and udpard.h. Upon a closer inspection it appeared to be easier to produce a completely new implementation that exploits the design features of Cyphal/UDP instead of adapting the original CAN implementation.

The code is fully covered by tests; Sonar reports a slightly lower value because of imperfections of llvm-cov.

Golden tests were produced with the help of PyCyphal; relevant Python snippets are provided in the comments.

The commit history is uninformative and should not be looked at.

…k to 10k to speed it up

tests/CMakeLists.txt

libudpard/udpard.h

tests/helpers.hpp

libudpard/udpard.c

thirtytwobits

I'm assuming we don't have the work in here to handle a low-priority, long-timeout item in a tx queue?

thirtytwobits · 2023-07-06T05:15:17Z

libudpard/udpard.c

+#define SUBJECT_MULTICAST_GROUP_ADDRESS_MASK 0xEF000000UL
+#define SERVICE_MULTICAST_GROUP_ADDRESS_MASK 0xEF010000UL
+
+UDPARD_PRIVATE uint32_t makeSubjectIPGroupAddress(const UdpardPortID subject_id)


I do think we should switch. I don't mind having a bunch of individual test executables. It's how I structured CETL even without the need to test cpp internals.

libudpard/udpard.c

…ot buildable

…rd.c>; not buildable" This reverts commit a35339c.

pavel-kirienko · 2023-07-06T19:06:34Z

I'm assuming we don't have the work in here to handle a low-priority, long-timeout item in a tx queue?

No, I considered this to not be part of what we defined as "significant completion". This will be taken care of after the core functionality is in place: #33

I do think we should switch. I don't mind having a bunch of individual test executables. It's how I structured CETL even without the need to test cpp internals.

Okay, I just did that and discovered (too late) that it won't work. The problem is that it is difficult to compile a C file using a C++ compiler without suppressing a very large set of sensible compiler warnings. Clang-Tidy and Sonar can also be made to not emit diagnostics for the included C file but it it slightly less trivial than I would like it to be. As C and C++ are, after all, very different languages, I'm worried that by compiling C with a C++ compiler we may create maintenance issues for us in the future, particularly if we decide to switch to a newer version of either standard at one point. You can see what it looks like in the second-to-last commit (the next one reverts it).

I suggest keeping the existing design as-is. The presence of exposed.hpp pains me but it is less undesirable than the alternative.

thirtytwobits · 2023-07-06T20:06:55Z

Okay, I just did that and discovered (too late) that it won't work. The problem is that it is difficult to compile a C file using a C++ compiler without suppressing a very large set of sensible compiler warnings...

import the .c into an exposed.c that defines test_hooks. for example:

// realcode.c
static int foo(const char* p)
{
 ...
}

// exposed.c
#include "realcode.c"

EXPORT int exposed_foo(const char* p)
{
    return foo(p);
}

pavel-kirienko · 2023-07-06T20:23:50Z

Could you please elaborate on the value of this approach compared to exposed.hpp? I see one in that there will be no need to customize UDPARD_INTERNAL but it doesn't seem significant; plus it doesn't obviate the need for exposed.hpp.

thirtytwobits · 2023-07-06T20:29:22Z

Could you please elaborate on the value of this approach compared to exposed.hpp? I see one in that there will be no need to customize UDPARD_INTERNAL but it doesn't seem significant; plus it doesn't obviate the need for exposed.hpp.

It avoids polluting the production code with a test macro or any knowledge of the tests. I can accept any crazy plan needed in unit tests but not in the product.

…ard.c directly (#37)

lydia-at-amazon

Looks good, all my comments are just nits.

lydia-at-amazon · 2023-07-07T22:23:46Z

libudpard/udpard.h

-
-/// The port number is defined in the Cyphal/UDP Specification. The same port number is used for all transfer kinds.
-#define UDPARD_UDP_PORT 9382U
+#define UDPARD_MTU_DEFAULT_MAX_SINGLE_FRAME (UDPARD_MTU_MAX - 4U)


I'm probably just blind, but where is UDPARD_MTU_MAX defined? Should this be UDPARD_MTU_DEFAULT - 4U?

Nice catch, this went unnoticed because there was no test for this. There is now, it's called testMakeChainSingleFrameDefaultMTU and it ensures that UDPARD_MTU_DEFAULT_MAX_SINGLE_FRAME-sized payload fits into a UDPARD_MTU_DEFAULT-sized frame, and a spillover occurs when the payload is enlarged by 1 byte.

lydia-at-amazon · 2023-07-07T22:33:16Z

libudpard/udpard.h

 /// See UDPARD_MTU_*.
 /// The value can be changed arbitrarily at any time between enqueue operations.
- size_t mtu_bytes;
+ /// The value is constrained by the library to be positive.
+ size_t mtu;


Why not mtu_bytes? Would prefer to keep the units

I have no strong preference either way but the constants do not mention bytes and MTU is in bytes (octets) by definition. We can change it back if needed.

lydia-at-amazon · 2023-07-08T00:03:01Z

libudpard/udpard.c

+ break;
+ }
+ const bool last = (payload_size_with_crc - offset) <= mtu;
+ byte_t* write_ptr = txSerializeHeader(&tqi->payload_buffer[0], meta, (uint32_t) out.count, last);


Since you're using out.count as the frame index, could you make the count member of TxChain a uint32_t so you don't have to cast here?

count is used to represent the number of items allocated in memory without relation to the design of the protocol header (that part belongs to a different scope), so I assumed it is reasonable to use size_t here to avoid scope leakage.

Usage of uint32_t requires a cast at tx->queue_size += chain.count in narrow-word platforms like AVR (caught by the CI).

lydia-at-amazon · 2023-07-08T01:20:59Z

libudpard/udpard.c

+ UdpardPriority priority; ///< Do we need this exposed in the public structure? We already have DSCP there.
+ // The MISRA violation here is hard to get rid of without having to allocate a separate memory block for the
+ // payload, which is much more costly risk-wise.
+ byte_t payload_buffer[]; // NOSONAR MISRA C 18.7 Flexible array member.


Are you choosing not to make the payload buffer the size of UDPARD_MTU_DEFAULT to save on memory?

That is not the main reason. The main reason is to support jumbo frames with MTU >> 1500 bytes.

lydia-at-amazon · 2023-07-08T01:30:55Z

libudpard/udpard.c

+ const uint32_t frame_index,
+ const bool end_of_transfer)
+{
+ byte_t* p = destination_buffer;


Is there a more descriptive variable name we could use instead of p or this common practice to refer to a generic pointer?

I think p is common, but on the other hand, some style guides prohibit single-word names except i,j,k, reserved for loop counters, so I changed it to ptr.

lydia-at-amazon · 2023-07-08T01:33:36Z

libudpard/udpard.c

+ user_transfer_reference);
+ if (NULL == out.head)
+ {
+ out.head = tqi;


What does tqi stand for? tx_queue_item? We have a style guideline that I like about not abbreviating identifiers by removing letters since it helps with readability.

Renamed as item.

lydia-at-amazon · 2023-07-08T01:39:36Z

libudpard/udpard.c

+// =====================================================================================================================
+
+/// This is a subclass of UdpardTxItem. A pointer to this type can be cast to UdpardTxItem safely.
+/// This is standard-compliant. The paragraph 6.7.2.1.15 says:


Which standard? The C standard?

tests/test_private_tx.cpp

lydia-at-amazon · 2023-07-08T02:02:49Z

libudpard/udpard.c

+ return p;
+}
+
+static inline byte_t* txSerializeHeader(byte_t* const destination_buffer,


I think this function would be more readable if we had a struct for the frame header like we did before:
https://github.com/OpenCyphal-Garage/libudpard/blob/main/libudpard/udpard.h#L133
https://github.com/OpenCyphal-Garage/libudpard/blob/main/libudpard/udpard.c#L325

And then populated the fields of the struct rather than using pointer arithmetic.

Is there a reason this new implementation moved away from that?

The old approach is endian-sensitive.

I think that TransferMetadata offers a stronger and more useful abstraction because it encapsulates all transfer-specific metadata in a single entity that is independent of values that vary within the transfer (the frame index and the EOR flag), which allows passing it unmodified from the top-level function to the header serializer.

…w analyzer

sonarcloud · 2023-07-08T12:02:08Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

96.3% Coverage
0.0% Duplication

aentinger · 2023-07-10T13:01:29Z

Nudging @thirtytwobits to proceed with the review because I'm looking into integrating libudpard - once ready - into 107-Arduino-Cyphal 😁

thirtytwobits · 2023-07-10T19:03:30Z

👀

pavel-kirienko added 17 commits June 30, 2023 14:54

Add CRC implementations #sonar

9be32a8

Cavl test: reduce the iteration count in the randomized test from 100…

7ff170a

…k to 10k to speed it up

udpardTxInit

1988da6

Add txPushSingleFrame() #sonar

f5e9b13

Test header serialization

4ebcffd

nits #sonar

140129c

Add txGenerateMultiFrameChain

99e4892

AVR-specific conversion fixes

66ccad1

Check deallocation size; initialize tx precedence based on priority

65a523b

txMakeChain is finished but not all cases are covered yet #sonar

4a8eb09

make chain tests done #sonar

b9c252b

txPush

5b834b7

Finish the TX pipeline, two tests missing #sonar

3cff534

Extend error handling

f1a0936

Fix nits #sonar

6bfea6d

Add missing tests #sonar

5de3f60

Minor adjustmet before sending the PR

5e27852

pavel-kirienko self-assigned this Jul 1, 2023

pavel-kirienko added 3 commits July 2, 2023 15:14

Use plain priority instead of precedence for simplification

e9929e6

Use fewer test data strings

1564317

Add prioritization test across multiple transfers

53e9f87

pavel-kirienko requested review from thirtytwobits and lydia-at-amazon July 2, 2023 12:59

pavel-kirienko marked this pull request as ready for review July 2, 2023 12:59

thirtytwobits reviewed Jul 4, 2023

View reviewed changes

pavel-kirienko mentioned this pull request Jul 4, 2023

The test suite should include a demo application written in C11 #35

Closed

pavel-kirienko added 3 commits July 4, 2023 12:40

Address simple review comments; mostly renamings and comments

8974689

Define HEADER_FRAME_INDEX_MAX for clarity

0c6e7f8

UDPARD_UDP_PORT does not need to be public

d4875cd

thirtytwobits reviewed Jul 6, 2023

View reviewed changes

pavel-kirienko added 3 commits July 6, 2023 21:05

Nits

b583110

BROKEN: switch to one-test-per-executable with #include <udpard.c>; n…

a35339c

…ot buildable

Revert "BROKEN: switch to one-test-per-executable with #include <udpa…

1f711fa

…rd.c>; not buildable" This reverts commit a35339c.

pavel-kirienko requested a review from thirtytwobits July 6, 2023 19:06

Switch from Google Test to Unity; make the internal tests include udp…

055358d

…ard.c directly (#37)

lydia-at-amazon approved these changes Jul 8, 2023

View reviewed changes

pavel-kirienko added 4 commits July 8, 2023 13:48

Add a test for UDPARD_MTU_DEFAULT_MAX_SINGLE_FRAME

7a85301

Do not leak memory from intrusive tests

f871b4a

Add a NOLINT comment in Cavl to suppress bogus warning from Clang flo…

a3f64f0

…w analyzer

Naming nits & formatting, nothing to see here

a6837dd

thirtytwobits approved these changes Jul 10, 2023

View reviewed changes

pavel-kirienko merged commit 5727576 into main2 Jul 10, 2023
24 checks passed

pavel-kirienko deleted the pavel branch July 10, 2023 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transmission pipeline and cosmetic adjustments to the API #34

Transmission pipeline and cosmetic adjustments to the API #34

pavel-kirienko commented Jul 1, 2023

thirtytwobits left a comment

thirtytwobits Jul 6, 2023

pavel-kirienko commented Jul 6, 2023

thirtytwobits commented Jul 6, 2023 •

edited

Loading

pavel-kirienko commented Jul 6, 2023

thirtytwobits commented Jul 6, 2023 •

edited

Loading

lydia-at-amazon left a comment

lydia-at-amazon Jul 7, 2023

pavel-kirienko Jul 8, 2023

lydia-at-amazon Jul 7, 2023

pavel-kirienko Jul 8, 2023

lydia-at-amazon Jul 8, 2023

pavel-kirienko Jul 8, 2023

lydia-at-amazon Jul 8, 2023

pavel-kirienko Jul 8, 2023

lydia-at-amazon Jul 8, 2023

pavel-kirienko Jul 8, 2023

lydia-at-amazon Jul 8, 2023

pavel-kirienko Jul 8, 2023

lydia-at-amazon Jul 8, 2023

pavel-kirienko Jul 8, 2023

lydia-at-amazon Jul 8, 2023

pavel-kirienko Jul 8, 2023

sonarcloud bot commented Jul 8, 2023

aentinger commented Jul 10, 2023

thirtytwobits commented Jul 10, 2023

Transmission pipeline and cosmetic adjustments to the API #34

Transmission pipeline and cosmetic adjustments to the API #34

Conversation

pavel-kirienko commented Jul 1, 2023

thirtytwobits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pavel-kirienko commented Jul 6, 2023

thirtytwobits commented Jul 6, 2023 • edited Loading

pavel-kirienko commented Jul 6, 2023

thirtytwobits commented Jul 6, 2023 • edited Loading

lydia-at-amazon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Jul 8, 2023

aentinger commented Jul 10, 2023

thirtytwobits commented Jul 10, 2023

thirtytwobits commented Jul 6, 2023 •

edited

Loading

thirtytwobits commented Jul 6, 2023 •

edited

Loading