[spinel] make max power table restoration work and survive old RCPs (#13524)

`RestoreProperties()` re-applies the max power table behind `#if
OPENTHREAD_POSIX_CONFIG_MAX_POWER_TABLE_ENABLE`, but
`radio_spinel.cpp` never sees that macro. It is a POSIX macro, and
this is a platform-agnostic library whose only project config hook is
OPENTHREAD_PROJECT_LIB_CONFIG_FILE, from `OT_LIB_CONFIG`, which is not
set anywhere. `-DOT_POSIX_MAX_POWER_TABLE=ON` puts the definition on
`ot-posix-config`, which only `openthread-posix` links.

So the block is compiled out everywhere, including in
`script/cmake-build posix` and `script/check-posix-pty`, which passes
`max-power-table` in its radio URL.  The values are applied at startup
and silently lost on the next RCP reset.

Dropping the guard exposes a second problem. An RCP without
`SPINEL_PROP_PHY_CHAN_MAX_POWER` answers
`SPINEL_STATUS_PROP_NOT_FOUND`, which `spinel_status_to_ot_error()`
maps to `OT_ERROR_NOT_IMPLEMENTED` -- not the `OT_ERROR_NOT_FOUND` the
code tolerates. Every RCP reset would then be fatal.  The loop now
warns once, clears `mMaxPowerTableSet` and stops, so a refusing RCP is
not asked again on later recoveries either.

That flag also guards the loop, following the `mTransmitPowerSet` and
`mCcaEnergyDetectThresholdSet` flags beside it, and is set only after
`Set()` succeeds. The loop calls `Set()` directly rather than
`SetChannelMaxTransmitPower()`, as the other restores here do: the
channel range was already checked when the value was configured, and
writing the value back into the table it was just read from would be
circular now that the setter also raises the flag. `MaxPowerTable`
initialises every channel to `kPowerDefault`, so without it the loop
would issue 16 blocking transactions on every restore for everyone,
including those who never pass `max-power-table`. Comparing against
`kPowerDefault` instead would have been cheaper but not equivalent: it
cannot tell an unconfigured channel from one deliberately set to 30
dBm.

With the macro no longer used outside the POSIX platform, enabling it by
default there makes `max-power-table` reachable, like `cca-threshold` and
`fem-lnagain` already are.

Three gtest cases cover it. The fake platform has to implement
`otPlatRadioSetChannelMaxTransmitPower` for them: the weak default in
`radio_platform.cpp` reports `kErrorNotImplemented`, so without an
implementation a test cannot tell a correct restore from one that
never arrived.

  shouldRestoreEachChannelWithItsOwnPower       three channels, distinct
                                                powers, asserted per channel
                                                after a restore
  shouldNotTouchTheRcpWhenNoChannelWasConfigured  pins the flag
  shouldSurviveAnRcpThatDoesNotImplementIt      refusing RCP, must not abort
                                                and must not retry

The first and third fail without the corresponding fix; the second
passes either way, because the removed guard takes the same loop with
it.

Also measured against a simulated RCP behind a socat PTY, with an RCP
reset forced by freezing the RCP process, using
`SPINEL_PROP_MAC_15_4_PANID` as the control for whether a restore ran
at all:

  config                                     startup  control  restored  alive
  before                                          16      10x         0    yes
  after                                           16      10x        80    yes
  after, no max-power-table in the URL             0      10x         0    yes
  old error handling, block on, old RCP            0       6x         0     NO
  after,                        old RCP            0      10x         0    yes
  old error handling, block on, capable RCP       16      10x        80    yes

Built and tested with OT_RCP_RESTORATION_MAX_COUNT at both 0 and 2.

src/lib/spinel/radio_spinel.cpp

@@ -95,6 +95,7 @@ RadioSpinel::RadioSpinel(void)
     , mMacKeySet(false)
     , mCcaEnergyDetectThresholdSet(false)
     , mTransmitPowerSet(false)
+    , mMaxPowerTableSet(false)
     , mCoexEnabledSet(false)
     , mFemLnaGainSet(false)
     , mEnergyScanning(false)
@@ -2271,23 +2272,40 @@ void RadioSpinel::RestoreProperties(void)
         SuccessOrDie(Set(SPINEL_PROP_PHY_FEM_LNA_GAIN, SPINEL_DATATYPE_INT8_S, mFemLnaGain));
     }
 
