Improve the README and the transmission API (#54)

Improve the TX API usability for the case of redundant interfaces: do not increment the transfer-ID automatically so that the caller does not have to restore the original value before sending a redundant copy of the transfer

Author: pavel-kirienko

README.md

@@ -53,6 +53,26 @@ and the network layer below the library using a third-party UDP/IP stack impleme
 In the most straightforward case, the network layer can be based on the standard Berkeley socket API
 or a lightweight embedded stack such as LwIP.
 
+```mermaid
+%%{init: {"fontFamily": "Ubuntu Mono, monospace", "flowchart": {"curve": "basis"}}}%%
+flowchart TD
+    classDef OpenCyphal color:#00DAC6,fill:#1700b3,stroke:#00DAC6,stroke-width:2px,font-weight:600
+    Application <-->|messages,\nrequests,\nresponses| LibUDPard[fa:fa-code LibUDPard]
+    class LibUDPard OpenCyphal
+    LibUDPard <-->|multicast datagrams| UDP
+    subgraph domain_udpip["3rd-party UDP/IP+IGMP stack"]
+        UDP <--> IP["IPv4, IGMPv1+"] <--> MAC
+    end
+    MAC <--> PHY
+```
+
+To integrate the library into your application, simply copy the files under `libudpard/` into your project tree,
+or add this entire repository as a submodule.
+The library contains only one translation unit named `udpard.c`;
+no special compiler options are needed to build it.
+The library should be compatible with all conventional computer architectures where a standards-compliant C99 compiler
+is available.
+
 **Read the API docs in [`libudpard/udpard.h`](libudpard/udpard.h).**
 For complete usage examples, please refer to <https://github.com/OpenCyphal-Garage/demos>.
 

libudpard/udpard.c

@@ -548,14 +548,13 @@ int32_t udpardTxPublish(struct UdpardTx* const     self,
                         const UdpardMicrosecond    deadline_usec,
                         const enum UdpardPriority  priority,
                         const UdpardPortID         subject_id,
-                        UdpardTransferID* const    transfer_id,
+                        const UdpardTransferID     transfer_id,
                         const struct UdpardPayload payload,
                         void* const                user_transfer_reference)
 {
     int32_t    out     = -UDPARD_ERROR_ARGUMENT;
     const bool args_ok = (self != NULL) && (self->local_node_id != NULL) && (priority <= UDPARD_PRIORITY_MAX) &&
-                         (subject_id <= UDPARD_SUBJECT_ID_MAX) && (transfer_id != NULL) &&
-                         ((payload.data != NULL) || (payload.size == 0U));
+                         (subject_id <= UDPARD_SUBJECT_ID_MAX) && ((payload.data != NULL) || (payload.size == 0U));
     if (args_ok)
     {
         out = txPush(self,
@@ -564,16 +563,12 @@ int32_t udpardTxPublish(struct UdpardTx* const     self,
                          .priority       = priority,
                          .src_node_id    = *self->local_node_id,
                          .dst_node_id    = UDPARD_NODE_ID_UNSET,
-                         .transfer_id    = *transfer_id,
+                         .transfer_id    = transfer_id,
                          .data_specifier = subject_id,
                      },
                      makeSubjectUDPIPEndpoint(subject_id),
                      payload,
                      user_transfer_reference);
-        if (out > 0)
-        {
-            ++(*transfer_id);
-        }
     }
     return out;
 }
@@ -583,14 +578,14 @@ int32_t udpardTxRequest(struct UdpardTx* const     self,
                         const enum UdpardPriority  priority,
                         const UdpardPortID         service_id,
                         const UdpardNodeID         server_node_id,
-                        UdpardTransferID* const    transfer_id,
+                        const UdpardTransferID     transfer_id,
                         const struct UdpardPayload payload,
                         void* const                user_transfer_reference)
 {
     int32_t    out     = -UDPARD_ERROR_ARGUMENT;
     const bool args_ok = (self != NULL) && (self->local_node_id != NULL) && (priority <= UDPARD_PRIORITY_MAX) &&
                          (service_id <= UDPARD_SERVICE_ID_MAX) && (server_node_id <= UDPARD_NODE_ID_MAX) &&
-                         (transfer_id != NULL) && ((payload.data != NULL) || (payload.size == 0U));
+                         ((payload.data != NULL) || (payload.size == 0U));
     if (args_ok)
     {
         out = txPush(self,
@@ -599,17 +594,13 @@ int32_t udpardTxRequest(struct UdpardTx* const     self,
                          .priority       = priority,
                          .src_node_id    = *self->local_node_id,
                          .dst_node_id    = server_node_id,
-                         .transfer_id    = *transfer_id,
+                         .transfer_id    = transfer_id,
                          .data_specifier = DATA_SPECIFIER_SERVICE_NOT_MESSAGE_MASK |
                                            DATA_SPECIFIER_SERVICE_REQUEST_NOT_RESPONSE_MASK | service_id,
                      },
                      makeServiceUDPIPEndpoint(server_node_id),
                      payload,
                      user_transfer_reference);
-        if (out > 0)
-        {
-            ++(*transfer_id);
-        }
     }
     return out;
 }

libudpard/udpard.h

@@ -506,12 +506,12 @@ int_fast8_t udpardTxInit(struct UdpardTx* const            self,
 /// The MTU of the generated datagrams is dependent on the value of the MTU setting at the time when this function
 /// is invoked. The MTU setting can be changed arbitrarily between invocations.
 ///
-/// The pointer to the transfer_id will be used to populate the transfer_id field of the generated datagrams and
-/// then to increment the pointed-to value to prepare it for the next publication.
+/// The transfer_id parameter will be used to populate the transfer_id field of the generated datagrams.
+/// The caller shall increment the transfer-ID counter after each successful invocation of this function
+/// per redundant interface; the same transfer published over redundant interfaces shall have the same transfer-ID.
 /// There shall be a separate transfer-ID counter per subject (topic).
-/// The lifetime of the pointed-to transfer-ID counter must exceed the lifetime of the intent to publish on this
-/// subject (topic); one common approach is to use a static variable.
-/// The transfer-ID counter is not modified if the function fails.
+/// The lifetime of the transfer-ID counter must exceed the lifetime of the intent to publish on this subject (topic);
+/// one common approach is to use a static variable or a field in a type that contains the state of the publisher.
 ///
 /// The user_transfer_reference is an opaque pointer that will be assigned to the user_transfer_reference field of
 /// each enqueued item. The library itself does not use or check this value in any way, so it can be NULL if not needed.
@@ -558,19 +558,21 @@ int32_t udpardTxPublish(struct UdpardTx* const     self,
                         const UdpardMicrosecond    deadline_usec,
                         const enum UdpardPriority  priority,
                         const UdpardPortID         subject_id,
-                        UdpardTransferID* const    transfer_id,
+                        const UdpardTransferID     transfer_id,
                         const struct UdpardPayload payload,
                         void* const                user_transfer_reference);
 
 /// This is similar to udpardTxPublish except that it is intended for service request transfers.
 /// It takes the node-ID of the server that is intended to receive the request.
 ///
-/// The pointer to the transfer_id will be used to populate the transfer_id field of the generated datagrams and
-/// then to increment the pointed-to value to prepare it for the next request.
+/// The transfer_id parameter will be used to populate the transfer_id field of the generated datagrams.
+/// The caller shall increment the transfer-ID counter after each successful invocation of this function
+/// per redundant interface; the same transfer published over redundant interfaces shall have the same transfer-ID.
 /// There shall be a separate transfer-ID counter per pair of (service-ID, server node-ID).
-/// The lifetime of the pointed-to transfer-ID counter must exceed the lifetime of the intent to invoke this service
-/// on this server node; one common approach is to use a static array indexed by the server node-ID per service-ID
-/// (memory-constrained applications may choose a more compact container).
+/// The lifetime of the transfer-ID counter must exceed the lifetime of the intent to invoke this service
+/// on this server node; one common approach is to use a static array or a struct field indexed by
+/// the server node-ID per service-ID (memory-constrained applications may choose a more compact container;
+/// e.g., a list or an AVL tree).
 ///
 /// Additional error conditions:
 ///     - UDPARD_ERROR_ARGUMENT if the server node-ID value is invalid.
@@ -582,14 +584,13 @@ int32_t udpardTxRequest(struct UdpardTx* const     self,
                         const enum UdpardPriority  priority,
                         const UdpardPortID         service_id,
                         const UdpardNodeID         server_node_id,
-                        UdpardTransferID* const    transfer_id,
+                        const UdpardTransferID     transfer_id,
                         const struct UdpardPayload payload,
                         void* const                user_transfer_reference);
 
-/// This is similar to udpardTxRequest except that it takes the node-ID of the client instead of server,
-/// and the transfer-ID is passed by value rather than by pointer.
-/// The transfer-ID is passed by value because when responding to an RPC-service request, the server must
-/// reuse the transfer-ID value of the request (this is to allow the client to match responses with their requests).
+/// This is similar to udpardTxRequest except that it takes the node-ID of the client instead of server.
+/// The transfer-ID must be the same as that of the corresponding RPC-request transfer;
+/// this is to allow the client to match responses with their requests.
 int32_t udpardTxRespond(struct UdpardTx* const     self,
                         const UdpardMicrosecond    deadline_usec,
                         const enum UdpardPriority  priority,

tests/src/test_e2e.cpp

@@ -101,7 +101,7 @@ void testPubSub()
                                       10'000'000,
                                       UdpardPrioritySlow,
                                       5000,
-                                      &transfer_id.at(0),
+                                      transfer_id.at(0)++,
                                       makePayload("Last night, I had a dream."),
                                       nullptr));
     const std::string_view Eden =
@@ -114,7 +114,7 @@ void testPubSub()
                                       10'001'000,
                                       UdpardPriorityNominal,
                                       5000,
-                                      &transfer_id.at(0),
+                                      transfer_id.at(0),
                                       makePayload(Eden),
                                       nullptr));
     node_id = 42;  // Change the node-ID to allow multi-frame transfers, then try again.
