Add LibUDPard demo #16

pavel-kirienko · 2023-09-04T11:04:15Z

The code is a little sloppy compared to the library itself but it should be adequate for the demo.

I moved the platform-specific parts -- the UDP stack and the storage I/O -- into separate modules to make the API surface clearly visible. This also simplifies porting if one wants to run this demo on an MCU.

I avoided building strong abstractions on top of LibUDPard because that may obscure the API of the library and attract attention of the reader to the features of the demo instead of those of the library itself. The flipside is that main.c has to be fairly large.

The Cavl library was added by copying the header file instead of adding the submodule because the entire library is just a single file alone so the submodule approach seemed like an overkill. We can change this if necessary, though. See a related question here: OpenCyphal/libudpard#11

thirtytwobits · 2023-09-06T23:47:20Z

👀

thirtytwobits

Will continue this review tomorrow but note that I get integer narrowing warnings from libudpard when building on 64-bit systems. Have you tried building on linux 64?

thirtytwobits · 2023-09-06T23:43:47Z

libudpard/src/memory_block.h

+    _Alignas(max_align_t) static uint_least8_t _name##_pool[(_block_size_bytes) * (_block_count)]; \
+    struct MemoryBlockAllocator _name = memoryBlockInit(sizeof(_name##_pool), &_name##_pool[0], (_block_size_bytes))
+
+/// The user shall not attempt to change any of the fields manually.


OH? Is that a challenge? Perhaps I will change these, thank-you-very-much!!!

I was going for the record for the smallest possible implementation of a block pool allocator. Counting 101 LoC at the moment.

libudpard/CMakeLists.txt

lydia-at-amazon · 2023-09-07T18:37:57Z

Very exciting to see! Will review more thoroughly tomorrow

libudpard_demo/src/udp.c

libudpard_demo/src/main.c

libudpard_demo/src/storage.c

libudpard_demo/src/main.c

libudpard_demo/src/storage.h

thirtytwobits · 2023-09-08T22:10:26Z

libudpard_demo/src/udp.c

+        ok = ok && (setsockopt(self->fd, SOL_SOCKET, SO_REUSEPORT, &reuse, sizeof(reuse)) == 0);
+#endif
+        // Binding to the multicast group address is necessary on GNU/Linux: https://habr.com/ru/post/141021/
+        // Binding to a multicast address is not allowed on Windows, and it is not necessary there;


So...this will fail on Windows?

Yes, this will fail on Windows. To make it work there, the binding has to be conditional. I could throw in some ifdefs in there but we don't really want to test it against Windows, do we?

It'd be nice if this worked on Windows, yes.

I added the required preprocessor check but this is not yet tested on Windows. I'm afraid that ensuring compatibility with Windows is a rather significant chunk of work.

… not usable here as it is not guaranteed to be defined on Linux.

libudpard_demo/src/main.c

lydia-at-amazon · 2023-09-14T19:07:19Z

libudpard_demo/src/udp.c

+/// Enable SO_REUSEPORT.
+#define _DEFAULT_SOURCE  // NOLINT(bugprone-reserved-identifier,cert-dcl37-c,cert-dcl51-cpp)


Does the SO_REUSEPORT comment relate to defining DEFAULT_SOURCE?

Yes. _DEFAULT_SOURCE is a feature test macro that enables certain non-POSIX extensions that are disabled by default when a strict standard mode is used; in this case, -std=c99. See https://man7.org/linux/man-pages/man7/feature_test_macros.7.html

lydia-at-amazon · 2023-09-14T20:35:53Z

libudpard_demo/src/udp.c

+    int16_t res = -EINVAL;
+    if ((self != NULL) && (local_iface_address > 0))
+    {
+        self->fd                 = socket(AF_INET, SOCK_DGRAM, IPPROTO_UDP);


Should we use SOCK_NONBLOCK? Guess that's more up to customers if they want to use that or not

On line 48, we have fcntl(self->fd, F_SETFL, O_NONBLOCK). We could move it here as well if desired.

lydia-at-amazon · 2023-09-14T21:24:37Z

libudpard_demo/src/main.c

+    }
+}
+
+static void cbOnNodeIDAllocationData(struct Subscriber* const self, struct UdpardRxTransfer* const transfer)


What does cb stand for?

Oh it stands for callback

libudpard_demo/src/main.c

lydia-at-amazon · 2023-09-14T22:57:57Z

libudpard_demo/src/main.c

+    }
+
+    // Process the RX sockets that became readable.
+    // The time has to be re-sampled because the blocking wait may have taken a long time.


What's the benefit of having the blocking wait?

It's just standard IO multiplexing. We suspend the thread when there's nothing to do. It is unblocked either when there is a socket to handle or the application needs to do something.

lydia-at-amazon · 2023-09-14T23:04:44Z

libudpard_demo/src/main.c

+            continue;
+        }
+        // Pass the data buffer into LibUDPard for further processing. It takes ownership of the buffer.
+        if (rx_aw[i].user_reference != NULL)  // This is used to differentiate subscription sockets from RPC sockets.


Is there not a better way to differentiate between messages and services than checking whether the user reference is NULL or not?

I ask myself the same question. We need to keep state per awaitable to differentiate between the two. We can't subtype it to add custom state because that breaks the array layout. All of the better solutions I could come up with are various shades of bad:

Pass awaitables as an array of pointers -- requires an extra array.

Use a linked list -- results in a clumsy API.

Add the required fields to the awaitable type -- breaks encapsulation.

I guess I better add an explanatory comment about this.

lydia-at-amazon · 2023-09-14T23:48:06Z

libudpard_demo/src/main.c