-#if OPENTHREAD_POSIX_CONFIG_MAX_POWER_TABLE_ENABLE
-    for (uint8_t channel = Radio::kChannelMin; channel <= Radio::kChannelMax; channel++)
+    // Guarded, because the table holds `kPowerDefault` for every channel unless
+    // the radio URL configured one. Without this, every restore would issue 16
+    // blocking transactions for users who never pass `max-power-table`.
+    if (mMaxPowerTableSet)
     {
-        int8_t power = mMaxPowerTable.GetTransmitPower(channel);
-
-        if (power != OT_RADIO_POWER_INVALID)
+        for (uint8_t channel = Radio::kChannelMin; channel <= Radio::kChannelMax; channel++)
         {
-            // Some old RCPs doesn't support max transmit power
-            otError error = SetChannelMaxTransmitPower(channel, power);
+            int8_t power = mMaxPowerTable.GetTransmitPower(channel);
 
-            if (error != OT_ERROR_NONE && error != OT_ERROR_NOT_FOUND)
+            if (power != OT_RADIO_POWER_INVALID)
             {
-                DieNow(OT_EXIT_FAILURE);
+                otError error =
+                    Set(SPINEL_PROP_PHY_CHAN_MAX_POWER, SPINEL_DATATYPE_UINT8_S SPINEL_DATATYPE_INT8_S, channel, power);
+
+                // An RCP without `SPINEL_PROP_PHY_CHAN_MAX_POWER` answers with
+                // `SPINEL_STATUS_PROP_NOT_FOUND`, which maps to
+                // `OT_ERROR_NOT_IMPLEMENTED`, not the `OT_ERROR_NOT_FOUND` this
+                // used to tolerate. Treating it as fatal would turn an ordinary
+                // RCP reset into an exit. Warn once and stop: the remaining
+                // channels would answer the same way.
+                if (error == OT_ERROR_NOT_IMPLEMENTED || error == OT_ERROR_NOT_FOUND)
+                {
+                    LogWarn("The RCP doesn't support setting the max transmit power");
+                    mMaxPowerTableSet = false;
+                    break;
+                }
+
+                if (error != OT_ERROR_NONE)
+                {
+                    DieNow(OT_EXIT_FAILURE);
+                }
             }
         }
     }
