From 2a8b4a57edb1b2272f4a07aee988446734147ca2 Mon Sep 17 00:00:00 2001 From: Sven Kirmess Date: Wed, 26 Aug 2026 22:50:54 +0200 Subject: [PATCH] [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 | 45 +++++++--- src/lib/spinel/radio_spinel.hpp | 1 + src/posix/platform/openthread-posix-config.h | 2 +- tests/gtest/fake_platform.cpp | 8 ++ tests/gtest/fake_platform.hpp | 35 +++++++- tests/gtest/radio_spinel_rcp_test.cpp | 88 ++++++++++++++++++++ 6 files changed, 164 insertions(+), 15 deletions(-) diff --git a/src/lib/spinel/radio_spinel.cpp b/src/lib/spinel/radio_spinel.cpp index caf36d21a..a9a406717 100644 --- a/src/lib/spinel/radio_spinel.cpp +++ b/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; diff --git a/src/lib/spinel/radio_spinel.hpp b/src/lib/spinel/radio_spinel.hpp index b3f5ecdc4..3a1e29030 100644 --- a/src/lib/spinel/radio_spinel.hpp +++ b/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. diff --git a/src/posix/platform/openthread-posix-config.h b/src/posix/platform/openthread-posix-config.h index ca7abd798..a5b5e3b90 100644 --- a/src/posix/platform/openthread-posix-config.h +++ b/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 /** diff --git a/tests/gtest/fake_platform.cpp b/tests/gtest/fake_platform.cpp index 5c43d00c8..3e4975b2a 100644 --- a/tests/gtest/fake_platform.cpp +++ b/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); diff --git a/tests/gtest/fake_platform.hpp b/tests/gtest/fake_platform.hpp index cdf398c69..764534cac 100644 --- a/tests/gtest/fake_platform.hpp +++ b/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>> mSettings; - bool mSrcMatchEnabled = false; - std::set mSrcMatchShortAddrs; - std::set mSrcMatchExtAddrs; + bool mSrcMatchEnabled = false; + std::set mSrcMatchShortAddrs; + std::set mSrcMatchExtAddrs; + std::map mChannelMaxTxPower; + otError mChannelMaxTxPowerError = OT_ERROR_NONE; }; template <> inline void FakePlatform::HandleSchedule<&FakePlatform::mMilliAlarmStart>() diff --git a/tests/gtest/radio_spinel_rcp_test.cpp b/tests/gtest/radio_spinel_rcp_test.cpp index 91644598a..bdcc20277 100644 --- a/tests/gtest/radio_spinel_rcp_test.cpp +++ b/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;