Stop trying to get NodeLabel and LocalConfigDisabled from configurati…

…on manager (#17758) * Stop trying to get NodeLabel and LocalConfigDisabled from configuration manager. We're never _storing_ these in the configuration manager, so getting them from there, on startup only, makes no sense. The added tests verify that we are in fact properly storing them in NVM. * Address review comments.
project-chip · Apr 27, 2022 · 1dd1e80 · 1dd1e80
1 parent 1f862e3
commit 1dd1e80
Show file tree

Hide file tree

Showing 19 changed files with 236 additions and 452 deletions.
diff --git a/examples/chip-tool-darwin/templates/tests/tests.js b/examples/chip-tool-darwin/templates/tests/tests.js
@@ -275,7 +275,8 @@ function getTests()
     'TestConstraints',
     'TestDelayCommands',
     'TestDescriptorCluster',
-    'TestBasicInformation',
+    // TestBasicInformation needs Reboot
+    //'TestBasicInformation',
     'TestGeneralCommissioning',
     'TestGroupsCluster',
     'TestGroupKeyManagementCluster',

diff --git a/src/app/clusters/basic/basic.cpp b/src/app/clusters/basic/basic.cpp
@@ -389,24 +389,22 @@ PlatformMgrDelegate gPlatformMgrDelegate;
 
 } // anonymous namespace
 
