diff --git a/libuavcan/include/uavcan/protocol/firmware_update_trigger.hpp b/libuavcan/include/uavcan/protocol/firmware_update_trigger.hpp index 6ab44f329a..9aaa65c97a 100644 --- a/libuavcan/include/uavcan/protocol/firmware_update_trigger.hpp +++ b/libuavcan/include/uavcan/protocol/firmware_update_trigger.hpp @@ -50,8 +50,8 @@ public: * @return True - the class will begin sending update requests. * False - the node will be ignored, no request will be sent. */ - virtual bool shouldSendFirmwareUpdateRequest(NodeID node_id, const protocol::GetNodeInfo::Response& node_info, - FirmwareFilePath& out_firmware_file_path) = 0; + virtual bool shouldRequestFirmwareUpdate(NodeID node_id, const protocol::GetNodeInfo::Response& node_info, + FirmwareFilePath& out_firmware_file_path) = 0; /** * This method will be invoked when a node responds to the update request with an error. If the request simply @@ -69,9 +69,9 @@ public: * @return True - the class will continue sending update requests with new firmware path. * False - the node will be forgotten, new requests will not be sent. */ - virtual bool shouldRetryFirmwareUpdateRequest(NodeID node_id, - const protocol::file::BeginFirmwareUpdate::Response& error_response, - FirmwareFilePath& out_firmware_file_path) = 0; + virtual bool shouldRetryFirmwareUpdate(NodeID node_id, + const protocol::file::BeginFirmwareUpdate::Response& error_response, + FirmwareFilePath& out_firmware_file_path) = 0; /** * This node is invoked when the node responds to the update request with confirmation. @@ -170,12 +170,16 @@ class FirmwareUpdateTrigger : public INodeInfoListener, /* * Methods of INodeInfoListener */ - virtual void handleNodeInfoUnavailable(NodeID) { /* Not used */ } + virtual void handleNodeInfoUnavailable(NodeID node_id) + { + UAVCAN_TRACE("FirmwareUpdateTrigger", "Node ID %d could not provide GetNodeInfo response", int(node_id.get())); + pending_nodes_.remove(node_id); // For extra paranoia + } virtual void handleNodeInfoRetrieved(const NodeID node_id, const protocol::GetNodeInfo::Response& node_info) { FirmwareFilePath firmware_file_path; - const bool update_needed = checker_.shouldSendFirmwareUpdateRequest(node_id, node_info, firmware_file_path); + const bool update_needed = checker_.shouldRequestFirmwareUpdate(node_id, node_info, firmware_file_path); if (update_needed) { UAVCAN_TRACE("FirmwareUpdateTrigger", "Node ID %d requires update; file path: %s", @@ -205,10 +209,8 @@ class FirmwareUpdateTrigger : public INodeInfoListener, void trySetPendingNode(const NodeID node_id, const FirmwareFilePath& path) { - FirmwareFilePath* const value = pending_nodes_.access(node_id); - if (value != NULL) + if (NULL != pending_nodes_.insert(node_id, path)) { - *value = path; if (!TimerBase::isRunning()) { TimerBase::startPeriodic(request_interval_); @@ -268,8 +270,8 @@ class FirmwareUpdateTrigger : public INodeInfoListener, { FirmwareFilePath firmware_file_path; const bool update_needed = - checker_.shouldRetryFirmwareUpdateRequest(result.getCallID().server_node_id, result.getResponse(), - firmware_file_path); + checker_.shouldRetryFirmwareUpdate(result.getCallID().server_node_id, result.getResponse(), + firmware_file_path); if (update_needed) { UAVCAN_TRACE("FirmwareUpdateTrigger", "Node ID %d requires retry; file path: %s", @@ -301,7 +303,7 @@ class FirmwareUpdateTrigger : public INodeInfoListener, FirmwareFilePath* const path = pending_nodes_.access(node_id); if (path == NULL) { - UAVCAN_ASSERT(0); + UAVCAN_ASSERT(0); // pickNextNodeID() returned a node ID that is not present in the map return; } @@ -345,7 +347,7 @@ public: } /** - * Starts the class. Once started, it can't be stopped unless destroyed. + * Starts the object. Once started, it can't be stopped unless destroyed. * * @param node_info_retriever The object will register itself against this retriever. * When the destructor is called, the object will unregister itself. @@ -422,7 +424,14 @@ public: * This method is mostly needed for testing. * When triggering is not in progress, the class consumes zero CPU time. */ - bool isTriggeringInProgress() const { return TimerBase::isRunning(); } + bool isTimerRunning() const { return TimerBase::isRunning(); } + + unsigned getNumPendingNodes() const + { + const unsigned ret = pending_nodes_.getSize(); + UAVCAN_ASSERT((ret > 0) ? isTimerRunning() : true); + return ret; + } }; } diff --git a/libuavcan/test/protocol/firmware_update_trigger.cpp b/libuavcan/test/protocol/firmware_update_trigger.cpp index 566a25fc07..9b1910a701 100644 --- a/libuavcan/test/protocol/firmware_update_trigger.cpp +++ b/libuavcan/test/protocol/firmware_update_trigger.cpp @@ -11,34 +11,66 @@ using namespace uavcan::protocol::file; struct FirmwareVersionChecker : public uavcan::IFirmwareVersionChecker { - virtual bool shouldSendFirmwareUpdateRequest(uavcan::NodeID node_id, - const uavcan::protocol::GetNodeInfo::Response& node_info, - FirmwareFilePath& out_firmware_file_path) + unsigned should_request_cnt; + unsigned should_retry_cnt; + unsigned confirmation_cnt; + + std::string firmware_path; + + bool should_retry; + std::string expected_node_name_to_update; + + BeginFirmwareUpdate::Response last_error_response; + + FirmwareVersionChecker() + : should_request_cnt(0) + , should_retry_cnt(0) + , confirmation_cnt(0) + , should_retry(false) + { } + + virtual bool shouldRequestFirmwareUpdate(uavcan::NodeID node_id, + const uavcan::protocol::GetNodeInfo::Response& node_info, + FirmwareFilePath& out_firmware_file_path) { - (void)node_id; - (void)node_info; - (void)out_firmware_file_path; - return false; + should_request_cnt++; + std::cout << "REQUEST? " << int(node_id.get()) << "\n" << node_info << std::endl; + out_firmware_file_path = firmware_path.c_str(); + return node_info.name == expected_node_name_to_update; } - virtual bool shouldRetryFirmwareUpdateRequest(uavcan::NodeID node_id, - const BeginFirmwareUpdate::Response& error_response, - FirmwareFilePath& out_firmware_file_path) + virtual bool shouldRetryFirmwareUpdate(uavcan::NodeID node_id, + const BeginFirmwareUpdate::Response& error_response, + FirmwareFilePath& out_firmware_file_path) { - (void)node_id; - (void)error_response; - (void)out_firmware_file_path; - return false; + last_error_response = error_response; + std::cout << "RETRY? " << int(node_id.get()) << "\n" << error_response << std::endl; + should_retry_cnt++; + out_firmware_file_path = firmware_path.c_str(); + return should_retry; } virtual void handleFirmwareUpdateConfirmation(uavcan::NodeID node_id, const BeginFirmwareUpdate::Response& response) { - (void)node_id; - (void)response; + confirmation_cnt++; + std::cout << "CONFIRMED " << int(node_id.get()) << "\n" << response << std::endl; } }; +static uint8_t response_error_code = 0; + +static void beginFirmwareUpdateRequestCallback( + const uavcan::ReceivedDataStructure& req, + uavcan::ServiceResponseDataStructure& res) +{ + std::cout << "REQUEST\n" << req << std::endl; + + res.error = response_error_code; + res.optional_error_message = "foobar"; +} + + TEST(FirmwareUpdateTrigger, Basic) { uavcan::GlobalDataTypeRegistry::instance().reset(); @@ -76,8 +108,87 @@ TEST(FirmwareUpdateTrigger, Basic) ASSERT_LE(0, provider->startAndPublish()); + ASSERT_FALSE(trigger.isTimerRunning()); + ASSERT_EQ(0, trigger.getNumPendingNodes()); + /* - * Discovering one node + * Updating one node + * The server that can confirm the request is not running yet */ + checker.firmware_path = "firmware_path"; + checker.expected_node_name_to_update = "Ivan"; + checker.should_retry = true; + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(2000)); + + ASSERT_TRUE(trigger.isTimerRunning()); + ASSERT_EQ(1, trigger.getNumPendingNodes()); + + ASSERT_EQ(1, checker.should_request_cnt); + ASSERT_EQ(0, checker.should_retry_cnt); + ASSERT_EQ(0, checker.confirmation_cnt); + + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(2000)); + + // Still running! + ASSERT_TRUE(trigger.isTimerRunning()); + ASSERT_EQ(1, trigger.getNumPendingNodes()); + + /* + * Starting the firmware update server that returns an error + * The checker will instruct the trigger to repeat + */ + uavcan::ServiceServer srv(nodes.b); + + ASSERT_LE(0, srv.start(beginFirmwareUpdateRequestCallback)); + + response_error_code = BeginFirmwareUpdate::Response::ERROR_UNKNOWN; + checker.should_retry = true; + + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(1100)); + + ASSERT_EQ(1, checker.should_request_cnt); + ASSERT_EQ(1, checker.should_retry_cnt); + ASSERT_EQ(0, checker.confirmation_cnt); + + // Still running! + ASSERT_TRUE(trigger.isTimerRunning()); + ASSERT_EQ(1, trigger.getNumPendingNodes()); + + /* + * Trying again, this time with ERROR_IN_PROGRESS + */ + response_error_code = BeginFirmwareUpdate::Response::ERROR_IN_PROGRESS; + checker.should_retry = false; + + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(2100)); + + ASSERT_EQ(1, checker.should_request_cnt); + ASSERT_EQ(1, checker.should_retry_cnt); + ASSERT_EQ(1, checker.confirmation_cnt); + + // Stopped! + ASSERT_FALSE(trigger.isTimerRunning()); + ASSERT_EQ(0, trigger.getNumPendingNodes()); + + /* + * Restarting the node info provider + * Now it doesn't need an update + */ + provider.reset(new uavcan::NodeStatusProvider(nodes.b)); + + provider->setName("Dmitry"); + provider->setHardwareVersion(hwver); + + ASSERT_LE(0, provider->startAndPublish()); + + nodes.spinBoth(uavcan::MonotonicDuration::fromMSec(2100)); + + ASSERT_EQ(2, checker.should_request_cnt); + ASSERT_EQ(1, checker.should_retry_cnt); + ASSERT_EQ(1, checker.confirmation_cnt); + + // Stopped! + ASSERT_FALSE(trigger.isTimerRunning()); + ASSERT_EQ(0, trigger.getNumPendingNodes()); }