From 600c29a9539787ee4de555dd1cbeeeae1854a93f Mon Sep 17 00:00:00 2001 From: Pavel Kirienko Date: Sun, 17 May 2015 16:29:19 +0300 Subject: [PATCH] NodeInfoRetriever - docs, logical fixes, tests --- .../uavcan/protocol/node_info_retriever.hpp | 73 +++++++++----- .../test/protocol/node_info_retriever.cpp | 94 ++++++++++++++++--- 2 files changed, 135 insertions(+), 32 deletions(-) diff --git a/libuavcan/include/uavcan/protocol/node_info_retriever.hpp b/libuavcan/include/uavcan/protocol/node_info_retriever.hpp index 4a10321e11..92a5e42471 100644 --- a/libuavcan/include/uavcan/protocol/node_info_retriever.hpp +++ b/libuavcan/include/uavcan/protocol/node_info_retriever.hpp @@ -61,8 +61,32 @@ public: /** * This class automatically retrieves a response to GetNodeInfo once a node appears online or restarts. * It does a number of attempts in case if there's a communication failure before assuming that the node does not - * implement the GetNodeInfo service. - * Events from this class can be routed to many subscribers. + * implement the GetNodeInfo service. All parameters are pre-configured with sensible default values that should fit + * virtually any use case, but they can be overriden if needed - refer to the setter methods below for details. + * + * Defaults are pre-configured so that the class is able to query 123 nodes (node ID 1..125, where 1 is our local + * node and 1 is one node that implements GetNodeInfo service, hence 123) of which none implements GetNodeInfo + * service in under 5 seconds. The 5 second limitation is imposed by UAVCAN-compatible bootloaders, which are + * unlikely to wait for more than that before continuing to boot. In case if this default value is not appropriate + * for the end application, the request interval can be overriden via @ref setRequestInterval(). + * + * Following the above explained requirements, the default request interval is defined as follows: + * request interval [ms] = foor(5000 [ms] bootloader timeout / 123 nodes) + * Which yields 40 ms. + * + * Given default service timeout 500 ms and the defined above request frequency 40 ms, the maximum number of + * concurrent requests will be: + * max concurrent requests = ceil(500 [ms] timeout / 40 [ms] request interval) + * Which yields 13 requests. + * + * Keep the above equations in mind when changing the default request interval. + * + * Obviously, if all calls are completing in under (request interval), the number of concurrent requests will never + * exceed one. This is actually the most likely scenario. + * + * Note that all nodes are queried in a round-robin fashion, regardless of their uptime, number of requests made, etc. + * + * Events from this class can be routed to many listeners, @ref INodeInfoListener. */ class UAVCAN_EXPORT NodeInfoRetriever : NodeStatusMonitor , TimerBase @@ -135,8 +159,9 @@ private: } }; - enum { NumStaticCalls = 4 }; - enum { DefaultNumRequestAttempts = 30 }; + enum { NumStaticCalls = 2 }; + enum { DefaultNumRequestAttempts = 16 }; + enum { DefaultTimerIntervalMSec = 40 }; ///< Read explanation in the class documentation /* * State @@ -147,16 +172,15 @@ private: ServiceClient get_node_info_client_; + MonotonicDuration request_interval_; + mutable uint8_t last_picked_node_; uint8_t num_attempts_; - uint8_t max_concurrent_requests_; /* * Methods */ - static MonotonicDuration getTimerPollInterval() { return MonotonicDuration::fromMSec(100); } - const Entry& getEntry(NodeID node_id) const { return const_cast(this)->getEntry(node_id); } Entry& getEntry(NodeID node_id) { @@ -171,9 +195,8 @@ private: { if (!TimerBase::isRunning()) { - TimerBase::startPeriodic(getTimerPollInterval()); - UAVCAN_TRACE("NodeInfoRetriever", "Timer started, interval %ld ms", - static_cast(getTimerPollInterval().toMSec())); + TimerBase::startPeriodic(request_interval_); + UAVCAN_TRACE("NodeInfoRetriever", "Timer started, interval %s sec", request_interval_.toString().c_str()); } } @@ -203,11 +226,6 @@ private: virtual void handleTimerEvent(const TimerEvent&) { - if (get_node_info_client_.getNumPendingCalls() >= max_concurrent_requests_) - { - return; - } - const NodeID next = pickNextNodeToQuery(); if (next.isUnicast()) { @@ -319,9 +337,9 @@ public: , TimerBase(node) , listeners_(node.getAllocator()) , get_node_info_client_(node) + , request_interval_(MonotonicDuration::fromMSec(DefaultTimerIntervalMSec)) , last_picked_node_(1) , num_attempts_(DefaultNumRequestAttempts) - , max_concurrent_requests_(NumStaticCalls * 2) { } /** @@ -381,6 +399,8 @@ public: } } + unsigned getNumListeners() const { return listeners_.getSize(); } + /** * Number of attempts to retrieve GetNodeInfo response before giving up on the assumption that the service is * not implemented. @@ -393,13 +413,24 @@ public: } /** - * Number of concurrent requests limits the number of simultaneous service calls to different nodes. - * This value cannot be less than one. + * Request interval also implicitly defines the maximum number of concurrent requests. + * Read the class documentation for details. */ - uint8_t getNumConcurrentRequests() const { return max_concurrent_requests_; } - void setNumConcurrentRequests(uint8_t num) + MonotonicDuration getRequestInterval() const { return request_interval_; } + void setRequestInterval(const MonotonicDuration interval) { - max_concurrent_requests_ = max(static_cast(1), num); + if (interval.isPositive()) + { + request_interval_ = interval; + if (TimerBase::isRunning()) + { + TimerBase::startPeriodic(request_interval_); + } + } + else + { + UAVCAN_ASSERT(0); + } } /** diff --git a/libuavcan/test/protocol/node_info_retriever.cpp b/libuavcan/test/protocol/node_info_retriever.cpp index 2b8f92d5d9..71088ebbef 100644 --- a/libuavcan/test/protocol/node_info_retriever.cpp +++ b/libuavcan/test/protocol/node_info_retriever.cpp @@ -92,6 +92,9 @@ TEST(NodeInfoRetriever, Basic) retr.removeListener(&listener); // Does nothing retr.addListener(&listener); + retr.addListener(&listener); + retr.addListener(&listener); + ASSERT_EQ(1, retr.getNumListeners()); uavcan::protocol::HardwareVersion hwver; hwver.unique_id[0] = 123; @@ -106,6 +109,8 @@ TEST(NodeInfoRetriever, Basic) ASSERT_FALSE(retr.isRetrievingInProgress()); ASSERT_EQ(0, retr.getNumPendingRequests()); + EXPECT_EQ(40, retr.getRequestInterval().toMSec()); // Default + /* * Waiting for discovery */ @@ -137,11 +142,11 @@ TEST(NodeInfoRetriever, Basic) publishNodeStatus(nodes.can_a, uavcan::NodeID(11), 0, 10, tid); publishNodeStatus(nodes.can_a, uavcan::NodeID(12), 0, 10, tid); - nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(110)); + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(45)); ASSERT_EQ(1, retr.getNumPendingRequests()); - nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(110)); + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(40)); ASSERT_EQ(2, retr.getNumPendingRequests()); - nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(110)); + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(40)); ASSERT_EQ(3, retr.getNumPendingRequests()); nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(1000)); ASSERT_TRUE(retr.isRetrievingInProgress()); @@ -151,11 +156,11 @@ TEST(NodeInfoRetriever, Basic) publishNodeStatus(nodes.can_a, uavcan::NodeID(11), 0, 11, tid); publishNodeStatus(nodes.can_a, uavcan::NodeID(12), 0, 11, tid); - nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(110)); + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(45)); ASSERT_EQ(1, retr.getNumPendingRequests()); - nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(110)); + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(40)); ASSERT_EQ(2, retr.getNumPendingRequests()); - nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(110)); + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(40)); ASSERT_EQ(3, retr.getNumPendingRequests()); nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(1000)); ASSERT_TRUE(retr.isRetrievingInProgress()); @@ -165,11 +170,11 @@ TEST(NodeInfoRetriever, Basic) publishNodeStatus(nodes.can_a, uavcan::NodeID(11), 0, 12, tid); publishNodeStatus(nodes.can_a, uavcan::NodeID(12), 0, 10, tid); // Reset - nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(110)); + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(45)); ASSERT_EQ(1, retr.getNumPendingRequests()); - nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(110)); + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(40)); ASSERT_EQ(2, retr.getNumPendingRequests()); - nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(110)); + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(40)); ASSERT_EQ(3, retr.getNumPendingRequests()); nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(1000)); ASSERT_TRUE(retr.isRetrievingInProgress()); @@ -181,9 +186,9 @@ TEST(NodeInfoRetriever, Basic) tid.increment(); publishNodeStatus(nodes.can_a, uavcan::NodeID(12), 0, 11, tid); - nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(110)); + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(45)); ASSERT_EQ(1, retr.getNumPendingRequests()); - nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(110)); + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(40)); ASSERT_EQ(1, retr.getNumPendingRequests()); // Still one because two went offline nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(1200)); ASSERT_TRUE(retr.isRetrievingInProgress()); @@ -199,3 +204,70 @@ TEST(NodeInfoRetriever, Basic) EXPECT_EQ(7, listener.status_change_cnt); // node 2 online/offline + 2 test nodes above online/offline + 1 EXPECT_EQ(3, listener.info_unavailable_cnt); } + + +TEST(NodeInfoRetriever, MaxConcurrentRequests) +{ + uavcan::GlobalDataTypeRegistry::instance().reset(); + uavcan::DefaultDataTypeRegistrator _reg1; + uavcan::DefaultDataTypeRegistrator _reg2; + uavcan::DefaultDataTypeRegistrator _reg3; + + InterlinkedTestNodesWithSysClock nodes; + + uavcan::NodeInfoRetriever retr(nodes.a); + std::cout << "sizeof(uavcan::NodeInfoRetriever): " << sizeof(uavcan::NodeInfoRetriever) << std::endl; + std::cout << "sizeof(uavcan::ServiceClient): " + << sizeof(uavcan::ServiceClient) << std::endl; + + NodeInfoListener listener; + + /* + * Initialization + */ + ASSERT_LE(0, retr.start()); + + retr.addListener(&listener); + ASSERT_EQ(1, retr.getNumListeners()); + + ASSERT_FALSE(retr.isRetrievingInProgress()); + ASSERT_EQ(0, retr.getNumPendingRequests()); + + ASSERT_EQ(40, retr.getRequestInterval().toMSec()); + + const unsigned MaxPendingRequests = 13; // See class docs + const unsigned MinPendingRequestsAtFullLoad = 12; + + /* + * Sending a lot of requests, making sure that the number of concurrent calls does not exceed the specified limit. + */ + for (uint8_t node_id = 1U; node_id <= 127U; node_id++) + { + publishNodeStatus(nodes.can_a, node_id, 0, 0, uavcan::TransferID()); + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(10)); + ASSERT_GE(MaxPendingRequests, retr.getNumPendingRequests()); + ASSERT_TRUE(retr.isRetrievingInProgress()); + } + + ASSERT_GE(MaxPendingRequests, retr.getNumPendingRequests()); + ASSERT_LE(MinPendingRequestsAtFullLoad, retr.getNumPendingRequests()); + + for (int i = 0; i < 8; i++) // Approximate + { + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(35)); + std::cout << "!!! SPIN " << i << " COMPLETE" << std::endl; + + ASSERT_GE(MaxPendingRequests, retr.getNumPendingRequests()); + ASSERT_LE(MinPendingRequestsAtFullLoad, retr.getNumPendingRequests()); + + ASSERT_TRUE(retr.isRetrievingInProgress()); + } + + ASSERT_LT(0, retr.getNumPendingRequests()); + ASSERT_TRUE(retr.isRetrievingInProgress()); + + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(5000)); + + ASSERT_EQ(0, retr.getNumPendingRequests()); + ASSERT_FALSE(retr.isRetrievingInProgress()); +}