+    }          // Otherwise, it's a request from another allocation client node, or we already have a node-ID.
+}
+
+static void cbOnMyData(struct Subscriber* const self, struct UdpardRxTransfer* const transfer)


You could use the same callback for multiple subscribers, correct?

lydia-at-amazon · 2023-09-14T23:57:16Z

libudpard_demo/src/register.h

+///                         `____/ .___/`___/_/ /_/`____/`__, / .___/_/ /_/`__,_/_/
+///                             /_/                     /____/_/
+///
+/// In Cyphal, registers are named values that can be read and possibly written over the network. They are used


If everything is configured statically, are registers still needed?

No, they are quite optional. The only required thing is the heartbeat publisher.

lydia-at-amazon · 2023-09-15T00:18:30Z

General feeling is that main is a little complex but that's why we're going to provide Libcyphal as well to help abstract a lot of the setup required to use libudpard from the user.

pavel-kirienko added 24 commits August 16, 2023 15:26

Start libudpard demo

e784baa

Add dictionary

ac8ace9

Fix source path

b432a01

Add memory allocator

a1103a6

Builds

524933d

Runs quite well

acd7695

UID

39cc78b

Add registers

ad0337d

Refactor things and finish the network layer

96186b6

Implement rx/tx polling

e39af2e

Working IO

726ce85

Implement PnP allocation

428e3da

Fix allocation logic -- anonymous transfers are not deduplicated

9435af3

Fix dynamic unsubscription

a302714

Respect DSCP

98f4d28

Add RPC service support, but no services are implemented yet

5c389c9

Implement registers

29f4ab8

Switch libudpard to main

758774b

Implement uavcan.node.port.List publisher

032fc15

Implement ExecuteCommand

5083008

Implement the data subscriber and publisher

9da554d

Update the comments

8ed15b8

Add the docs for the libudpard demo

593b6bd

Remove unnecessary branch spec

db9d90f

pavel-kirienko requested a review from thirtytwobits September 4, 2023 11:04

pavel-kirienko self-assigned this Sep 4, 2023

Disable the easter egg and add a note about the diagnostic register

ab3bfad

thirtytwobits reviewed Sep 7, 2023

View reviewed changes

libudpard/CMakeLists.txt Outdated Show resolved Hide resolved

Flip NO_STATIC_ANALYSIS

c066c49

pavel-kirienko force-pushed the libudpard branch from 641d9db to c066c49 Compare September 7, 2023 10:56

lydia-at-amazon reviewed Sep 7, 2023

View reviewed changes

libudpard/CMakeLists.txt Outdated Show resolved Hide resolved

pavel-kirienko added 5 commits September 7, 2023 21:43

{libudpard}/* -> {libudpard_demos}/*

c72db5f

Remove the old libudpard submodule

ff3711a

Add new libudpard submodule

0334215

Fix ODR violation

0d95338

Fix path in README

02dc2d9

thirtytwobits reviewed Sep 8, 2023

View reviewed changes

pavel-kirienko added 2 commits September 9, 2023 02:16

Add explicit conversion to nfds_t with an overflow check. OPEN_MAX is…

c6f2dd9

… not usable here as it is not guaranteed to be defined on Linux.

Where is your dog now?

d786093

pavel-kirienko requested a review from thirtytwobits September 8, 2023 23:31

thirtytwobits reviewed Sep 9, 2023

View reviewed changes

libudpard_demo/src/main.c Show resolved Hide resolved

Make RX socket binding compatible with Windows

a9f522f

pavel-kirienko requested a review from thirtytwobits September 9, 2023 18:09

lydia-at-amazon approved these changes Sep 15, 2023

View reviewed changes

pavel-kirienko added 2 commits September 15, 2023 12:15

Describe the deal with UDPRxAwaitable::user_reference=NULL

bf9d929

aww

2d038c3

pavel-kirienko merged commit 961171b into main Sep 15, 2023

pavel-kirienko deleted the libudpard branch September 15, 2023 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add LibUDPard demo #16

Add LibUDPard demo #16

pavel-kirienko commented Sep 4, 2023

thirtytwobits commented Sep 6, 2023

thirtytwobits left a comment

thirtytwobits Sep 6, 2023

pavel-kirienko Sep 7, 2023

lydia-at-amazon commented Sep 7, 2023

thirtytwobits Sep 8, 2023

pavel-kirienko Sep 8, 2023

thirtytwobits Sep 9, 2023

pavel-kirienko Sep 9, 2023

lydia-at-amazon Sep 14, 2023

pavel-kirienko Sep 15, 2023

lydia-at-amazon Sep 14, 2023

pavel-kirienko Sep 15, 2023

lydia-at-amazon Sep 14, 2023

lydia-at-amazon Sep 14, 2023

lydia-at-amazon Sep 14, 2023

pavel-kirienko Sep 15, 2023

lydia-at-amazon Sep 14, 2023

pavel-kirienko Sep 15, 2023

lydia-at-amazon Sep 14, 2023

pavel-kirienko Sep 15, 2023

lydia-at-amazon Sep 14, 2023

pavel-kirienko Sep 15, 2023

lydia-at-amazon commented Sep 15, 2023 •

edited

		/// Enable SO_REUSEPORT.
		#define _DEFAULT_SOURCE // NOLINT(bugprone-reserved-identifier,cert-dcl37-c,cert-dcl51-cpp)

Add LibUDPard demo #16

Add LibUDPard demo #16

Conversation

pavel-kirienko commented Sep 4, 2023

thirtytwobits commented Sep 6, 2023

thirtytwobits left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lydia-at-amazon commented Sep 7, 2023

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

Choose a reason for hiding this comment

lydia-at-amazon commented Sep 15, 2023 • edited

lydia-at-amazon commented Sep 15, 2023 •

edited