Make supported Wi-Fi bands API less error prone (#31349)

* Make supported Wi-Fi bands API less error prone - Return a bitmask rather than mutating a span which could have led to misuse. - Make Network Commissioning cluster revision programmatic to avoid rev 1 leftovers (illegal starting at 1.3). - Fix an internal error case on writes. Fixes #30107 Issue #30110 Testing done: - Existing integration tests still pass with refactor. * Restyled by clang-format * Fix CI * Address review comments * Fix CI --------- Co-authored-by: Restyled.io <[email protected]> Co-authored-by: [email protected] <[email protected]>
project-chip · Feb 20, 2024 · 1704626 · 1704626
1 parent 3624470
commit 1704626
Show file tree

Hide file tree

Showing 6 changed files with 56 additions and 42 deletions.
diff --git a/src/app/clusters/network-commissioning/network-commissioning.cpp b/src/app/clusters/network-commissioning/network-commissioning.cpp
@@ -1,6 +1,6 @@
 /*
  *
- *    Copyright (c) 2021 Project CHIP Authors
+ *    Copyright (c) 2021-2024 Project CHIP Authors
  *    All rights reserved.
  *
  *    Licensed under the Apache License, Version 2.0 (the "License");
@@ -16,6 +16,8 @@
  *    limitations under the License.
  */
 
+#include <limits>
+
 #include "network-commissioning.h"
 
 #include <app-common/zap-generated/attributes/Accessors.h>
@@ -54,8 +56,7 @@ namespace {
 // For WiFi and Thread scan results, each item will cost ~60 bytes in TLV, thus 15 is a safe upper bound of scan results.
 constexpr size_t kMaxNetworksInScanResponse = 15;
 
-// The maximum number of Wi-Fi bands a device can support.
-constexpr size_t kMaxWiFiBands = 6;
+constexpr uint16_t kCurrentClusterRevision = 2;
 
 #if CHIP_DEVICE_CONFIG_ENABLE_WIFI_PDC
 constexpr size_t kPossessionNonceSize = 32;
@@ -269,22 +270,31 @@ CHIP_ERROR Instance::Read(const ConcreteReadAttributePath & aPath, AttributeValu
     case Attributes::FeatureMap::Id:
         return aEncoder.Encode(mFeatureFlags);
 
+    case Attributes::ClusterRevision::Id:
+        return aEncoder.Encode(kCurrentClusterRevision);
+
     case Attributes::SupportedWiFiBands::Id:
-        VerifyOrReturnError(mFeatureFlags.Has(Feature::kWiFiNetworkInterface), CHIP_NO_ERROR);
-        VerifyOrReturnError(mpDriver.Valid(), CHIP_NO_ERROR);
+#if (!CHIP_DEVICE_CONFIG_ENABLE_WIFI_STATION && !CHIP_DEVICE_CONFIG_ENABLE_WIFI_AP)
+        return CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute);
+#else
+        VerifyOrReturnError(mFeatureFlags.Has(Feature::kWiFiNetworkInterface), CHIP_IM_GLOBAL_STATUS(UnsupportedAttribute));
 
-        return aEncoder.EncodeList([this](const auto & encoder) -> CHIP_ERROR {
-            WiFiBand bandsBuffer[kMaxWiFiBands];
-            Span<WiFiBand> bands(bandsBuffer);
-            ReturnErrorOnFailure(mpDriver.Get<WiFiDriver *>()->GetSupportedWiFiBands(bands));
+        return aEncoder.EncodeList([this](const auto & encoder) {
+            uint32_t bands = mpDriver.Get<WiFiDriver *>()->GetSupportedWiFiBandsMask();
 
-            for (WiFiBand band : bands)
+            // Extract every band from the bitmap of supported bands, starting positionally on the right.
+            for (uint32_t band_bit_pos = 0; band_bit_pos < std::numeric_limits<uint32_t>::digits; ++band_bit_pos)
             {
-                ReturnErrorOnFailure(encoder.Encode(band));
+                uint32_t band_mask = static_cast<uint32_t>(1UL << band_bit_pos);
+                if ((bands & band_mask) != 0)
+                {
+                    ReturnErrorOnFailure(encoder.Encode(static_cast<WiFiBandEnum>(band_bit_pos)));
+                }
             }
-
             return CHIP_NO_ERROR;
         });
+#endif
+        break;
 
     case Attributes::SupportedThreadFeatures::Id:
         VerifyOrReturnError(mFeatureFlags.Has(Feature::kThreadNetworkInterface), CHIP_NO_ERROR);
@@ -310,7 +320,7 @@ CHIP_ERROR Instance::Write(const ConcreteDataAttributePath & aPath, AttributeVal
         ReturnErrorOnFailure(aDecoder.Decode(value));
         return mpBaseDriver->SetEnabled(value);
     default:
-        return CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE;
+        return CHIP_IM_GLOBAL_STATUS(InvalidAction);
     }
 }
 

diff --git a/src/app/tests/suites/certification/Test_TC_CNET_1_3.yaml b/src/app/tests/suites/certification/Test_TC_CNET_1_3.yaml
@@ -37,7 +37,7 @@ tests:
       command: "readAttribute"
       attribute: "ClusterRevision"
       response:
-          value: 1
+          value: 2
           constraints:
               type: int16u
 

diff --git a/src/include/platform/NetworkCommissioning.h b/src/include/platform/NetworkCommissioning.h
@@ -29,13 +29,15 @@
 #include <lib/core/CHIPSafeCasts.h>
 #include <lib/core/Optional.h>
 #include <lib/support/ThreadOperationalDataset.h>
+#include <lib/support/TypeTraits.h>
 #include <lib/support/Variant.h>
 #include <platform/CHIPDeviceConfig.h>
 #include <platform/internal/DeviceNetworkInfo.h>
 
 #include <app-common/zap-generated/cluster-enums.h>
 
 #include <limits>
+#include <utility>
 
 namespace chip {
 namespace DeviceLayer {
@@ -129,9 +131,13 @@ using NetworkIterator            = Iterator<Network>;
 using WiFiScanResponseIterator   = Iterator<WiFiScanResponse>;
 using ThreadScanResponseIterator = Iterator<ThreadScanResponse>;
 using Status                     = app::Clusters::NetworkCommissioning::NetworkCommissioningStatusEnum;
-using WiFiBand                   = app::Clusters::NetworkCommissioning::WiFiBandEnum;
-using WiFiSecurity               = app::Clusters::NetworkCommissioning::WiFiSecurityBitmap;
-using ThreadCapabilities         = app::Clusters::NetworkCommissioning::ThreadCapabilitiesBitmap;
+using WiFiBandEnum               = app::Clusters::NetworkCommissioning::WiFiBandEnum;
+// For backwards compatibility with pre-rename enum values.
+using WiFiBand           = WiFiBandEnum;
+using WiFiSecurityBitmap = app::Clusters::NetworkCommissioning::WiFiSecurityBitmap;
+// For backwards compatibility with pre-rename bitmap values.
+using WiFiSecurity       = WiFiSecurityBitmap;
+using ThreadCapabilities = app::Clusters::NetworkCommissioning::ThreadCapabilitiesBitmap;
 
 // BaseDriver and WirelessDriver are the common interfaces for a network driver, platform drivers should not implement this
 // directly, instead, users are expected to implement WiFiDriver, ThreadDriver and EthernetDriver.
@@ -352,24 +358,26 @@ class WiFiDriver : public Internal::WirelessDriver
 #endif // CHIP_DEVICE_CONFIG_ENABLE_WIFI_PDC
 
     /**
-     *  @brief Provide all the frequency bands supported by the Wi-Fi interface
+     *  @brief Provide all the frequency bands supported by the Wi-Fi interface.
      *
      *  The default implementation returns the 2.4 GHz band support.
-     *  Note: WiFi platforms should implement this function in their WiFiDriver to provide their complete device capabilities
+     *  Note: WiFi platforms should implement this function in their WiFiDriver to provide their complete device capabilities.
      *
-     *  @param bands Reference to a span that is used to return the supported bands.
-     *               The span is initialized with a fixed-size buffer.
-     *               The implementation is expected to resize the span to match the number of returned results.
+     *  The returned bit mask has values of WiFiBandEnum packed into the bits. For example:
      *
-     *  @return CHIP_NO_ERROR on success or a CHIP_ERROR on failure.
+     *    - Bit 0 = (WiFiBandEnum::k2g4 == 0) --> (1 << 0) == (1 << WiFiBandEnum::k2g4)
+     *    - Bit 2 = (WiFiBandEnum::k5g == 2) --> (1 << 2) == (1 << WiFiBandEnum::k5g)
+     *    - If both 2.4G and 5G are supported --> ((1 << k2g4) || (1 << k5g)) == (1 || 4) == 5
+     *
+     *  On error, return 0 (no bands supported). This should never happen... Note that
+     *  certification tests will REQUIRE at least one bit set in the set.
+     *
+     *  @return a bitmask of supported Wi-Fi bands where each bit is associated with a WiFiBandEnum value.
      */
-    virtual CHIP_ERROR GetSupportedWiFiBands(Span<WiFiBand> & bands)
+    virtual uint32_t GetSupportedWiFiBandsMask() const
     {
-        ReturnErrorCodeIf(bands.empty(), CHIP_ERROR_BUFFER_TOO_SMALL);
-        bands.front() = WiFiBand::k2g4;
-        bands.reduce_size(1);
-
-        return CHIP_NO_ERROR;
+        // Default to 2.4G support (100% example platform coverage as of Matter 1.3) listed.
+        return static_cast<uint32_t>(1UL << chip::to_underlying(WiFiBandEnum::k2g4));
     }
 
     ~WiFiDriver() override = default;

diff --git a/src/platform/nrfconnect/wifi/NrfWiFiDriver.cpp b/src/platform/nrfconnect/wifi/NrfWiFiDriver.cpp
@@ -17,10 +17,13 @@
 
 #include "NrfWiFiDriver.h"
 
+#include <stdint.h>
+
 #include <platform/KeyValueStoreManager.h>
 
 #include <lib/support/CodeUtils.h>
 #include <lib/support/SafeInt.h>
+#include <lib/support/TypeTraits.h>
 #include <platform/CHIPDeviceLayer.h>
 
 using namespace ::chip;
@@ -276,20 +279,13 @@ void NrfWiFiDriver::ScanNetworks(ByteSpan ssid, WiFiDriver::ScanCallback * callb
     }
 }
 
-CHIP_ERROR NrfWiFiDriver::GetSupportedWiFiBands(Span<WiFiBand> & bands)
+uint32_t NrfWiFiDriver::GetSupportedWiFiBandsMask() const
 {
-    static constexpr WiFiBand kBands[] = {
-        WiFiBand::k2g4,
+    uint32_t bands = static_cast<uint32_t>(1UL << chip::to_underlying(WiFiBandEnum::k2g4));
 #ifndef CONFIG_BOARD_NRF7001
-        WiFiBand::k5g,
+    bands |= static_cast<uint32_t>(1UL << chip::to_underlying(WiFiBandEnum::k5g));
 #endif
-    };
-
-    VerifyOrReturnError(ArraySize(kBands) <= bands.size(), CHIP_ERROR_BUFFER_TOO_SMALL);
-    memcpy(bands.data(), kBands, sizeof(kBands));
-    bands.reduce_size(ArraySize(kBands));
-
-    return CHIP_NO_ERROR;
+    return bands;
 }
 
 } // namespace NetworkCommissioning

diff --git a/src/platform/nrfconnect/wifi/NrfWiFiDriver.h b/src/platform/nrfconnect/wifi/NrfWiFiDriver.h
@@ -86,7 +86,7 @@ class NrfWiFiDriver final : public WiFiDriver
     Status AddOrUpdateNetwork(ByteSpan ssid, ByteSpan credentials, MutableCharSpan & outDebugText,
                               uint8_t & outNetworkIndex) override;
     void ScanNetworks(ByteSpan ssid, ScanCallback * callback) override;
-    CHIP_ERROR GetSupportedWiFiBands(Span<WiFiBand> & bands) override;
+    uint32_t GetSupportedWiFiBandsMask() const override;
 
     static NrfWiFiDriver & Instance()
     {

diff --git a/zzz_generated/darwin-framework-tool/zap-generated/test/Commands.h b/zzz_generated/darwin-framework-tool/zap-generated/test/Commands.h