-void emberAfBasicClusterServerInitCallback(chip::EndpointId endpoint)
+namespace chip {
+namespace app {
+namespace Clusters {
+namespace Basic {
+bool IsLocalConfigDisabled()
 {
-    EmberAfStatus status;
-
-    char nodeLabel[DeviceLayer::ConfigurationManager::kMaxNodeLabelLength + 1];
-    if (ConfigurationMgr().GetNodeLabel(nodeLabel, sizeof(nodeLabel)) == CHIP_NO_ERROR)
-    {
-        status = Attributes::NodeLabel::Set(endpoint, chip::CharSpan::fromCharString(nodeLabel));
-        VerifyOrdo(EMBER_ZCL_STATUS_SUCCESS == status, ChipLogError(Zcl, "Error setting Node Label: 0x%02x", status));
-    }
-
-    bool localConfigDisabled;
-    if (ConfigurationMgr().GetLocalConfigDisabled(localConfigDisabled) == CHIP_NO_ERROR)
-    {
-        status = Attributes::LocalConfigDisabled::Set(endpoint, localConfigDisabled);
-        VerifyOrdo(EMBER_ZCL_STATUS_SUCCESS == status, ChipLogError(Zcl, "Error setting Local Config Disabled: 0x%02x", status));
-    }
+    bool disabled        = false;
+    EmberAfStatus status = LocalConfigDisabled::Get(0, &disabled);
+    return status == EMBER_ZCL_STATUS_SUCCESS && disabled;
 }
+} // namespace Basic
+} // namespace Clusters
+} // namespace app
+} // namespace chip
+
+void emberAfBasicClusterServerInitCallback(chip::EndpointId endpoint) {}
 
 void MatterBasicPluginServerInitCallback()
 {

diff --git a/src/app/clusters/basic/basic.h b/src/app/clusters/basic/basic.h
@@ -19,6 +19,21 @@
 
 #include <app/util/basic-types.h>
 
+namespace chip {
+namespace app {
+namespace Clusters {
+namespace Basic {
+/**
+ * Check whether LocalConfigDisabled is set (on endpoint 0, which is the only
+ * place the Basic Information cluster exists and can have the attribute be
+ * set).
+ */
+bool IsLocalConfigDisabled();
+} // namespace Basic
+} // namespace Clusters
+} // namespace app
+} // namespace chip
+
 /** @brief Basic Cluster Server Init
  *
  * This function is called at startup for a given endpoint to initialize

diff --git a/src/app/clusters/ota-requestor/DefaultOTARequestor.cpp b/src/app/clusters/ota-requestor/DefaultOTARequestor.cpp
@@ -20,6 +20,7 @@
  * OTA Requestor logic is contained in this class.
  */
 
+#include <app/clusters/basic/basic.h>
 #include <app/clusters/ota-requestor/ota-requestor-server.h>
 #include <lib/core/CHIPEncoding.h>
 #include <platform/CHIPDeviceLayer.h>
@@ -733,7 +734,7 @@ CHIP_ERROR DefaultOTARequestor::SendQueryImageRequest(OperationalDeviceProxy & d
     ReturnErrorOnFailure(DeviceLayer::ConfigurationMgr().GetSoftwareVersion(args.softwareVersion));
 
     args.protocolsSupported = kProtocolsSupported;
-    args.requestorCanConsent.SetValue(mOtaRequestorDriver->CanConsent());
+    args.requestorCanConsent.SetValue(!Basic::IsLocalConfigDisabled() && mOtaRequestorDriver->CanConsent());
 
     uint16_t hardwareVersion;
     if (DeviceLayer::ConfigurationMgr().GetHardwareVersion(hardwareVersion) == CHIP_NO_ERROR)

diff --git a/src/app/clusters/ota-requestor/ExtendedOTARequestorDriver.cpp b/src/app/clusters/ota-requestor/ExtendedOTARequestorDriver.cpp
@@ -30,12 +30,7 @@ constexpr System::Clock::Seconds32 kUserConsentPollInterval = System::Clock::Sec
 
 bool ExtendedOTARequestorDriver::CanConsent()
 {
-    bool localConfigDisabled = false;
-    VerifyOrdo(DeviceLayer::ConfigurationMgr().GetLocalConfigDisabled(localConfigDisabled) == CHIP_NO_ERROR,
-               ChipLogProgress(SoftwareUpdate, "Failed to get local config disabled, assuming not disabled"));
-
-    // User consent delegation SHALL NOT be used if a Node is configured with the LocalConfigDisabled attribute set to True
-    return localConfigDisabled == false;
+    return mUserConsentDelegate != nullptr;
 }
 
 void ExtendedOTARequestorDriver::UpdateAvailable(const UpdateDescription & update, System::Clock::Seconds32 delay)

diff --git a/src/app/tests/suites/TestBasicInformation.yaml b/src/app/tests/suites/TestBasicInformation.yaml
@@ -82,3 +82,75 @@ tests:
                   0xFFFB, # AttributeList
                   0xFFFD, # ClusterRevision
               ]
+
+    - label: "Read NodeLabel"
+      command: "readAttribute"
+      attribute: "NodeLabel"
+      response:
+          value: ""
+
+    - label: "Write NodeLabel"
+      command: "writeAttribute"
+      attribute: "NodeLabel"
+      arguments:
+          value: "My node"
+
+    - label: "Read back NodeLabel"
+      command: "readAttribute"
+      attribute: "NodeLabel"
+      response:
+          value: "My node"
+
+    - label: "Read LocalConfigDisabled"
+      command: "readAttribute"
+      attribute: "LocalConfigDisabled"
+      response:
+          value: false
+
+    - label: "Write LocalConfigDisabled"
+      command: "writeAttribute"
+      attribute: "LocalConfigDisabled"
+      arguments:
+          value: true
+
+    - label: "Read back LocalConfigDisabled"
+      command: "readAttribute"
+      attribute: "LocalConfigDisabled"
+      response:
+          value: true
+
+    - label: "Reboot the device"
+      cluster: "SystemCommands"
+      command: "Reboot"
+
+    - label: "Connect to the device again"
+      cluster: "DelayCommands"
+      command: "WaitForCommissionee"
+      arguments:
+          values:
+              - name: "nodeId"
+                value: nodeId
+
+    - label: "Read back NodeLabel after reboot"
+      command: "readAttribute"
+      attribute: "NodeLabel"
+      response:
+          value: "My node"
+
+    - label: "Restore initial NodeLabel value"
+      command: "writeAttribute"
+      attribute: "NodeLabel"
+      arguments:
+          value: ""
+
+    - label: "Read back LocalConfigDisabled after reboot"
+      command: "readAttribute"
+      attribute: "LocalConfigDisabled"
+      response:
+          value: true
+
+    - label: "Restore initial LocalConfigDisabled value"
+      command: "writeAttribute"
+      attribute: "LocalConfigDisabled"
+      arguments:
+          value: false
diff --git a/src/darwin/Framework/CHIP/templates/tests/tests.js b/src/darwin/Framework/CHIP/templates/tests/tests.js
@@ -271,7 +271,8 @@ function getTests()
     'TestConstraints',
     'TestDelayCommands',
     'TestDescriptorCluster',
-    'TestBasicInformation',
+    // TestBasicInformation needs support for Reboot with no discriminator
+    //'TestBasicInformation',
     'TestGeneralCommissioning',
     'TestGroupsCluster',
     'TestGroupKeyManagementCluster',

diff --git a/src/darwin/Framework/CHIPTests/CHIPClustersTests.m b/src/darwin/Framework/CHIPTests/CHIPClustersTests.m
diff --git a/src/include/platform/ConfigurationManager.h b/src/include/platform/ConfigurationManager.h
@@ -63,7 +63,6 @@ class ConfigurationManager
     {
         kMaxVendorNameLength            = 32,
         kMaxProductNameLength           = 32,
-        kMaxNodeLabelLength             = 32,
         kMaxLocationLength              = 2,
         kMaxHardwareVersionStringLength = 64,
         kMaxSoftwareVersionStringLength = 64,
@@ -123,12 +122,9 @@ class ConfigurationManager
     virtual CHIP_ERROR StoreTotalOperationalHours(uint32_t totalOperationalHours)      = 0;
     virtual CHIP_ERROR GetBootReason(uint32_t & bootReason)                            = 0;
     virtual CHIP_ERROR StoreBootReason(uint32_t bootReason)                            = 0;
-    virtual CHIP_ERROR GetNodeLabel(char * buf, size_t bufSize)                        = 0;
-    virtual CHIP_ERROR StoreNodeLabel(const char * buf, size_t bufSize)                = 0;
     virtual CHIP_ERROR GetPartNumber(char * buf, size_t bufSize)                       = 0;
     virtual CHIP_ERROR GetProductURL(char * buf, size_t bufSize)                       = 0;
     virtual CHIP_ERROR GetProductLabel(char * buf, size_t bufSize)                     = 0;
-    virtual CHIP_ERROR GetLocalConfigDisabled(bool & disabled)                         = 0;
     virtual CHIP_ERROR GetUniqueId(char * buf, size_t bufSize)                         = 0;
     virtual CHIP_ERROR StoreUniqueId(const char * uniqueId, size_t uniqueIdLen)        = 0;
     virtual CHIP_ERROR GenerateUniqueId(char * buf, size_t bufSize)                    = 0;

diff --git a/src/include/platform/internal/GenericConfigurationManagerImpl.h b/src/include/platform/internal/GenericConfigurationManagerImpl.h
@@ -104,12 +104,9 @@ class GenericConfigurationManagerImpl : public ConfigurationManager
     CHIP_ERROR StoreTotalOperationalHours(uint32_t totalOperationalHours) override;
     CHIP_ERROR GetBootReason(uint32_t & bootReason) override;
     CHIP_ERROR StoreBootReason(uint32_t bootReason) override;
-    CHIP_ERROR GetNodeLabel(char * buf, size_t bufSize) override;
-    CHIP_ERROR StoreNodeLabel(const char * buf, size_t bufSize) override;
     CHIP_ERROR GetPartNumber(char * buf, size_t bufSize) override;
     CHIP_ERROR GetProductURL(char * buf, size_t bufSize) override;
     CHIP_ERROR GetProductLabel(char * buf, size_t bufSize) override;
-    CHIP_ERROR GetLocalConfigDisabled(bool & disabled) override;
     CHIP_ERROR GetUniqueId(char * buf, size_t bufSize) override;
     CHIP_ERROR StoreUniqueId(const char * uniqueId, size_t uniqueIdLen) override;
     CHIP_ERROR GenerateUniqueId(char * buf, size_t bufSize) override;