From 52888aab6433348a5c0cfbd352575f8cb9862802 Mon Sep 17 00:00:00 2001 From: Andrew Tridgell Date: Mon, 17 Feb 2025 13:49:38 +1100 Subject: [PATCH] C++: simplify the linked list handling use a single linked list for message reception, not a list of lists this allows for the creation of new HandlerList handlers at runtime --- canard/handler_list.h | 20 +++++++++++---- canard/service_client.h | 55 +++++++++-------------------------------- canard/service_server.h | 20 ++++++--------- canard/subscriber.h | 46 +++++++--------------------------- 4 files changed, 43 insertions(+), 98 deletions(-) diff --git a/canard/handler_list.h b/canard/handler_list.h index 9c77e43..7178a80 100644 --- a/canard/handler_list.h +++ b/canard/handler_list.h @@ -85,8 +85,11 @@ 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; } @@ -94,7 +97,8 @@ class HandlerList { /// @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 index 7a1eee7..c766e91 100644 --- 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* 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* 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,8 +113,6 @@ class Client : public HandlerList, public Sender { } private: - static Client* branch_head[CANARD_NUM_HANDLERS]; - Client* next; uint8_t server_node_id; uint8_t req_buf[rsptype::cxx_iface::REQ_MAX_SIZE]; @@ -148,7 +120,4 @@ class Client : public HandlerList, public Sender { uint8_t transfer_id; }; -template -Client *Client::branch_head[] = {nullptr}; - } // namespace Canard diff --git a/canard/service_server.h b/canard/service_server.h index 6ad783e..da8a57c 100644 --- a/canard/service_server.h +++ b/canard/service_server.h @@ -41,9 +41,6 @@ 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 } @@ -51,23 +48,20 @@ class Server : public HandlerList { 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 index 21663b7..bda6e32 100644 --- a/canard/subscriber.h +++ b/canard/subscriber.h @@ -41,12 +41,8 @@ class Subscriber : public HandlerList { Subscriber(Callback &_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 @@ -54,49 +50,25 @@ class Subscriber : public HandlerList { // 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* 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* 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* next; - static Subscriber *branch_head[CANARD_NUM_HANDLERS]; Callback &cb; }; -template -Subscriber* Subscriber::branch_head[] = {nullptr}; - template class SubscriberArgCb { public: