Factory reset improvements (#660)

This PR adds the safety checks on Factory Reset datagrams that are in the standard. Specifically, a factory reset datagram payload is supposed to carry the node ID that is being factory reset. This PR enforces this argument to be present and correct.

Adds provisions for virtual nodes when factory resets are happening. Specifically, a virtual node's factory reset does NOT invoke the stack's config update / factory reset feature. This is important for command stations, because factory resetting a locomotive node is not intended to lose all the command station's settings. A weak function is added as a hook to implement some application-specific factory reset mechanism for virtual nodes, but the default implementation is to reject the datagram.

Adds implementation for factory reset command to MemoryConfigClient. This is needed for user interfaces to send factory reset commands to openlcb targets.

Misc / dependency:
- Adds a function `get_default_node_id()` on the abstract If object. 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.

===

* 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.

* 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.

* 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.

* Adds unit test for factory reset of a virtual node.

* 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.

* Adds unit tests covering the special cases in the memory config client
for sending factory reset datagrams.

* fix whitespace

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.

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

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());

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;
 

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

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();

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_;

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

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 *>(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 *>(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)

src/openlcb/MemoryConfig.cxxtest

@@ -64,7 +64,8 @@ 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,13 +379,21 @@ TEST_F(MemoryConfigTest, GetSpaceInfoRO_NZLA)
 struct GlobalMock : public Singleton<GlobalMock> {
     MOCK_METHOD0(reboot, void());
     MOCK_METHOD0(factory_reset, void());
+    MOCK_METHOD1(custom_factory_reset, void(NodeID));
 };
 
 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
@@ -412,6 +421,42 @@ TEST_F(MemoryConfigTest, Reboot)
     wait();
 }
 
+TEST_F(MemoryConfigTest, FactoryResetWrongNodeId)
+{
+    StrictMock<GlobalMock> 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<GlobalMock> 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<GlobalMock> mock;
@@ -422,13 +467,38 @@ 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());
     twait();
 }
 
+// Tests addressing a factory reset command to a virtual node (non-default
+// node).
+TEST_F(MemoryConfigTest, FactoryResetVNode)
+{
+    setup_other_node(false);
+    StrictMock<GlobalMock> 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

src/openlcb/MemoryConfig.hxx

@@ -484,8 +484,20 @@ private:
             }
             case MemoryConfigDefs::COMMAND_FACTORY_RESET:
             {
-                handle_factory_reset();
-                return respond_ok(0);
+                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)
+                {
+                    return respond_ok(0);
+                }
+                else
+                {
+                    return respond_reject(ret);
+                }
             }
             case MemoryConfigDefs::COMMAND_OPTIONS:
             {
@@ -536,8 +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
     {
         if (!response_.empty())