@@ -123,7 +123,7 @@ void testPubSub()
                                       10'002'000,
                                       UdpardPriorityOptional,
                                       5000,
-                                      &transfer_id.at(0),
+                                      transfer_id.at(0)++,
                                       makePayload(Eden),
                                       nullptr));
     TEST_ASSERT_EQUAL(5, tx.queue_size);
@@ -136,7 +136,7 @@ void testPubSub()
                                       10'003'000,
                                       UdpardPriorityNominal,
                                       5001,
-                                      &transfer_id.at(1),
+                                      transfer_id.at(1)++,
                                       makePayload(Later),
                                       nullptr));
     TEST_ASSERT_EQUAL(6, tx.queue_size);
@@ -148,7 +148,7 @@ void testPubSub()
                                       10'004'000,
                                       UdpardPriorityNominal,
                                       5002,
-                                      &transfer_id.at(2),
+                                      transfer_id.at(2)++,
                                       makePayload(Dark),
                                       nullptr));
     TEST_ASSERT_EQUAL(7, tx.queue_size);
@@ -446,7 +446,7 @@ void testRPC()
                                             UdpardPriorityFast,
                                             200,
                                             4321,
-                                            &transfer_id_shared,
+                                            transfer_id_shared++,
                                             makePayload(Entry),
                                             nullptr));
     TEST_ASSERT_EQUAL(1, tx.queue_size);
@@ -464,7 +464,6 @@ void testRPC()
                                             makePayload(Forest),
                                             nullptr));
     TEST_ASSERT_EQUAL(2, tx.queue_size);
-    TEST_ASSERT_EQUAL(1, transfer_id_shared);  // Not incremented.
 
     // Transmit the enqueued frames by pushing them into the RPC dispatcher.
     UdpardRxRPCTransfer transfer{};