[all-devices-app] Make the Pigweed RPC server port configurable (project-chip#74139)

* [all-devices-app] Make the Pigweed RPC server port configurable

The POSIX all-devices-app hardcoded the Pigweed RPC listen port to 33000
(with a "TODO: Add an arg for Pw port"), so two instances of the app could
not run concurrently and the in-tree PwRPC test was pinned to that port.

Add a --rpc-server-port option, named to match the equivalent option in
examples/platform/linux so that existing tooling and docs carry over. It
defaults to 33000, so behavior is unchanged when the flag is not passed.
Note this is distinct from --port, which sets the Matter operational port.

The option and the AppConfig field are parsed unconditionally rather than
being guarded by PW_RPC_ENABLED, for two reasons:

  - the app_options unit test target does not define PW_RPC_ENABLED, so a
    guarded option would compile away and could not be tested;
  - all-devices-app defines PW_RPC_ENABLED as 0/1 while
    examples/platform/linux/Options.h tests it with #if defined(), so
    spreading the define to more targets risks changing the visible layout
    of LinuxDeviceOptions between translation units.

Only the consumption of the value in main.cpp is guarded; builds without
Pigweed RPC log a warning if the flag is passed rather than silently
ignoring it.

While here, the duplicated inline port parsing is factored into a shared
AppOptions::ParsePortNumber() used by both --port and --rpc-server-port.
This adds an `endptr == value` check the previous --port code lacked, so
an empty value is now rejected instead of silently parsing as 0.

The boolean-state-sensor PwRPC test no longer hardcodes 127.0.0.1:33000
and accepts "--int-arg rpc_server_port:<port>".

* [all-devices-app] Address review feedback on --rpc-server-port

Two fixes from review:

Reject port 0 for --rpc-server-port. Binding port 0 makes the OS pick an
ephemeral port, and because the pw_rpc port is not advertised anywhere and
the app does not report the port it actually bound, no client could reach
the server. The check is applied at the option site rather than in the
shared ParsePortNumber(), so --port 0 keeps working: the Matter operational
port is advertised over DNS-SD, so an ephemeral port is still resolvable
there.

Track whether --rpc-server-port was actually supplied, by making the
AppConfig field a std::optional<uint16_t> (matching the neighbouring `port`
field). Previously the non-PW_RPC build compared the value against the
default, so "--rpc-server-port 33000" was silently ignored instead of
warning. main.cpp now uses value_or(kDefaultRpcServerPort) when RPC is
enabled and has_value() for the warning when it is not.

* [all-devices-app] Document --rpc-server-port in the boolean-state-sensor test

Wire the new option through both halves of the example invocation in the
test's docstring: --rpc-server-port on the app side and the matching
--int-arg rpc_server_port on the script side, so the two stay in sync.

Addresses review feedback from @sxb427.

Author: soares-sergio

examples/all-devices-app/all-devices-common/device/types/boolean-state-sensor/test.py

@@ -30,13 +30,15 @@
     --app out/linux-x64-all-devices-ipv6only-no-ble-no-wifi-rpc-asan-clang-test/all-devices-app \
     --app-args "\
         --device contact-sensor:1 \
-        --device water-leak-detector:2" \
+        --device water-leak-detector:2 \
+        --rpc-server-port 33000" \
     --factory-reset \
     --script examples/all-devices-app/all-devices-common/device/types/boolean-state-sensor/test.py \
     --script-args "\
         --commissioning-method on-network \
         --discriminator 3840 \
-        --passcode 20202021\
+        --passcode 20202021 \
+        --int-arg rpc_server_port:33000\
         " \
     --app-stdin-pipe /tmp/app_stdin.txt'
 
@@ -103,12 +105,15 @@ async def test_TC_BOOL_1_1(self):
         self.step(3, "Toggle and assert state values on Endpoint 1 and Endpoint 2 independently via PwRPC")
 
         # Establish PwRPC connection
-        logger.info("Establishing Pigweed RPC connection...")
+        # Defaults to the app's own default port; override with
+        # "--int-arg rpc_server_port:<port>" when the app is started with --rpc-server-port.
+        rpc_server_port = self.user_params.get("rpc_server_port", 33000)
+        logger.info("Establishing Pigweed RPC connection on port %d...", rpc_server_port)
         device_connection = create_device_serial_or_socket_connection(
             device="",
             baudrate=115200,
             token_databases=[],
-            socket_addr="127.0.0.1:33000",
+            socket_addr=f"127.0.0.1:{rpc_server_port}",
             compiled_protos=[attributes_service_pb2],
             rpc_logging=True,
             channel_id=rpc.DEFAULT_CHANNEL_ID,

examples/all-devices-app/posix/app_options/AppOptions.cpp

@@ -63,6 +63,7 @@ constexpr uint16_t kOptionEnableKey     = 0xffde;
 #if CHIP_DEVICE_CONFIG_ENABLE_WIFIPAF
 constexpr uint16_t kOptionWiFiPAF = 0xffdf;
 #endif
+constexpr uint16_t kOptionRpcServerPort = 0xffe0;
 
 DeviceTypeParser AppOptions::sParser;
 AppOptions::AppConfig AppOptions::mConfig;
@@ -112,6 +113,27 @@ std::vector<uint16_t> AppOptions::ParseWiFiPafFreqList(const std::string & extCm
 }
 #endif // CHIP_DEVICE_CONFIG_ENABLE_WIFIPAF
 
+bool AppOptions::ParsePortNumber(const char * value, uint16_t & port)
+{
+    if (value == nullptr || *value == '\0')
+    {
+        return false;
+    }
+
+    char * endptr           = nullptr;
+    const unsigned long val = strtoul(value, &endptr, 0);
+
+    // `endptr == value` catches input with no digits at all; strtoul would otherwise
+    // report success with a value of 0.
+    if (endptr == value || *endptr != '\0' || val > UINT16_MAX)
+    {
+        return false;
+    }
+
+    port = static_cast<uint16_t>(val);
+    return true;
+}
+
 const AppOptions::AppConfig & AppOptions::GetConfig()
 {
     VerifyOrDie(sIsConfigValidated);
@@ -196,15 +218,33 @@ bool AppOptions::AllDevicesAppOptionHandler(const char * program, OptionSet * op
         mConfig.productId = static_cast<uint16_t>(strtoul(value, nullptr, 0));
         return true;
     case kOptionPort: {
-        char * endptr;
-        unsigned long val = strtoul(value, &endptr, 0);
-        if (*endptr != '\0' || val > 0xFFFF)
+        uint16_t port = 0;
+        if (!ParsePortNumber(value, port))
         {
             ChipLogError(Support, "Invalid port: %s", value);
             return false;
         }
-        mConfig.port = static_cast<uint16_t>(val);
-        ChipLogProgress(AppServer, "Port option set to %u", static_cast<uint16_t>(val));
+        mConfig.port = port;
+        ChipLogProgress(AppServer, "Port option set to %u", port);
+        return true;
+    }
+    case kOptionRpcServerPort: {
+        uint16_t port = 0;
+        if (!ParsePortNumber(value, port))
+        {
+            ChipLogError(Support, "Invalid RPC server port: %s", value);
+            return false;
+        }
+        // Port 0 would make the OS pick an ephemeral port. Unlike the Matter operational port,
+        // which is advertised over DNS-SD, the pw_rpc port is not discoverable and the app does
+        // not report the port it actually bound, so no client could ever reach the server.
+        if (port == 0)
+        {
+            ChipLogError(Support, "Invalid RPC server port: 0 is not a usable listen port");
+            return false;
+        }
+        mConfig.rpcServerPort = port;
+        ChipLogProgress(AppServer, "RPC server port option set to %u", port);
         return true;
     }
     case kOptionInterfaceId:
@@ -273,6 +313,7 @@ OptionSet * AppOptions::GetOptions()
         { "trace-to", kArgumentRequired, kOptionTraceTo },
         { "dac_provider", kArgumentRequired, kOptionDacProvider },
         { "enable-key", kArgumentRequired, kOptionEnableKey },
+        { "rpc-server-port", kArgumentRequired, kOptionRpcServerPort },
 #if CHIP_DEVICE_CONFIG_ENABLE_WIFIPAF
         { "wifipaf", kArgumentRequired, kOptionWiFiPAF },
 #endif
@@ -344,6 +385,11 @@ OptionSet * AppOptions::GetOptions()
         result += "  --enable-key <key>\n";
         result += "       A 16-byte, hex-encoded key, used to validate TestEventTrigger command of General Diagnostics cluster\n\n";
 
+        result += "  --rpc-server-port <number>\n";
+        result += "       Listen port for the Pigweed RPC server, 1-65535 (default: 33000). This is\n";
+        result += "       separate from --port, which sets the Matter operational port. Only has an\n";
+        result += "       effect in builds compiled with Pigweed RPC support (chip_enable_pw_rpc).\n\n";
+
 #if CHIP_DEVICE_CONFIG_ENABLE_WIFIPAF
         result += "  --wifipaf freq_list=<freq_1>,<freq_2>...\n";
         result += "       Enable Wi-Fi PAF via wpa_supplicant, on these NAN frequencies in MHz.\n";

examples/all-devices-app/posix/app_options/AppOptions.h

@@ -30,6 +30,11 @@
 class AppOptions
 {
 public:
+    /// Default listen port for the Pigweed RPC server. Matches the default used by the
+    /// other Linux examples (see LinuxDeviceOptions::rpcServerPort) so that existing
+    /// tooling keeps working when no port is given on the command line.
+    static constexpr uint16_t kDefaultRpcServerPort = 33000;
+
     struct AppConfig
     {
         std::vector<DeviceTypeParser::Entry> deviceTypeEntries;
@@ -47,6 +52,12 @@ class AppOptions
         uint8_t testEventTriggerEnableKey[16] = { 0 };
         bool enableWiFi                       = false;
         uint32_t bleController                = 0;
+        /// Listen port for the Pigweed RPC server, unset when --rpc-server-port was not given.
+        /// This is unconditionally parsed, but only consumed by builds compiled with Pigweed RPC
+        /// support (chip_enable_pw_rpc), so that the option stays unit-testable and the struct
+        /// layout does not vary per build flavor. Callers should fall back to
+        /// kDefaultRpcServerPort.
+        std::optional<uint16_t> rpcServerPort;
 #if CHIP_DEVICE_CONFIG_ENABLE_WIFIPAF
         std::string wifipafExtCmds;
         // Frequencies in MHz parsed out of "--wifipaf freq_list=", in the order given.
@@ -58,6 +69,11 @@ class AppOptions
 
     static const AppConfig & GetConfig();
 
+    /// Parse a TCP/UDP port number given on the command line. Accepts decimal, octal and
+    /// hexadecimal notation (strtoul with base 0). Returns false, leaving `port` untouched,
+    /// when the value is empty, is not entirely numeric, or does not fit in a uint16_t.
+    static bool ParsePortNumber(const char * value, uint16_t & port);
+
 #if CHIP_DEVICE_CONFIG_ENABLE_WIFIPAF
     /// Parse the frequencies out of a "--wifipaf" argument of the form
     /// "freq_list=<freq_1>,<freq_2>...".  Returns an empty list when the key is absent

examples/all-devices-app/posix/app_options/tests/TestAppOptions.cpp

@@ -24,6 +24,77 @@
 #include <string>
 #include <vector>
 
+namespace {
+
+TEST(TestAppOptionsPortNumber, AcceptsPortsAcrossTheWholeRange)
+{
+    uint16_t port = 0;
+
+    EXPECT_TRUE(AppOptions::ParsePortNumber("5540", port));
+    EXPECT_EQ(port, static_cast<uint16_t>(5540));
+
+    EXPECT_TRUE(AppOptions::ParsePortNumber("33000", port));
+    EXPECT_EQ(port, static_cast<uint16_t>(33000));
+
+    // 0 stays acceptable: --port already allowed it before the parsing was shared.
+    EXPECT_TRUE(AppOptions::ParsePortNumber("0", port));
+    EXPECT_EQ(port, static_cast<uint16_t>(0));
+
+    EXPECT_TRUE(AppOptions::ParsePortNumber("65535", port));
+    EXPECT_EQ(port, static_cast<uint16_t>(65535));
+}
+
+TEST(TestAppOptionsPortNumber, AcceptsHexadecimalAndOctalNotation)
+{
+    uint16_t port = 0;
+
+    EXPECT_TRUE(AppOptions::ParsePortNumber("0x80e8", port));
+    EXPECT_EQ(port, static_cast<uint16_t>(33000));
+
+    EXPECT_TRUE(AppOptions::ParsePortNumber("010", port));
+    EXPECT_EQ(port, static_cast<uint16_t>(8));
+}
+
+TEST(TestAppOptionsPortNumber, RejectsValuesOutsideAUint16)
+{
+    uint16_t port = 1;
+
+    EXPECT_FALSE(AppOptions::ParsePortNumber("65536", port));
+    EXPECT_FALSE(AppOptions::ParsePortNumber("70000", port));
+    EXPECT_FALSE(AppOptions::ParsePortNumber("-1", port));
+
+    // Rejected input must leave the caller's value untouched.
+    EXPECT_EQ(port, static_cast<uint16_t>(1));
+}
+
+TEST(TestAppOptionsPortNumber, RejectsInputThatIsNotEntirelyNumeric)
+{
+    uint16_t port = 1;
+
+    EXPECT_FALSE(AppOptions::ParsePortNumber(nullptr, port));
+    EXPECT_FALSE(AppOptions::ParsePortNumber("", port));
+    EXPECT_FALSE(AppOptions::ParsePortNumber("abc", port));
+    EXPECT_FALSE(AppOptions::ParsePortNumber("5540x", port));
+    EXPECT_FALSE(AppOptions::ParsePortNumber("5540 ", port));
+
+    EXPECT_EQ(port, static_cast<uint16_t>(1));
+}
+
+TEST(TestAppOptionsRpcServerPort, IsUnsetUntilTheOptionIsGiven)
+{
+    // Left unset so that a non-PW_RPC build can tell "--rpc-server-port 33000" apart from the
+    // option not being passed at all, and warn in the former case.
+    const AppOptions::AppConfig defaults;
+    EXPECT_FALSE(defaults.rpcServerPort.has_value());
+
+    // Keep in sync with LinuxDeviceOptions::rpcServerPort so that tooling which does not pass
+    // --rpc-server-port keeps reaching the app.
+    EXPECT_EQ(AppOptions::kDefaultRpcServerPort, static_cast<uint16_t>(33000));
+    EXPECT_EQ(defaults.rpcServerPort.value_or(AppOptions::kDefaultRpcServerPort), AppOptions::kDefaultRpcServerPort);
+}
+
+} // namespace
+
 #if CHIP_DEVICE_CONFIG_ENABLE_WIFIPAF
 
 namespace {

examples/all-devices-app/posix/main.cpp

@@ -398,8 +398,15 @@ void RunApplication(AppMainLoopImplementation * mainLoop = nullptr)
     static chip::app::PigweedAttributeAccessor sPwOobAccessor;
     chip::rpc::PigweedDebugAccessInterceptorRegistry::Instance().Register(&sPwOobAccessor);
 
-    chip::rpc::Init(33000); // TODO: Add an arg for Pw port.
-    ChipLogProgress(AppServer, "PW_RPC initialized.");
+    const uint16_t rpcServerPort = AppOptions::GetConfig().rpcServerPort.value_or(AppOptions::kDefaultRpcServerPort);
+    chip::rpc::Init(rpcServerPort);
+    ChipLogProgress(AppServer, "PW_RPC initialized on port %u.", rpcServerPort);
+#else
+    if (AppOptions::GetConfig().rpcServerPort.has_value())
+    {
+        ChipLogError(AppServer,
+                     "--rpc-server-port was specified, but this binary was built without Pigweed RPC support. Ignoring it.");
+    }
 #endif // PW_RPC_ENABLED
 
     // Init ZCL Data Model and CHIP App Server