C++: simplify the linked list handling

tridge · tridge · commit 5d7b725ce114 · 2025-05-06T09:51:35.000+10:00
use a single linked list for message reception, not a list of lists

this allows for the creation of new HandlerList handlers at runtime
diff --git a/canard/handler_list.h b/canard/handler_list.h
@@ -85,16 +85,20 @@ class HandlerList {
         while (entry != nullptr) {
             if (transfer.data_type_id == entry->msgid &&
                 entry->transfer_type == transfer.transfer_type) {
-                entry->handle_message(transfer);
-                return;
+                if (entry->handle_message(transfer) &&
+                    transfer.transfer_type != CanardTransferTypeBroadcast) {
+                    // we only allow one request or response for non-broadcast
+                    break;
+                }
             }
             entry = entry->next;
         }
     }
 
     /// @brief Method to handle a message implemented by the derived class
     /// @param transfer transfer object of the request
-    virtual void handle_message(const CanardRxTransfer& transfer) = 0;
+    /// @return true if the message is for this consumer
+    virtual bool handle_message(const CanardRxTransfer& transfer) = 0;
 
 protected:
     virtual ~HandlerList() {}
@@ -104,14 +108,20 @@ class HandlerList {
     static Canard::Semaphore sem[CANARD_NUM_HANDLERS];
 #endif
 
-    // add ourselves to the handler list. the caller must be holding the semaphore.
+    // add ourselves to the handler list
     void link(void) NOINLINE_FUNC {
+#ifdef WITH_SEMAPHORE
+        WITH_SEMAPHORE(sem[index]);
+#endif
         next = head[index];
         head[index] = this;
     }
 
-    // remove ourselves from the handler list. the caller must be holding the semaphore.
+    // remove ourselves from the handler list
     void unlink(void) NOINLINE_FUNC {
+#ifdef WITH_SEMAPHORE
+        WITH_SEMAPHORE(sem[index]);
+#endif
         HandlerList* entry = head[index];
         if (entry == this) {
             head[index] = next;
diff --git a/canard/service_client.h b/canard/service_client.h
@@ -41,56 +41,30 @@ class Client : public HandlerList, public Sender {
     Sender(_interface),
     server_node_id(255),
     cb(_cb) {
-#ifdef WITH_SEMAPHORE
-        WITH_SEMAPHORE(sem[index]);
-#endif
-        next = branch_head[index];
-        branch_head[index] = this;
-        link(); // link ourselves into the handler list now that we're in the branch list
+        // link ourselves into the handler list
+        link();
     }
 
     // delete copy constructor and assignment operator
     Client(const Client&) = delete;
 
-    // destructor, remove the entry from the singly-linked list
+    // destructor
     ~Client() NOINLINE_FUNC {
-#ifdef WITH_SEMAPHORE
-        WITH_SEMAPHORE(sem[index]);
-#endif
-        unlink(); // unlink ourselves from the handler list before the branch list
-        Client<rsptype>* entry = branch_head[index];
-        if (entry == this) {
-            branch_head[index] = next;
-            return;
-        }
-        while (entry != nullptr) {
-            if (entry->next == this) {
-                entry->next = next;
-                return;
-            }
-            entry = entry->next;
-        }
+        // unlink ourselves from the handler list
+        unlink();
     }
 
     /// @brief handles incoming messages
     /// @param transfer transfer object of the request
-    void handle_message(const CanardRxTransfer& transfer) override NOINLINE_FUNC {
+    bool handle_message(const CanardRxTransfer& transfer) override NOINLINE_FUNC {
         rsptype msg {};
-        if (rsptype::cxx_iface::rsp_decode(&transfer, &msg)) {
-            // invalid decode
-            return;
-        }
-
-        // scan through the list of entries for corresponding server node id and transfer id
-        Client<rsptype>* entry = branch_head[index];
-        while (entry != nullptr) {
-            if (entry->server_node_id == transfer.source_node_id
-                && entry->transfer_id == transfer.transfer_id) {
-                entry->cb(transfer, msg);
-                return;
-            }
-            entry = entry->next;
+        if (server_node_id == transfer.source_node_id &&
+            transfer_id == transfer.transfer_id &&
+            !rsptype::cxx_iface::rsp_decode(&transfer, &msg)) {
+                cb(transfer, msg);
+                return true;
         }
+        return false;
     }
 
     /// @brief makes service request
@@ -139,16 +113,11 @@ class Client : public HandlerList, public Sender {
     }
 
 private:
-    static Client<rsptype>* branch_head[CANARD_NUM_HANDLERS];
-    Client<rsptype>* next;
     uint8_t server_node_id;
 
     uint8_t req_buf[rsptype::cxx_iface::REQ_MAX_SIZE];
     Callback<rsptype> &cb;
     uint8_t transfer_id;
 };
 
-template <typename rsptype>
-Client<rsptype> *Client<rsptype>::branch_head[] = {nullptr};
-
 } // namespace Canard
diff --git a/canard/service_server.h b/canard/service_server.h
@@ -41,33 +41,27 @@ class Server : public HandlerList {
     HandlerList(CanardTransferTypeRequest, reqtype::cxx_iface::ID, reqtype::cxx_iface::SIGNATURE, _interface.get_index()),
     interface(_interface),
     cb(_cb) {
-#ifdef WITH_SEMAPHORE
-        WITH_SEMAPHORE(sem[index]);
-#endif
         link(); // link ourselves into the handler list
     }
 
     // delete copy constructor and assignment operator
     Server(const Server&) = delete;
 
     ~Server() NOINLINE_FUNC {
-#ifdef WITH_SEMAPHORE
-        WITH_SEMAPHORE(sem[index]);
-#endif
         unlink(); // unlink ourselves from the handler list
     }
 
     /// @brief handles incoming messages
     /// @param transfer transfer object of the request
-    void handle_message(const CanardRxTransfer& transfer) override NOINLINE_FUNC {
+    /// @return true if message has been handled
+    bool handle_message(const CanardRxTransfer& transfer) override NOINLINE_FUNC {
         reqtype msg {};
-        if (reqtype::cxx_iface::req_decode(&transfer, &msg)) {
-            // invalid decode
-            return;
+        if (!reqtype::cxx_iface::req_decode(&transfer, &msg)) {
+            transfer_id = transfer.transfer_id;
+            cb(transfer, msg);
+            return true;
         }
-        transfer_id = transfer.transfer_id;
-        // call the registered callback
-        cb(transfer, msg);
+        return false;
     }
 
     /// @brief Send a response to the request
diff --git a/canard/subscriber.h b/canard/subscriber.h
@@ -41,62 +41,34 @@ class Subscriber : public HandlerList {
     Subscriber(Callback<msgtype> &_cb, uint8_t _index) NOINLINE_FUNC :
     HandlerList(CanardTransferTypeBroadcast, msgtype::cxx_iface::ID, msgtype::cxx_iface::SIGNATURE, _index),
     cb (_cb) {
-#ifdef WITH_SEMAPHORE
-        WITH_SEMAPHORE(sem[index]);
-#endif
-        next = branch_head[index];
-        branch_head[index] = this;
-        link(); // link ourselves into the handler list now that we're in the branch list
+        // link ourselves into the handler list
+        link();
     }
 
     // delete copy constructor and assignment operator
     Subscriber(const Subscriber&) = delete;
 
     // destructor, remove the entry from the singly-linked list
     ~Subscriber() NOINLINE_FUNC {
-#ifdef WITH_SEMAPHORE
-        WITH_SEMAPHORE(sem[index]);
-#endif
-        unlink(); // unlink ourselves from the handler list before the branch list
-        Subscriber<msgtype>* entry = branch_head[index];
-        if (entry == this) {
-            branch_head[index] = next;
-            return;
-        }
-        while (entry != nullptr) {
-            if (entry->next == this) {
-                entry->next = next;
-                return;
-            }
-            entry = entry->next;
-        }
+        // unlink ourselves from the handler list
+        unlink();
     }
 
     /// @brief parse the message and call the callback
     /// @param transfer transfer object
-    void handle_message(const CanardRxTransfer& transfer) override NOINLINE_FUNC {
+    bool handle_message(const CanardRxTransfer& transfer) override NOINLINE_FUNC {
         msgtype msg {};
-        if (msgtype::cxx_iface::decode(&transfer, &msg)) {
-            // invalid decode
-            return;
-        }
-        // call all registered callbacks in one go
-        Subscriber<msgtype>* entry = branch_head[index];
-        while (entry != nullptr) {
-            entry->cb(transfer, msg);
-            entry = entry->next;
+        if (!msgtype::cxx_iface::decode(&transfer, &msg)) {
+            cb(transfer, msg);
+            return true;
         }
+        return false;
     }
 
 private:
-    Subscriber<msgtype>* next;
-    static Subscriber<msgtype> *branch_head[CANARD_NUM_HANDLERS];
     Callback<msgtype> &cb;
 };
 
-template <typename msgtype>
-Subscriber<msgtype>* Subscriber<msgtype>::branch_head[] = {nullptr};
-
 template <typename T, typename msgtype>
 class SubscriberArgCb {
 public:
