Address PR review feedback from liamcottle (second round)

- Rename eth command to eth.status for consistency with other commands - Rename generateDeviceMac to generateEthernetMac for clarity - Refactor ethernet_handle_command to return false by default - Allow new TCP clients to replace existing connections (repeater, room server, SerialEthernetInterface) - Boot companion radio without Ethernet on init failure (LoRa-only recovery mode) - Remove > prompt from ethernet CLI for consistency with serial interface - Fix variable redeclaration compile error in SerialEthernetInterface when ETHERNET_STATIC_IP is defined - Fix TCP socket leak when duplicate client detection fires - Remove dead recv_queue and adv_restart_time members from SerialEthernetInterface - Fix port numbers in docs (port 23 for repeater/room server CLI, port 5000 for companion radio) - Clarify eth.status command is only available in repeater and room server firmware Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
4 months ago · 61ba79966b
8 changed files with 81 additions and 93 deletions
--- a/docs/cli_commands.md
+++ b/docs/cli_commands.md
@ -885,22 +885,22 @@ region save

 ### Ethernet (when Ethernet support is compiled in)

-Ethernet support is available on RAK4631 boards with a RAK13800 (W5100S) Ethernet module. Use the `_eth` firmware variants (e.g. `RAK_4631_repeater_eth`) to enable this feature.
+Ethernet support is available on RAK4631 boards with a RAK13800 (W5100S) Ethernet module. Use the `_ethernet` firmware variants (e.g. `RAK_4631_repeater_ethernet`) to enable this feature.

 ---

 #### View Ethernet connection status
 **Usage:**
- `eth`
+- `eth.status`

 **Output:**
- `ETH: <ip>:<port>` when connected (e.g. `ETH: 192.168.1.50:5000`)
+- `ETH: <ip>:<port>` when connected (e.g. `ETH: 192.168.1.50:23`)
 - `ETH: not connected` when Ethernet is not active

 **Notes:**
+- Available on repeater and room server firmware only. Companion radio ethernet firmware does not expose a CLI.
 - The Ethernet interface obtains an IP address via DHCP automatically on boot.
- A TCP server listens on port 5000 (default) for CLI connections.
- For repeaters and room servers, connect with any TCP client (e.g. `nc`, PuTTY) to access the same CLI available over serial.
- For companion radio firmware, the Ethernet interface replaces BLE/USB as the transport to companion apps.
+- A TCP server listens on port 23 (default) for CLI connections.
+- Connect with any TCP client (e.g. `nc`, PuTTY) to access the same CLI available over serial.

 ---
