From e9e224282c91a19c0ca1a4ac391aa13c2dcc1077 Mon Sep 17 00:00:00 2001 From: Balazs Racz Date: Sun, 25 Sep 2022 11:26:58 +0200 Subject: [PATCH 1/7] Adds a virtual function to If object called `get_default_node_id`. This function returns the node ID of the default node on an interface. For single-node interfaces like SimpleStack this is always the same as the node ID of that single node. Generally SimpleStack takes a NodeID on the constructor, and that node ID is returned. This is well defined, because a default node ID is needed for both CAN and TCP interfaces by the transport standard: - The CAN interface needs a node ID while allocating aliases. - The TCP interface needs to add a last gateway node ID into every message header. --- src/openlcb/If.hxx | 5 +++++ src/openlcb/IfCan.cxx | 9 +++++++++ src/openlcb/IfCan.cxxtest | 5 +++++ src/openlcb/IfCan.hxx | 2 ++ src/openlcb/IfTcp.cxx | 5 +++++ src/openlcb/IfTcp.cxxtest | 7 ++++++- src/openlcb/IfTcp.hxx | 3 +++ src/openlcb/IfTcpImpl.hxx | 6 ++++++ src/utils/async_if_test_helper.hxx | 9 ++++++++- 9 files changed, 49 insertions(+), 2 deletions(-) diff --git a/src/openlcb/If.hxx b/src/openlcb/If.hxx index f76384c2c..80cd7a02a 100644 --- a/src/openlcb/If.hxx +++ b/src/openlcb/If.hxx @@ -325,6 +325,11 @@ public: * on the interface executor. */ virtual void canonicalize_handle(NodeHandle *h) {} + /// @return the node ID of the default node on this interface. For TCP + /// interfaces this is the gateway node ID, for CAN interfaces this is the + /// node ID used for alias allocation on the CAN-bus. + virtual NodeID get_default_node_id() = 0; + /// Sets a transmit hook. This function will be called once for every /// OpenLCB message transmitted. Used for implementing activity LEDs. /// @param hook function to call for each transmit message. diff --git a/src/openlcb/IfCan.cxx b/src/openlcb/IfCan.cxx index 704b16f23..6f352af1e 100644 --- a/src/openlcb/IfCan.cxx +++ b/src/openlcb/IfCan.cxx @@ -805,4 +805,13 @@ Node *IfCan::lookup_local_node_handle(NodeHandle h) return lookup_local_node(h.id); } +NodeID IfCan::get_default_node_id() +{ + if (!aliasAllocator_) + { + return 0; + } + return aliasAllocator_->if_node_id(); +} + } // namespace openlcb diff --git a/src/openlcb/IfCan.cxxtest b/src/openlcb/IfCan.cxxtest index 5a7c267b0..978e03f75 100644 --- a/src/openlcb/IfCan.cxxtest +++ b/src/openlcb/IfCan.cxxtest @@ -141,6 +141,11 @@ TEST_F(AsyncIfTest, AMEReceiveSupport) wait(); } +TEST_F(AsyncIfTest, GetDefaultNodeId) +{ + EXPECT_EQ(TEST_NODE_ID, ifCan_->get_default_node_id()); +} + TEST_F(AsyncNodeTest, GlobalAMESendSupport) { EXPECT_TRUE(node_->is_initialized()); diff --git a/src/openlcb/IfCan.hxx b/src/openlcb/IfCan.hxx index b5121483f..442179d96 100644 --- a/src/openlcb/IfCan.hxx +++ b/src/openlcb/IfCan.hxx @@ -132,6 +132,8 @@ public: Node *lookup_local_node_handle(NodeHandle handle) override; + NodeID get_default_node_id() override; + private: void canonicalize_handle(NodeHandle *h) override; diff --git a/src/openlcb/IfTcp.cxx b/src/openlcb/IfTcp.cxx index df91922f5..5a659cc3e 100644 --- a/src/openlcb/IfTcp.cxx +++ b/src/openlcb/IfTcp.cxx @@ -210,6 +210,11 @@ IfTcp::~IfTcp() } } +NodeID IfTcp::get_default_node_id() +{ + return sendFlow_->get_gateway_node_id(); +} + } // namespace openlcb #endif // OPENMRN_FEATURE_EXECUTOR_SELECT diff --git a/src/openlcb/IfTcp.cxxtest b/src/openlcb/IfTcp.cxxtest index 08431a20b..a33dbeb52 100644 --- a/src/openlcb/IfTcp.cxxtest +++ b/src/openlcb/IfTcp.cxxtest @@ -151,7 +151,7 @@ protected: HubPortInterface *fakeSource_ = (HubPortInterface *)123456; TestSequenceGenerator seq_; static constexpr uint64_t GW_NODE_ID = 0x101112131415ULL; - LocalIf localIf_{10}; + LocalIf localIf_{10, GW_NODE_ID}; TcpSendFlow sendFlow_{ &localIf_, GW_NODE_ID, &fakeSendTarget_, fakeSource_, &seq_}; }; @@ -409,6 +409,11 @@ TEST_F(TcpIfTest, create) { } +TEST_F(TcpIfTest, get_node_id) +{ + EXPECT_EQ(GW_NODE_ID, ifTcp_.get_default_node_id()); +} + TEST_F(TcpIfTest, send_message_global) { capture_next_packet(); diff --git a/src/openlcb/IfTcp.hxx b/src/openlcb/IfTcp.hxx index 96abaecef..9f063df28 100644 --- a/src/openlcb/IfTcp.hxx +++ b/src/openlcb/IfTcp.hxx @@ -113,6 +113,9 @@ public: /// @param on_error will be invoked when a socket error is encountered. void add_network_fd(int fd, Notifiable *on_error = nullptr); + /// @return the gateway node ID. + NodeID get_default_node_id() override; + private: /// Where to send traffic to. HubFlow *device_; diff --git a/src/openlcb/IfTcpImpl.hxx b/src/openlcb/IfTcpImpl.hxx index fb26358c0..da95b5042 100644 --- a/src/openlcb/IfTcpImpl.hxx +++ b/src/openlcb/IfTcpImpl.hxx @@ -303,6 +303,12 @@ public: { } + /// @return the node ID the gateway uses to set on outgoing messages. + NodeID get_gateway_node_id() + { + return gatewayId_; + } + private: /// Handler where dequeueing of messages to be sent starts. /// @return next state diff --git a/src/utils/async_if_test_helper.hxx b/src/utils/async_if_test_helper.hxx index ea6e5cd11..473f379ec 100644 --- a/src/utils/async_if_test_helper.hxx +++ b/src/utils/async_if_test_helper.hxx @@ -325,8 +325,9 @@ static const NodeID TEST_NODE_ID = 0x02010d000003ULL; class LocalIf : public If { public: - LocalIf(int local_nodes_count) + LocalIf(int local_nodes_count, NodeID gateway_node_id) : If(&g_executor, local_nodes_count) + , gatewayNodeID_(gateway_node_id) { } @@ -352,8 +353,14 @@ public: return false; } + NodeID get_default_node_id() override + { + return gatewayNodeID_; + } + private: std::vector> ownedFlows_; + NodeID gatewayNodeID_; }; /** Test fixture base class with helper methods for exercising the asynchronous From f066f1f7276b39b0d3807bf95cd944bea34ae9a3 Mon Sep 17 00:00:00 2001 From: Balazs Racz Date: Sun, 25 Sep 2022 11:32:12 +0200 Subject: [PATCH 2/7] Updates to the handle factory reset function: - separates to a standard implementation and an application specific weak implementation. - adds openlcb error code as return - add the node ID that is being factory reset as an argument - The standard function delegates to the app function if it detects that a non-default node is being factory reset (virtual node on the same interface) - the config service factoy reset is only called when the default node is being factory reset. --- src/openlcb/MemoryConfig.cxx | 23 ++++++++++++++++++----- src/openlcb/MemoryConfig.hxx | 12 ++++++++++-- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/src/openlcb/MemoryConfig.cxx b/src/openlcb/MemoryConfig.cxx index f6d6ee215..cd4f0a843 100644 --- a/src/openlcb/MemoryConfig.cxx +++ b/src/openlcb/MemoryConfig.cxx @@ -73,13 +73,26 @@ static constexpr unsigned FACTORY_RESET_REBOOT_DELAY_MSEC = 50; static constexpr unsigned FACTORY_RESET_REBOOT_DELAY_MSEC = 500; #endif +uint16_t __attribute__((weak, noinline)) +MemoryConfigHandler::app_handle_factory_reset(NodeID target) +{ + return Defs::ERROR_UNIMPLEMENTED; +} -void __attribute__((weak)) MemoryConfigHandler::handle_factory_reset() +uint16_t MemoryConfigHandler::handle_factory_reset(NodeID target) { - static_cast(ConfigUpdateFlow::instance()) - ->factory_reset(); - (new RebootTimer(service())) - ->start(MSEC_TO_NSEC(FACTORY_RESET_REBOOT_DELAY_MSEC)); + if (target == dg_service()->iface()->get_default_node_id()) + { + static_cast(ConfigUpdateFlow::instance()) + ->factory_reset(); + (new RebootTimer(service())) + ->start(MSEC_TO_NSEC(FACTORY_RESET_REBOOT_DELAY_MSEC)); + return 0; + } + else + { + return app_handle_factory_reset(target); + } } FileMemorySpace::FileMemorySpace(int fd, address_t len) diff --git a/src/openlcb/MemoryConfig.hxx b/src/openlcb/MemoryConfig.hxx index 218a7b151..2f84f30a8 100644 --- a/src/openlcb/MemoryConfig.hxx +++ b/src/openlcb/MemoryConfig.hxx @@ -484,8 +484,16 @@ private: } case MemoryConfigDefs::COMMAND_FACTORY_RESET: { - handle_factory_reset(); - return respond_ok(0); + NodeID id = message()->data()->dst->node_id(); + uint16_t ret = handle_factory_reset(id); + if (!ret) + { + return respond_ok(0); + } + else + { + return respond_reject(ret); + } } case MemoryConfigDefs::COMMAND_OPTIONS: { From 8cf8b78a5ad609264ebdf02ae3fa6d7e5703f742 Mon Sep 17 00:00:00 2001 From: Balazs Racz Date: Sun, 25 Sep 2022 11:33:25 +0200 Subject: [PATCH 3/7] Adds standard safety check to factory reset command carrying a node ID argument. The standard requires the factory reset datagram to contain a redundant parameter which repeats the target node's node ID. This PR checks that this parameter is supplied and correctly filled in. --- src/openlcb/MemoryConfig.cxxtest | 38 +++++++++++++++++++++++++++++++- src/openlcb/MemoryConfig.hxx | 17 +++++++++++++- 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/src/openlcb/MemoryConfig.cxxtest b/src/openlcb/MemoryConfig.cxxtest index 839a3115c..0af768cc8 100644 --- a/src/openlcb/MemoryConfig.cxxtest +++ b/src/openlcb/MemoryConfig.cxxtest @@ -412,6 +412,42 @@ TEST_F(MemoryConfigTest, Reboot) wait(); } +TEST_F(MemoryConfigTest, FactoryResetWrongNodeId) +{ + StrictMock mock; + ConfigUpdateFlow update_flow{ifCan_.get()}; + update_flow.TEST_set_fd(23); + + FactoryResetListener l; + // rejected with error "invalid arguments" + expect_packet(":X19A4822AN077C1080;"); + + EXPECT_CALL(mock, factory_reset()).Times(0); + send_packet(":X1A22A77CN20AA010101010101;"); + wait(); + + EXPECT_CALL(mock, reboot()).Times(0); + twait(); +} + +TEST_F(MemoryConfigTest, FactoryResetNoNodeId) +{ + StrictMock mock; + ConfigUpdateFlow update_flow{ifCan_.get()}; + update_flow.TEST_set_fd(23); + + FactoryResetListener l; + // rejected with error "invalid arguments" + expect_packet(":X19A4822AN077C1080;"); + + EXPECT_CALL(mock, factory_reset()).Times(0); + send_packet(":X1A22A77CN20AA;"); + wait(); + + EXPECT_CALL(mock, reboot()).Times(0); + twait(); +} + TEST_F(MemoryConfigTest, FactoryReset) { StrictMock mock; @@ -422,7 +458,7 @@ TEST_F(MemoryConfigTest, FactoryReset) expect_packet(":X19A2822AN077C00;"); // received OK, no response EXPECT_CALL(mock, factory_reset()); - send_packet(":X1A22A77CN20AA;"); + send_packet(":X1A22A77CN20AA02010d000003;"); wait(); EXPECT_CALL(mock, reboot()); diff --git a/src/openlcb/MemoryConfig.hxx b/src/openlcb/MemoryConfig.hxx index 2f84f30a8..bb6eb237a 100644 --- a/src/openlcb/MemoryConfig.hxx +++ b/src/openlcb/MemoryConfig.hxx @@ -485,6 +485,10 @@ private: case MemoryConfigDefs::COMMAND_FACTORY_RESET: { NodeID id = message()->data()->dst->node_id(); + if (len < 8 || (data_to_node_id(&bytes[2]) != id)) + { + return respond_reject(Defs::ERROR_INVALID_ARGS); + } uint16_t ret = handle_factory_reset(id); if (!ret) { @@ -544,7 +548,18 @@ private: /// Invokes the openlcb config handler to do a factory reset. Starts a /// timer to reboot the device after a little time. - void handle_factory_reset(); + /// @param target the node ID for which factory reset was invoked. + /// @return openlcb error code, 0 on success + uint16_t handle_factory_reset(NodeID target); + + /// Weak definition for invoking a factory reset on virtual nodes. The + /// default implementation does nothing and return an unimplemented + /// error. Applications that use virtual nodes and need to support factory + /// reset should reimplement this function. + /// @param target the node ID for which factory reset was invoked. + /// @return openlcb error code, 0 on success + uint16_t __attribute__((noinline)) app_handle_factory_reset(NodeID target); + Action ok_response_sent() OVERRIDE { From 30dc04fa87903e82448bae84c780d8157cb88299 Mon Sep 17 00:00:00 2001 From: Balazs Racz Date: Sun, 25 Sep 2022 11:44:00 +0200 Subject: [PATCH 4/7] Adds unit test for factory reset of a virtual node. --- src/openlcb/MemoryConfig.cxxtest | 35 +++++++++++++++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/src/openlcb/MemoryConfig.cxxtest b/src/openlcb/MemoryConfig.cxxtest index 0af768cc8..8f18f704d 100644 --- a/src/openlcb/MemoryConfig.cxxtest +++ b/src/openlcb/MemoryConfig.cxxtest @@ -64,7 +64,7 @@ public: class MemoryConfigTest : public TwoNodeDatagramTest { protected: - MemoryConfigTest() : memoryOne_(&datagram_support_, node_, 10) + MemoryConfigTest() : memoryOne_(&datagram_support_, nullptr, 10) { ON_CALL(space, write(_, _, _, _, _)) .WillByDefault(DoAll(SetArgPointee<3>(0x2F00), Return(0))); @@ -378,6 +378,7 @@ TEST_F(MemoryConfigTest, GetSpaceInfoRO_NZLA) struct GlobalMock : public Singleton { MOCK_METHOD0(reboot, void()); MOCK_METHOD0(factory_reset, void()); + MOCK_METHOD1(custom_factory_reset, void(NodeID)); }; extern "C" void reboot() @@ -385,6 +386,13 @@ extern "C" void reboot() GlobalMock::instance()->reboot(); } +// Overrides the weak implementation in the stack. +uint16_t MemoryConfigHandler::app_handle_factory_reset(NodeID id) +{ + GlobalMock::instance()->custom_factory_reset(id); + return 0x1234; +} + struct FactoryResetListener : public DefaultConfigUpdateListener { void factory_reset(int fd) override @@ -465,6 +473,31 @@ TEST_F(MemoryConfigTest, FactoryReset) twait(); } +// Tests addressing a factory reset command to a virtual node (non-default +// node). +TEST_F(MemoryConfigTest, FactoryResetVNode) +{ + setup_other_node(false); + StrictMock mock; + ConfigUpdateFlow update_flow{ifCan_.get()}; + update_flow.TEST_set_fd(23); + + FactoryResetListener l; + + expect_packet(":X19A48225N077C1234;"); // Rejected with error 0x1234 + + // Default reset won't be invoked. + EXPECT_CALL(mock, factory_reset()).Times(0); + // Custom reset will. + EXPECT_CALL(mock, custom_factory_reset(OTHER_NODE_ID)); + send_packet(":X1A22577CN20AA02010d000103;"); + wait(); + + // No reboot. + EXPECT_CALL(mock, reboot()).Times(0); + twait(); +} + static const char MEMORY_BLOCK_DATA[] = "abrakadabra12345678xxxxyyyyzzzzwww."; class StaticBlockTest : public MemoryConfigTest From 89d1f3d5f1eac0833eefbed6cb8c8d25b1395782 Mon Sep 17 00:00:00 2001 From: Balazs Racz Date: Sun, 25 Sep 2022 11:58:54 +0200 Subject: [PATCH 5/7] Adds support to MemoryConfigClient for sending factory reset command. This is different from generic metadata command because there might be no currently known remote node ID. Therefore the system uses a node ID lookup flow as well. --- src/openlcb/MemoryConfigClient.cxxtest | 125 ++++++++++++++++++++++++- src/openlcb/MemoryConfigClient.hxx | 75 ++++++++++++++- 2 files changed, 197 insertions(+), 3 deletions(-) diff --git a/src/openlcb/MemoryConfigClient.cxxtest b/src/openlcb/MemoryConfigClient.cxxtest index c9e7f64a7..0d16d200e 100644 --- a/src/openlcb/MemoryConfigClient.cxxtest +++ b/src/openlcb/MemoryConfigClient.cxxtest @@ -33,7 +33,10 @@ */ #include "openlcb/MemoryConfigClient.hxx" + +#include "openlcb/ConfigUpdateFlow.hxx" #include "openlcb/DatagramCan.hxx" +#include "utils/ConfigUpdateListener.hxx" #include "utils/async_datagram_test_helper.hxx" @@ -98,6 +101,31 @@ public: SyncNotifiable n_; }; +struct GlobalMock : public Singleton { + MOCK_METHOD0(reboot, void()); + MOCK_METHOD0(factory_reset, void()); +}; + +extern "C" void reboot() +{ + GlobalMock::instance()->reboot(); +} + +struct FactoryResetListener : public DefaultConfigUpdateListener +{ + void factory_reset(int fd) override + { + GlobalMock::instance()->factory_reset(); + } + + UpdateAction apply_configuration( + int fd, bool initial_load, BarrierNotifiable *done) + { + done->notify(); + return UPDATED; + } +}; + TEST_F(MemoryConfigClientTest, create) { } @@ -216,7 +244,7 @@ TEST_F(MemoryConfigClientTest, reboot_reboot) EXPECT_EQ(MemoryConfigClient::OPERATION_PENDING, b->data()->resultCode); send_packet(":X19100499N050101011400;"); wait(); // we purposefully do not do timed wait here; the init complete - // message sould terminate the timeout timer. + // message should terminate the timeout timer. EXPECT_EQ(0, b->data()->resultCode); ASSERT_TRUE(b->data()->done.is_done()); } @@ -260,6 +288,101 @@ TEST_F(MemoryConfigClientTest, unfreeze) ASSERT_TRUE(b->data()->done.is_done()); } +class FactoryResetTest : public MemoryConfigClientTest { +protected: + FactoryResetTest() + { + updateFlow_.TEST_set_fd(23); + eb_.release_block(); + twait(); + } + + ~FactoryResetTest() { + twait(); + } + + StrictMock mock_; + BlockExecutor eb_{ifCan_->executor()}; + ConfigUpdateFlow updateFlow_{ifCan_.get()}; + FactoryResetListener l_; +}; // FactoryResetTest + +TEST_F(FactoryResetTest, create) {} + +// Sends a factory reset command to a target represented with a node ID. +TEST_F(FactoryResetTest, send_with_id) +{ + print_all_packets(); + expect_any_packet(); + ::testing::InSequence seq; + EXPECT_CALL(mock_, factory_reset()); + EXPECT_CALL(mock_, reboot()); + auto b = invoke_flow(&clientTwo_, MemoryConfigClientRequest::FACTORY_RESET, + NodeHandle(TEST_NODE_ID)); + EXPECT_EQ(0, b->data()->resultCode); +} + +// Sends a factory reset command to a target represented with a node alias +// only, no ID. +TEST_F(FactoryResetTest, send_with_alias) +{ + print_all_packets(); + expect_any_packet(); + ::testing::InSequence seq; + EXPECT_CALL(mock_, factory_reset()); + EXPECT_CALL(mock_, reboot()); + auto b = invoke_flow(&clientTwo_, MemoryConfigClientRequest::FACTORY_RESET, + NodeHandle(NodeAlias(0x22A))); + EXPECT_EQ(0, b->data()->resultCode); +} + +// Sends a factory reset command to a the local node. +TEST_F(FactoryResetTest, do_local) +{ + print_all_packets(); + expect_any_packet(); + ::testing::InSequence seq; + EXPECT_CALL(mock_, factory_reset()); + EXPECT_CALL(mock_, reboot()); + auto b = invoke_flow(&clientTwo_, MemoryConfigClientRequest::FACTORY_RESET, + NodeHandle(TWO_NODE_ID)); + EXPECT_EQ(0, b->data()->resultCode); +} + +// Sends a factory reset command to a remote node by alias, with expectations +// on each packet. +TEST_F(FactoryResetTest, send_alias_lookup) +{ + print_all_packets(); + ::testing::InSequence seq; + expect_packet(":X19488FF2N0499;"); // looking for dstThree_ node + auto b = invoke_client_no_block( + MemoryConfigClientRequest::FACTORY_RESET, dstThree_); + wait(); + clear_expect(true); + EXPECT_EQ(MemoryConfigClient::OPERATION_PENDING, b->data()->resultCode); + + send_packet_and_expect_response( // + ":X19170499N010203040506;", // + ":X1A499FF2N20AA010203040506;"); + clear_expect(true); + EXPECT_EQ(MemoryConfigClient::OPERATION_PENDING, b->data()->resultCode); + + // Reject with an error. + send_packet(":X19A48499N0FF21234;"); + twait(); + + EXPECT_EQ(0x1234, b->data()->resultCode); + + EXPECT_CALL(mock_, factory_reset()).Times(0); + EXPECT_CALL(mock_, reboot()).Times(0); +} + + +TEST_F(MemoryConfigClientTest, factory_reset) +{ +} + class MemoryConfigLocalClientTest : public AsyncDatagramTest { protected: ~MemoryConfigLocalClientTest() { diff --git a/src/openlcb/MemoryConfigClient.hxx b/src/openlcb/MemoryConfigClient.hxx index 965400d3d..487f56db9 100644 --- a/src/openlcb/MemoryConfigClient.hxx +++ b/src/openlcb/MemoryConfigClient.hxx @@ -36,8 +36,9 @@ #define _OPENLCB_MEMORYCONFIGCLIENT_HXX_ #include "executor/CallableFlow.hxx" -#include "openlcb/MemoryConfig.hxx" #include "openlcb/DatagramHandlerDefault.hxx" +#include "openlcb/IfCan.hxx" +#include "openlcb/MemoryConfig.hxx" namespace openlcb { @@ -69,6 +70,11 @@ struct MemoryConfigClientRequest : public CallableFlowRequestBase REBOOT }; + enum FactoryResetCmd + { + FACTORY_RESET + }; + enum FreezeCmd { FREEZE @@ -164,6 +170,21 @@ struct MemoryConfigClientRequest : public CallableFlowRequestBase payload.push_back(MemoryConfigDefs::COMMAND_RESET); } + /// Sets up a command to send a Factory Reset request to a remote node. + /// @param FactoryResetCmd polymorphic matching arg; always set to + /// FACTORY_RESET. + /// @param d is the destination node + void reset(FactoryResetCmd, NodeHandle d) + { + reset_base(); + cmd = CMD_FACTORY_RESET; + dst = d; + payload.clear(); + payload.reserve(8); + payload.push_back(DatagramDefs::CONFIGURATION); + payload.push_back(MemoryConfigDefs::COMMAND_FACTORY_RESET); + } + /// Sets up a command to send a Freeze request to a remote node. /// @param FreezeCmd polymorphic matching arg; always set to /// FREEZE. @@ -203,7 +224,8 @@ struct MemoryConfigClientRequest : public CallableFlowRequestBase CMD_READ, CMD_READ_PART, CMD_WRITE, - CMD_META_REQUEST + CMD_META_REQUEST, + CMD_FACTORY_RESET }; /// Helper function invoked at every other reset call. @@ -277,6 +299,8 @@ private: case MemoryConfigClientRequest::CMD_META_REQUEST: return allocate_and_call( STATE(do_meta_request), dg_service()->client_allocator()); + case MemoryConfigClientRequest::CMD_FACTORY_RESET: + return call_immediately(STATE(prepare_factory_reset)); default: break; } @@ -580,6 +604,51 @@ private: } } + /// Before we send out a factory reset command, we have to ensure that we + /// know the target node's node ID, not just the alias. + Action prepare_factory_reset() + { + node()->iface()->canonicalize_handle(&request()->dst); + if (request()->dst.id) + { + return call_immediately(STATE(factory_reset_have_id)); + } + // Now: we have a dst with alias only, so we must be running on CAN-bus. + IfCan *iface = (IfCan *)node()->iface(); + if (!nodeIdlookupFlow_) + { + // This is so rarely used that we rather allocate it dynamically. + nodeIdlookupFlow_.reset(new NodeIdLookupFlow(iface)); + } + return invoke_subflow_and_wait(nodeIdlookupFlow_.get(), + STATE(dst_id_complete), node(), request()->dst); + } + + /// Completed the ID lookup flow. + Action dst_id_complete() + { + auto rb = + get_buffer_deleter(full_allocation_result(nodeIdlookupFlow_.get())); + request()->dst = rb->data()->handle; + // Object not needed anymore. + nodeIdlookupFlow_.reset(); + if (request()->dst.id) + { + return call_immediately(STATE(factory_reset_have_id)); + } + return return_with_error(Defs::ERROR_OPENMRN_NOT_FOUND); + } + + /// Called from different places to do the factory reset request once we + /// have the node ID filled in the dst handle. + Action factory_reset_have_id() + { + request()->payload.resize(8); + node_id_to_data(request()->dst.id, &request()->payload[2]); + return allocate_and_call( + STATE(do_meta_request), dg_service()->client_allocator()); + } + class ResponseFlow : public DefaultDatagramHandler { public: @@ -661,6 +730,8 @@ private: ResponseFlow responseFlow_{this}; /// Notify helper. BarrierNotifiable bn_; + /// Rarely used helper flow to look up full node IDs from aliases. + std::unique_ptr nodeIdlookupFlow_; /// Next byte to read from the memory space. uint32_t offset_; /// Next byte in the payload to write. From 23a425b6810dc50a28394def679a5fd41672e66b Mon Sep 17 00:00:00 2001 From: Balazs Racz Date: Sun, 25 Sep 2022 12:08:49 +0200 Subject: [PATCH 6/7] Adds unit tests covering the special cases in the memory config client for sending factory reset datagrams. --- src/openlcb/MemoryConfigClient.cxxtest | 27 ++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/src/openlcb/MemoryConfigClient.cxxtest b/src/openlcb/MemoryConfigClient.cxxtest index 0d16d200e..8c3817a38 100644 --- a/src/openlcb/MemoryConfigClient.cxxtest +++ b/src/openlcb/MemoryConfigClient.cxxtest @@ -378,6 +378,33 @@ TEST_F(FactoryResetTest, send_alias_lookup) EXPECT_CALL(mock_, reboot()).Times(0); } +// Sends a factory reset command to a remote node by alias, when there is +// already a cache entry for that remote alias. +TEST_F(FactoryResetTest, cached_alias) +{ + print_all_packets(); + ::testing::InSequence seq; + // AMD frame to prefill cache. + send_packet(":X10701499N010203040506;"); + wait(); + // Datagram sent straight away + expect_packet(":X1A499FF2N20AA010203040506;"); + auto b = invoke_client_no_block( + MemoryConfigClientRequest::FACTORY_RESET, dstThree_); + wait(); + clear_expect(true); + EXPECT_EQ(MemoryConfigClient::OPERATION_PENDING, b->data()->resultCode); + + // Accept with no response pending + send_packet(":X19A28499N0FF200;"); + twait(); + + EXPECT_EQ(0, b->data()->resultCode); + + EXPECT_CALL(mock_, factory_reset()).Times(0); + EXPECT_CALL(mock_, reboot()).Times(0); +} + TEST_F(MemoryConfigClientTest, factory_reset) { From 1446d15775106aec77375ebb406293d663a901a5 Mon Sep 17 00:00:00 2001 From: Balazs Racz Date: Sun, 25 Sep 2022 13:36:14 +0200 Subject: [PATCH 7/7] fix whitespace --- src/openlcb/If.hxx | 2 +- src/openlcb/IfTcp.cxxtest | 2 +- src/openlcb/IfTcp.hxx | 2 +- src/openlcb/MemoryConfig.cxxtest | 15 ++++++++------- src/openlcb/MemoryConfig.hxx | 1 - src/openlcb/MemoryConfigClient.cxxtest | 22 +++++++++++++--------- src/openlcb/MemoryConfigClient.hxx | 4 ++-- 7 files changed, 26 insertions(+), 22 deletions(-) diff --git a/src/openlcb/If.hxx b/src/openlcb/If.hxx index 80cd7a02a..95364c8d0 100644 --- a/src/openlcb/If.hxx +++ b/src/openlcb/If.hxx @@ -329,7 +329,7 @@ public: /// interfaces this is the gateway node ID, for CAN interfaces this is the /// node ID used for alias allocation on the CAN-bus. virtual NodeID get_default_node_id() = 0; - + /// Sets a transmit hook. This function will be called once for every /// OpenLCB message transmitted. Used for implementing activity LEDs. /// @param hook function to call for each transmit message. diff --git a/src/openlcb/IfTcp.cxxtest b/src/openlcb/IfTcp.cxxtest index a33dbeb52..cb70dd472 100644 --- a/src/openlcb/IfTcp.cxxtest +++ b/src/openlcb/IfTcp.cxxtest @@ -151,7 +151,7 @@ protected: HubPortInterface *fakeSource_ = (HubPortInterface *)123456; TestSequenceGenerator seq_; static constexpr uint64_t GW_NODE_ID = 0x101112131415ULL; - LocalIf localIf_{10, GW_NODE_ID}; + LocalIf localIf_ {10, GW_NODE_ID}; TcpSendFlow sendFlow_{ &localIf_, GW_NODE_ID, &fakeSendTarget_, fakeSource_, &seq_}; }; diff --git a/src/openlcb/IfTcp.hxx b/src/openlcb/IfTcp.hxx index 9f063df28..3bdb3f5f9 100644 --- a/src/openlcb/IfTcp.hxx +++ b/src/openlcb/IfTcp.hxx @@ -115,7 +115,7 @@ public: /// @return the gateway node ID. NodeID get_default_node_id() override; - + private: /// Where to send traffic to. HubFlow *device_; diff --git a/src/openlcb/MemoryConfig.cxxtest b/src/openlcb/MemoryConfig.cxxtest index 8f18f704d..b23251a0b 100644 --- a/src/openlcb/MemoryConfig.cxxtest +++ b/src/openlcb/MemoryConfig.cxxtest @@ -64,7 +64,8 @@ public: class MemoryConfigTest : public TwoNodeDatagramTest { protected: - MemoryConfigTest() : memoryOne_(&datagram_support_, nullptr, 10) + MemoryConfigTest() + : memoryOne_(&datagram_support_, nullptr, 10) { ON_CALL(space, write(_, _, _, _, _)) .WillByDefault(DoAll(SetArgPointee<3>(0x2F00), Return(0))); @@ -423,9 +424,9 @@ TEST_F(MemoryConfigTest, Reboot) TEST_F(MemoryConfigTest, FactoryResetWrongNodeId) { StrictMock mock; - ConfigUpdateFlow update_flow{ifCan_.get()}; + ConfigUpdateFlow update_flow {ifCan_.get()}; update_flow.TEST_set_fd(23); - + FactoryResetListener l; // rejected with error "invalid arguments" expect_packet(":X19A4822AN077C1080;"); @@ -441,9 +442,9 @@ TEST_F(MemoryConfigTest, FactoryResetWrongNodeId) TEST_F(MemoryConfigTest, FactoryResetNoNodeId) { StrictMock mock; - ConfigUpdateFlow update_flow{ifCan_.get()}; + ConfigUpdateFlow update_flow {ifCan_.get()}; update_flow.TEST_set_fd(23); - + FactoryResetListener l; // rejected with error "invalid arguments" expect_packet(":X19A4822AN077C1080;"); @@ -479,9 +480,9 @@ TEST_F(MemoryConfigTest, FactoryResetVNode) { setup_other_node(false); StrictMock mock; - ConfigUpdateFlow update_flow{ifCan_.get()}; + ConfigUpdateFlow update_flow {ifCan_.get()}; update_flow.TEST_set_fd(23); - + FactoryResetListener l; expect_packet(":X19A48225N077C1234;"); // Rejected with error 0x1234 diff --git a/src/openlcb/MemoryConfig.hxx b/src/openlcb/MemoryConfig.hxx index bb6eb237a..a6d049b22 100644 --- a/src/openlcb/MemoryConfig.hxx +++ b/src/openlcb/MemoryConfig.hxx @@ -560,7 +560,6 @@ private: /// @return openlcb error code, 0 on success uint16_t __attribute__((noinline)) app_handle_factory_reset(NodeID target); - Action ok_response_sent() OVERRIDE { if (!response_.empty()) diff --git a/src/openlcb/MemoryConfigClient.cxxtest b/src/openlcb/MemoryConfigClient.cxxtest index 8c3817a38..05afc63f0 100644 --- a/src/openlcb/MemoryConfigClient.cxxtest +++ b/src/openlcb/MemoryConfigClient.cxxtest @@ -101,7 +101,8 @@ public: SyncNotifiable n_; }; -struct GlobalMock : public Singleton { +struct GlobalMock : public Singleton +{ MOCK_METHOD0(reboot, void()); MOCK_METHOD0(factory_reset, void()); }; @@ -288,7 +289,8 @@ TEST_F(MemoryConfigClientTest, unfreeze) ASSERT_TRUE(b->data()->done.is_done()); } -class FactoryResetTest : public MemoryConfigClientTest { +class FactoryResetTest : public MemoryConfigClientTest +{ protected: FactoryResetTest() { @@ -297,17 +299,20 @@ protected: twait(); } - ~FactoryResetTest() { + ~FactoryResetTest() + { twait(); } - + StrictMock mock_; - BlockExecutor eb_{ifCan_->executor()}; - ConfigUpdateFlow updateFlow_{ifCan_.get()}; + BlockExecutor eb_ {ifCan_->executor()}; + ConfigUpdateFlow updateFlow_ {ifCan_.get()}; FactoryResetListener l_; }; // FactoryResetTest -TEST_F(FactoryResetTest, create) {} +TEST_F(FactoryResetTest, create) +{ +} // Sends a factory reset command to a target represented with a node ID. TEST_F(FactoryResetTest, send_with_id) @@ -332,7 +337,7 @@ TEST_F(FactoryResetTest, send_with_alias) EXPECT_CALL(mock_, factory_reset()); EXPECT_CALL(mock_, reboot()); auto b = invoke_flow(&clientTwo_, MemoryConfigClientRequest::FACTORY_RESET, - NodeHandle(NodeAlias(0x22A))); + NodeHandle(NodeAlias(0x22A))); EXPECT_EQ(0, b->data()->resultCode); } @@ -405,7 +410,6 @@ TEST_F(FactoryResetTest, cached_alias) EXPECT_CALL(mock_, reboot()).Times(0); } - TEST_F(MemoryConfigClientTest, factory_reset) { } diff --git a/src/openlcb/MemoryConfigClient.hxx b/src/openlcb/MemoryConfigClient.hxx index 487f56db9..1ce044017 100644 --- a/src/openlcb/MemoryConfigClient.hxx +++ b/src/openlcb/MemoryConfigClient.hxx @@ -74,7 +74,7 @@ struct MemoryConfigClientRequest : public CallableFlowRequestBase { FACTORY_RESET }; - + enum FreezeCmd { FREEZE @@ -184,7 +184,7 @@ struct MemoryConfigClientRequest : public CallableFlowRequestBase payload.push_back(DatagramDefs::CONFIGURATION); payload.push_back(MemoryConfigDefs::COMMAND_FACTORY_RESET); } - + /// Sets up a command to send a Freeze request to a remote node. /// @param FreezeCmd polymorphic matching arg; always set to /// FREEZE.