-#endif // OPENTHREAD_POSIX_CONFIG_MAX_POWER_TABLE_ENABLE
 
     if ((sRadioCaps & OT_RADIO_CAPS_RX_ON_WHEN_IDLE) != 0)
     {
@@ -2339,7 +2357,12 @@ otError RadioSpinel::SetChannelMaxTransmitPower(uint8_t aChannel, int8_t aMaxPow
     otError error = OT_ERROR_NONE;
     VerifyOrExit(aChannel >= Radio::kChannelMin && aChannel <= Radio::kChannelMax, error = OT_ERROR_INVALID_ARGS);
     mMaxPowerTable.SetTransmitPower(aChannel, aMaxPower);
-    error = Set(SPINEL_PROP_PHY_CHAN_MAX_POWER, SPINEL_DATATYPE_UINT8_S SPINEL_DATATYPE_INT8_S, aChannel, aMaxPower);
+    SuccessOrExit(error = Set(SPINEL_PROP_PHY_CHAN_MAX_POWER, SPINEL_DATATYPE_UINT8_S SPINEL_DATATYPE_INT8_S, aChannel,
+                              aMaxPower));
+
+#if OPENTHREAD_SPINEL_CONFIG_RCP_RESTORATION_MAX_COUNT > 0
+    mMaxPowerTableSet = true;
+#endif
 
 exit:
     return error;

src/lib/spinel/radio_spinel.hpp

@@ -1329,6 +1329,7 @@ private:
     bool mMacKeySet : 1;                   ///< Whether MAC key has been set.
     bool mCcaEnergyDetectThresholdSet : 1; ///< Whether CCA energy detect threshold has been set.
     bool mTransmitPowerSet : 1;            ///< Whether transmit power has been set.
+    bool mMaxPowerTableSet : 1;            ///< Whether any channel's max transmit power has been set.
     bool mCoexEnabledSet : 1;              ///< Whether coex enabled has been set.
     bool mFemLnaGainSet : 1;               ///< Whether FEM LNA gain has been set.
     bool mEnergyScanning : 1;              ///< If fails while scanning, restarts scanning.

src/posix/platform/openthread-posix-config.h

@@ -113,7 +113,7 @@
  * Define as 1 to enable max power table support.
  */
 #ifndef OPENTHREAD_POSIX_CONFIG_MAX_POWER_TABLE_ENABLE
-#define OPENTHREAD_POSIX_CONFIG_MAX_POWER_TABLE_ENABLE 0
+#define OPENTHREAD_POSIX_CONFIG_MAX_POWER_TABLE_ENABLE 1
 #endif
 
 /**

tests/gtest/fake_platform.cpp

@@ -414,6 +414,14 @@ void otPlatRadioEnableSrcMatch(otInstance *, bool aEnabled)
     FakePlatform::CurrentPlatform().SrcMatchEnable(aEnabled);
 }
 
+// Overrides the weak default in radio_platform.cpp, which reports
+// `kErrorNotImplemented`. Without this the fake RCP cannot accept a max power
+// table at all, so a test could not tell a correct restore from a missing one.
+otError otPlatRadioSetChannelMaxTransmitPower(otInstance *, uint8_t aChannel, int8_t aMaxPower)
+{
+    return FakePlatform::CurrentPlatform().ChannelMaxTxPowerSet(aChannel, aMaxPower);
+}
+
 otError otPlatRadioAddSrcMatchShortEntry(otInstance *, uint16_t aShortAddr)
 {
     FakePlatform::CurrentPlatform().SrcMatchAddShortEntry(aShortAddr);

tests/gtest/fake_platform.hpp

@@ -127,6 +127,33 @@ public:
     virtual void   SrcMatchClearExtEntries(void) { mSrcMatchExtAddrs.clear(); }
     virtual size_t SrcMatchCountExtEntries(void) const { return mSrcMatchExtAddrs.size(); }
 
+    // Records what the RCP was told, so a test can assert which channel got
+    // which power rather than only that something was sent.
+    static constexpr int8_t kNoMaxTxPower = INT8_MIN;
+
+    virtual otError ChannelMaxTxPowerSet(uint8_t aChannel, int8_t aPower)
+    {
+        otError error = mChannelMaxTxPowerError;
+
+        if (error == OT_ERROR_NONE)
+        {
+            mChannelMaxTxPower[aChannel] = aPower;
+        }
+
+        return error;
+    }
+
+    // Makes the fake RCP refuse the property, the way a firmware without
+    // `SPINEL_PROP_PHY_CHAN_MAX_POWER` does.
+    virtual void   ChannelMaxTxPowerFailWith(otError aError) { mChannelMaxTxPowerError = aError; }
+    virtual int8_t ChannelMaxTxPowerGet(uint8_t aChannel) const
+    {
+        auto it = mChannelMaxTxPower.find(aChannel);
+        return (it == mChannelMaxTxPower.end()) ? kNoMaxTxPower : it->second;
+    }
+    virtual void   ChannelMaxTxPowerClear(void) { mChannelMaxTxPower.clear(); }
+    virtual size_t ChannelMaxTxPowerCount(void) const { return mChannelMaxTxPower.size(); }
+
 protected:
     void ProcessSchedules(uint64_t &aTimeout);
 
@@ -159,9 +186,11 @@ protected:
 
     std::map<uint32_t, std::vector<std::vector<uint8_t>>> mSettings;
 
-    bool                   mSrcMatchEnabled = false;
-    std::set<uint16_t>     mSrcMatchShortAddrs;
-    std::set<otExtAddress> mSrcMatchExtAddrs;
+    bool                      mSrcMatchEnabled = false;
+    std::set<uint16_t>        mSrcMatchShortAddrs;
+    std::set<otExtAddress>    mSrcMatchExtAddrs;
+    std::map<uint8_t, int8_t> mChannelMaxTxPower;
+    otError                   mChannelMaxTxPowerError = OT_ERROR_NONE;
 };
 
 template <> inline void FakePlatform::HandleSchedule<&FakePlatform::mMilliAlarmStart>()

tests/gtest/radio_spinel_rcp_test.cpp

@@ -492,6 +492,94 @@ TEST(RadioSpinelSrcMatch, shouldBeAbleToClearAllRadioSrcMatchExtEntres)
 }
 
 #if OPENTHREAD_SPINEL_CONFIG_RCP_RESTORATION_MAX_COUNT > 0
+TEST(RadioSpinelMaxPowerTable, shouldRestoreEachChannelWithItsOwnPower)
+{
+    constexpr uint8_t kChannelA = 11;
+    constexpr uint8_t kChannelB = 15;
+    constexpr uint8_t kChannelC = 26;
+    constexpr int8_t  kPowerA   = 5;
+    constexpr int8_t  kPowerB   = 14;
+    constexpr int8_t  kPowerC   = 20;
+
+    FakeCoprocessorPlatform platform;
+
+    ASSERT_EQ(platform.mRadioSpinel.Enable(FakePlatform::CurrentInstance()), kErrorNone);
+
+    ASSERT_EQ(platform.mRadioSpinel.SetChannelMaxTransmitPower(kChannelA, kPowerA), kErrorNone);
+    ASSERT_EQ(platform.mRadioSpinel.SetChannelMaxTransmitPower(kChannelB, kPowerB), kErrorNone);
+    ASSERT_EQ(platform.mRadioSpinel.SetChannelMaxTransmitPower(kChannelC, kPowerC), kErrorNone);
+
+    ASSERT_EQ(platform.ChannelMaxTxPowerGet(kChannelA), kPowerA);
+    ASSERT_EQ(platform.ChannelMaxTxPowerGet(kChannelB), kPowerB);
+    ASSERT_EQ(platform.ChannelMaxTxPowerGet(kChannelC), kPowerC);
+
+    // Forget what the RCP was told, so what follows can only come from the
+    // restore itself.
+    platform.ChannelMaxTxPowerClear();
+    ASSERT_EQ(platform.ChannelMaxTxPowerCount(), 0u);
+
+    platform.mRadioSpinel.RestoreProperties();
+
+    // Each configured channel comes back with its own power, not with a
+    // neighbour's and not with the table default.
+    EXPECT_EQ(platform.ChannelMaxTxPowerGet(kChannelA), kPowerA);
+    EXPECT_EQ(platform.ChannelMaxTxPowerGet(kChannelB), kPowerB);
+    EXPECT_EQ(platform.ChannelMaxTxPowerGet(kChannelC), kPowerC);
+
+    // Channels never configured keep the table default, which is what the
+    // restore sends for them. Copied into a local first: `kPowerDefault` is a
+    // `static constexpr` with no out-of-line definition, and EXPECT_EQ binds
+    // its arguments by reference, which would odr-use it.
+    constexpr int8_t kDefaultPower = MaxPowerTable::kPowerDefault;
+    EXPECT_EQ(platform.ChannelMaxTxPowerGet(12), kDefaultPower);
+}
+
+TEST(RadioSpinelMaxPowerTable, shouldNotTouchTheRcpWhenNoChannelWasConfigured)
+{
+    FakeCoprocessorPlatform platform;
+
+    ASSERT_EQ(platform.mRadioSpinel.Enable(FakePlatform::CurrentInstance()), kErrorNone);
+
+    platform.ChannelMaxTxPowerClear();
+    platform.mRadioSpinel.RestoreProperties();
+
+    // Nothing was configured, so the restore must not send the table's
+    // defaults to the RCP -- 16 blocking transactions on every recovery for a
+    // feature nobody asked for.
+    EXPECT_EQ(platform.ChannelMaxTxPowerCount(), 0u);
+}
+
+TEST(RadioSpinelMaxPowerTable, shouldSurviveAnRcpThatDoesNotImplementIt)
+{
+    constexpr uint8_t kChannel = 11;
+    constexpr int8_t  kPower   = 5;
+
+    FakeCoprocessorPlatform platform;
+
+    ASSERT_EQ(platform.mRadioSpinel.Enable(FakePlatform::CurrentInstance()), kErrorNone);
+    ASSERT_EQ(platform.mRadioSpinel.SetChannelMaxTransmitPower(kChannel, kPower), kErrorNone);
+
+    // From here the RCP refuses the property. A firmware that lacks it answers
+    // `SPINEL_STATUS_PROP_NOT_FOUND` while this returns
+    // `SPINEL_STATUS_UNIMPLEMENTED`, but both reach the host as
+    // `OT_ERROR_NOT_IMPLEMENTED`, which is the path under test.
+    platform.ChannelMaxTxPowerFailWith(OT_ERROR_NOT_IMPLEMENTED);
+    platform.ChannelMaxTxPowerClear();
+
+    // Must not abort: this used to be a `DieNow()`, so a regression takes the
+    // whole test binary down rather than failing an assertion.
+    platform.mRadioSpinel.RestoreProperties();
+
+    EXPECT_EQ(platform.ChannelMaxTxPowerCount(), 0u);
+
+    // And it must give up rather than ask again on every later recovery, even
+    // once the RCP would accept it.
+    platform.ChannelMaxTxPowerFailWith(OT_ERROR_NONE);
+    platform.mRadioSpinel.RestoreProperties();
+
+    EXPECT_EQ(platform.ChannelMaxTxPowerCount(), 0u);
+}
+
 TEST(RadioSpinelSrcMatch, shouldNotDuplicateSrcMatchEntriesOnRestoreProperties)
 {
     constexpr uint16_t      kTestShortAddr = 0x1234;