--- a/docs/faq.md
+++ b/docs/faq.md
@ -892,14 +892,14 @@ MeshCore supports Ethernet on RAK4631 boards using the [RAK13800](https://docs.r

 **Firmware:**
 Flash one of the Ethernet-enabled firmware variants:
- `RAK_4631_repeater_eth` - Repeater with Ethernet CLI access
- `RAK_4631_room_server_eth` - Room server with Ethernet CLI access
- `RAK_4631_companion_radio_eth` - Companion radio over Ethernet (replaces BLE)
+- `RAK_4631_repeater_ethernet` - Repeater with Ethernet CLI access
+- `RAK_4631_room_server_ethernet` - Room server with Ethernet CLI access
+- `RAK_4631_companion_radio_ethernet` - Companion radio over Ethernet (replaces BLE)

 **Connecting:**
 - The device obtains an IP address via DHCP automatically on boot.
- For repeaters and room servers, connect to the device on TCP port 5000 using any TCP client (e.g. `nc <ip> 5000` or PuTTY in raw mode). This gives you the same CLI available over serial/USB.
- For companion radio firmware, the Ethernet interface replaces BLE as the transport to companion apps.
- Use the `eth` CLI command to check connection status and see the assigned IP address.
+- For repeaters and room servers, connect to the device on TCP port 23 using any TCP client (e.g. `nc <ip> 23` or PuTTY in raw mode). This gives you the same CLI available over serial/USB.
+- For companion radio firmware, the Ethernet interface replaces BLE as the transport to companion apps. Connect on TCP port 5000 (same as the WiFi companion radio).
+- Use the `eth.status` CLI command to check connection status and see the assigned IP address.

 ---
--- a/examples/companion_radio/main.cpp
+++ b/examples/companion_radio/main.cpp
@ -160,6 +160,7 @@ void setup() {

 #ifdef BLE_PIN_CODE
  serial_interface.begin(BLE_NAME_PREFIX, the_mesh.getNodePrefs()->node_name, the_mesh.getBLEPin());
+  the_mesh.startInterface(serial_interface);
 #elif defined(ETHERNET_ENABLED)
  Serial.print("Waiting for serial to connect...\n");
  time_t timeout = millis();
@ -167,14 +168,15 @@ void setup() {
    if ((millis() - timeout) < 5000) { delay(100); } else { break; }
  }
  Serial.println("Initializing Ethernet adapter...");
-  if (!serial_interface.begin()) {
-    Serial.println("ETH: Init failed, halting");
-    halt();
+  if (serial_interface.begin()) {
+    the_mesh.startInterface(serial_interface);
+  } else {
+    Serial.println("ETH: Init failed, continuing without Ethernet (mesh only)");
  }
 #else
  serial_interface.begin(Serial);
-#endif
  the_mesh.startInterface(serial_interface);
+#endif
 #elif defined(RP2040_PLATFORM)
  LittleFS.begin();
  store.begin();
--- a/examples/simple_repeater/main.cpp
+++ b/examples/simple_repeater/main.cpp
@ -51,7 +51,7 @@
    Ethernet.init(ETHERNET_SPI_PORT, PIN_ETHERNET_SS);

    uint8_t mac[6];
-    generateDeviceMac(mac);
+    generateEthernetMac(mac);
    Serial.printf("ETH: MAC: %02X:%02X:%02X:%02X:%02X:%02X\n",
        mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);

@ -92,20 +92,20 @@

  // Format ethernet status into reply buffer. Returns true if command was handled.
  static bool ethernet_handle_command(const char* command, char* reply) {
-    if (strcmp(command, "eth") != 0) return false;
-    if (!ethernet_running) {
-      strcpy(reply, "ETH: not connected");
-    } else {
-      IPAddress ip = Ethernet.localIP();
-      sprintf(reply, "ETH: %u.%u.%u.%u:%d", ip[0], ip[1], ip[2], ip[3], ETHERNET_TCP_PORT);
+    if (strcmp(command, "eth.status") == 0) {
+      if (!ethernet_running) {
+        strcpy(reply, "ETH: not connected");
+      } else {
+        IPAddress ip = Ethernet.localIP();
+        sprintf(reply, "ETH: %u.%u.%u.%u:%d", ip[0], ip[1], ip[2], ip[3], ETHERNET_TCP_PORT);
+      }
+      return true;
    }
-    return true;
+    return false;
  }

-  // Check for new TCP client connections
+  // Check for new TCP client connections, replacing any existing connection
  static void ethernet_check_client() {
-    if (ethernet_client && ethernet_client.connected()) return;
-
    auto newClient = ethernet_server.available();
    if (newClient) {
      if (ethernet_client) ethernet_client.stop();
@ -113,7 +113,6 @@
      IPAddress ip = ethernet_client.remoteIP();
      Serial.printf("ETH: Client connected from %u.%u.%u.%u\n", ip[0], ip[1], ip[2], ip[3]);
      ethernet_client.println("MeshCore Repeater CLI");
-      ethernet_client.print("> ");
    }
  }
 #endif
@ -285,7 +284,6 @@ void loop() {
      if (reply[0]) {
        ethernet_client.print("  -> "); ethernet_client.println(reply);
      }
-      ethernet_client.print("> ");
      ethernet_command[0] = 0;
    }
  }
--- a/examples/simple_room_server/main.cpp
+++ b/examples/simple_room_server/main.cpp
@ -44,7 +44,7 @@
    Ethernet.init(ETHERNET_SPI_PORT, PIN_ETHERNET_SS);

    uint8_t mac[6];
-    generateDeviceMac(mac);
+    generateEthernetMac(mac);
    Serial.printf("ETH: MAC: %02X:%02X:%02X:%02X:%02X:%02X\n",
        mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);

@ -80,18 +80,20 @@
  }

  static bool ethernet_handle_command(const char* command, char* reply) {
-    if (strcmp(command, "eth") != 0) return false;
-    if (!ethernet_running) {
-      strcpy(reply, "ETH: not connected");
-    } else {
-      IPAddress ip = Ethernet.localIP();
-      sprintf(reply, "ETH: %u.%u.%u.%u:%d", ip[0], ip[1], ip[2], ip[3], ETHERNET_TCP_PORT);
+    if (strcmp(command, "eth.status") == 0) {
+      if (!ethernet_running) {
+        strcpy(reply, "ETH: not connected");
+      } else {
+        IPAddress ip = Ethernet.localIP();
+        sprintf(reply, "ETH: %u.%u.%u.%u:%d", ip[0], ip[1], ip[2], ip[3], ETHERNET_TCP_PORT);
+      }
+      return true;
    }
-    return true;
+    return false;
  }

+  // Check for new TCP client connections, replacing any existing connection
  static void ethernet_check_client() {
-    if (ethernet_client && ethernet_client.connected()) return;
    auto newClient = ethernet_server.available();
    if (newClient) {
      if (ethernet_client) ethernet_client.stop();
@ -99,7 +101,6 @@
      IPAddress ip = ethernet_client.remoteIP();
      Serial.printf("ETH: Client connected from %u.%u.%u.%u\n", ip[0], ip[1], ip[2], ip[3]);
      ethernet_client.println("MeshCore Room Server CLI");
-      ethernet_client.print("> ");
    }
  }
 #endif
@ -255,7 +256,6 @@ void loop() {
      if (reply[0]) {
        ethernet_client.print("  -> "); ethernet_client.println(reply);
      }
-      ethernet_client.print("> ");
      ethernet_command[0] = 0;
    }
  }
--- a/src/helpers/nrf52/EthernetMac.h
+++ b/src/helpers/nrf52/EthernetMac.h
@ -2,7 +2,7 @@

 #include <Arduino.h>

-static inline void generateDeviceMac(uint8_t mac[6]) {
+static inline void generateEthernetMac(uint8_t mac[6]) {
  uint32_t device_id = NRF_FICR->DEVICEID[0];
  mac[0] = 0x02;
  mac[1] = 0x92;
--- a/src/helpers/nrf52/SerialEthernetInterface.cpp
+++ b/src/helpers/nrf52/SerialEthernetInterface.cpp
@ -39,7 +39,7 @@ bool SerialEthernetInterface::begin() {
 #endif

  uint8_t mac[6];
-  generateDeviceMac(mac);
+  generateEthernetMac(mac);
  ETHERNET_DEBUG_PRINTLN(
      "Ethernet MAC: %02X:%02X:%02X:%02X:%02X:%02X",
      mac[0],
@ -80,14 +80,9 @@ bool SerialEthernetInterface::begin() {
  }
  #endif
  ETHERNET_DEBUG_PRINTLN("Ethernet begin complete");
-  IPAddress ip = Ethernet.localIP();
-  ETHERNET_DEBUG_PRINT_IP("IP", ip);
-
-  IPAddress subnet = Ethernet.subnetMask();
-  ETHERNET_DEBUG_PRINT_IP("Subnet", subnet);
-
-  IPAddress gateway = Ethernet.gatewayIP();
-  ETHERNET_DEBUG_PRINT_IP("Gateway", gateway);
+  ETHERNET_DEBUG_PRINT_IP("IP", Ethernet.localIP());
+  ETHERNET_DEBUG_PRINT_IP("Subnet", Ethernet.subnetMask());
+  ETHERNET_DEBUG_PRINT_IP("Gateway", Ethernet.gatewayIP());

  server.begin();   // start listening for clients
  ETHERNET_DEBUG_PRINTLN("Ethernet: listening on TCP port: %d", ETHERNET_TCP_PORT);
@ -132,48 +127,45 @@ bool SerialEthernetInterface::isWriteBusy() const {
 }

 size_t SerialEthernetInterface::checkRecvFrame(uint8_t dest[]) {
-  // check if new client connected
-  if (client && client.connected()) {
-    // Avoid polling for new clients while an active connection exists.
-  } else {
-    auto newClient = server.available();
-    if (newClient) {
-      IPAddress new_ip = newClient.remoteIP();
-      uint16_t new_port = newClient.remotePort();
+  // check if new client connected; new connections replace existing ones
+  auto newClient = server.available();
+  if (newClient) {
+    IPAddress new_ip = newClient.remoteIP();
+    uint16_t new_port = newClient.remotePort();
+    ETHERNET_DEBUG_PRINTLN(
+        "New client available %u.%u.%u.%u:%u",
+        new_ip[0],
+        new_ip[1],
+        new_ip[2],
+        new_ip[3],
+        new_port);
+    if (client && client.connected()) {
+      IPAddress cur_ip = client.remoteIP();
+      uint16_t cur_port = client.remotePort();
      ETHERNET_DEBUG_PRINTLN(
-          "New client available %u.%u.%u.%u:%u",
-          new_ip[0],
-          new_ip[1],
-          new_ip[2],
-          new_ip[3],
-          new_port);
-      if (client && client.connected()) {
-        IPAddress cur_ip = client.remoteIP();
-        uint16_t cur_port = client.remotePort();
-        ETHERNET_DEBUG_PRINTLN(
-            "Current client %u.%u.%u.%u:%u",
-            cur_ip[0],
-            cur_ip[1],
-            cur_ip[2],
-            cur_ip[3],
-            cur_port);
-        if (cur_ip == new_ip && cur_port == new_port) {
-          ETHERNET_DEBUG_PRINTLN("Ignoring duplicate client");
-          return 0;
-        }
+          "Current client %u.%u.%u.%u:%u",
+          cur_ip[0],
+          cur_ip[1],
+          cur_ip[2],
+          cur_ip[3],
+          cur_port);
+      if (cur_ip == new_ip && cur_port == new_port) {
+        ETHERNET_DEBUG_PRINTLN("Ignoring duplicate client");
+        newClient.stop();
+        return 0;
      }
+    }

-      deviceConnected = false;
-      if (client) {
-        ETHERNET_DEBUG_PRINTLN("Closing previous client");
-        client.stop();
-      }
-      _state = RECV_STATE_IDLE;
-      _frame_len = 0;
-      _rx_len = 0;
-      client = newClient;
-      ETHERNET_DEBUG_PRINTLN("Switched to new client");
+    deviceConnected = false;
+    if (client) {
+      ETHERNET_DEBUG_PRINTLN("Closing previous client");
+      client.stop();
    }
+    _state = RECV_STATE_IDLE;
+    _frame_len = 0;
+    _rx_len = 0;
+    client = newClient;
+    ETHERNET_DEBUG_PRINTLN("Switched to new client");
  }

  if (client.connected()) {
--- a/src/helpers/nrf52/SerialEthernetInterface.h
+++ b/src/helpers/nrf52/SerialEthernetInterface.h
@ -13,7 +13,6 @@ class SerialEthernetInterface : public BaseSerialInterface {
  bool deviceConnected;
  bool _isEnabled;
  unsigned long _last_write;
-  unsigned long adv_restart_time;
  uint8_t _state;
  uint16_t _frame_len;
  uint16_t _rx_len;
@ -28,13 +27,10 @@ class SerialEthernetInterface : public BaseSerialInterface {
  };

  #define FRAME_QUEUE_SIZE 4
-  int recv_queue_len;
-  Frame recv_queue[FRAME_QUEUE_SIZE];
  int send_queue_len;
  Frame send_queue[FRAME_QUEUE_SIZE];

  void clearBuffers() {
-    recv_queue_len = 0;
    send_queue_len = 0;
    _state = 0;
    _frame_len = 0;
@ -48,7 +44,7 @@ class SerialEthernetInterface : public BaseSerialInterface {
        deviceConnected = false;
        _isEnabled = false;
        _last_write = 0;
-        send_queue_len = recv_queue_len = 0;
+        send_queue_len = 0;
        _state = 0;
        _frame_len = 0;
        _rx_len = 0;