Merge branch 'master' into add_smokeco_chef

src/app/CommandHandler.cpp

@@ -303,6 +303,9 @@ Status CommandHandler::ProcessCommandDataIB(CommandDataIB::Parser & aCommandElem
 
     if (CommandIsFabricScoped(concretePath.mClusterId, concretePath.mCommandId))
     {
+        // SPEC: Else if the command in the path is fabric-scoped and there is no accessing fabric,
+        // a CommandStatusIB SHALL be generated with the UNSUPPORTED_ACCESS Status Code.
+
         // Fabric-scoped commands are not allowed before a specific accessing fabric is available.
         // This is mostly just during a PASE session before AddNOC.
         if (GetAccessingFabricIndex() == kUndefinedFabricIndex)
@@ -470,8 +473,7 @@ CHIP_ERROR CommandHandler::AddStatus(const ConcreteCommandPath & aCommandPath, c
     return AddStatusInternal(aCommandPath, StatusIB(aStatus));
 }
 
-CHIP_ERROR CommandHandler::AddStatusAndLogIfFailure(const ConcreteCommandPath & aCommandPath, const Status aStatus,
-                                                    const char * aMessage)
+void CommandHandler::AddStatusAndLogIfFailure(const ConcreteCommandPath & aCommandPath, const Status aStatus, const char * aMessage)
 {
     if (aStatus != Status::Success)
     {
@@ -482,7 +484,15 @@ CHIP_ERROR CommandHandler::AddStatusAndLogIfFailure(const ConcreteCommandPath &
                      ChipLogValueIMStatus(aStatus), aMessage);
     }
 
-    return AddStatus(aCommandPath, aStatus);
+    CHIP_ERROR err = AddStatus(aCommandPath, aStatus);
+    if (err != CHIP_NO_ERROR)
+    {
+        ChipLogError(DataManagement,
+                     "Failed to set status on Endpoint=%u Cluster=" ChipLogFormatMEI " Command=" ChipLogFormatMEI
+                     ": %" CHIP_ERROR_FORMAT,
+                     aCommandPath.mEndpointId, ChipLogValueMEI(aCommandPath.mClusterId), ChipLogValueMEI(aCommandPath.mCommandId),
+                     err.Format());
+    }
 }
 
 CHIP_ERROR CommandHandler::AddClusterSpecificSuccess(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus)

src/app/CommandHandler.h

@@ -173,9 +173,10 @@ class CommandHandler : public Messaging::ExchangeDelegate
     CHIP_ERROR AddStatus(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus);
 
     // Same as AddStatus, but logs that the command represented by aCommandPath failed with the given
-    // error status and error message, if aStatus is an error.
-    CHIP_ERROR AddStatusAndLogIfFailure(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus,
-                                        const char * aMessage);
+    // error status and error message, if aStatus is an error. Errors on AddStatus are just logged
+    // (since the caller likely can only log and not further add a status).
+    void AddStatusAndLogIfFailure(const ConcreteCommandPath & aCommandPath, const Protocols::InteractionModel::Status aStatus,
+                                  const char * aMessage);
 
     CHIP_ERROR AddClusterSpecificSuccess(const ConcreteCommandPath & aCommandPath, ClusterStatus aClusterStatus);
 

src/app/clusters/group-key-mgmt-server/group-key-mgmt-server.cpp

@@ -401,7 +401,34 @@ ValidateKeySetWriteArguments(const chip::app::Clusters::GroupKeyManagement::Comm
     return Status::Success;
 }
 
-constexpr uint16_t GroupKeyManagementAttributeAccess::kClusterRevision;
+bool GetProviderAndFabric(chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
+                          Credentials::GroupDataProvider ** outGroupDataProvider, const FabricInfo ** outFabricInfo)
+{
+    VerifyOrDie(commandObj != nullptr);
+    VerifyOrDie(outGroupDataProvider != nullptr);
+    VerifyOrDie(outFabricInfo != nullptr);
+
+    // Internal failures on internal inconsistencies.
+    auto provider = GetGroupDataProvider();
+    auto fabric   = Server::GetInstance().GetFabricTable().FindFabricWithIndex(commandObj->GetAccessingFabricIndex());
+
+    if (nullptr == provider)
+    {
+        commandObj->AddStatusAndLogIfFailure(commandPath, Status::Failure, "Internal consistency error on provider!");
+        return false;
+    }
+
+    if (nullptr == fabric)
+    {
+        commandObj->AddStatusAndLogIfFailure(commandPath, Status::Failure, "Internal consistency error on access fabric!");
+        return false;
+    }
+
+    *outGroupDataProvider = provider;
+    *outFabricInfo        = fabric;
+
+    return true;
+}
 
 GroupKeyManagementAttributeAccess gAttribute;
 
@@ -420,12 +447,12 @@ bool emberAfGroupKeyManagementClusterKeySetWriteCallback(
     chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
     const chip::app::Clusters::GroupKeyManagement::Commands::KeySetWrite::DecodableType & commandData)
 {
-    auto provider = GetGroupDataProvider();
-    auto fabric   = Server::GetInstance().GetFabricTable().FindFabricWithIndex(commandObj->GetAccessingFabricIndex());
+    Credentials::GroupDataProvider * provider = nullptr;
+    const FabricInfo * fabric                 = nullptr;
 
-    if (nullptr == provider || nullptr == fabric)
+    if (!GetProviderAndFabric(commandObj, commandPath, &provider, &fabric))
     {
-        commandObj->AddStatusAndLogIfFailure(commandPath, Status::Failure, "Internal consistency error on provider/fabric");
+        // Command will already have status populated from validation.
         return true;
     }
 
@@ -516,32 +543,29 @@ bool emberAfGroupKeyManagementClusterKeySetWriteCallback(
     }
 
     // Send response
-    err = commandObj->AddStatus(commandPath, StatusIB(err).mStatus);
-    if (CHIP_NO_ERROR != err)
-    {
-        ChipLogDetail(Zcl, "GroupKeyManagementCluster: KeySetWrite failed: %" CHIP_ERROR_FORMAT, err.Format());
-    }
+    commandObj->AddStatus(commandPath, StatusIB(err).mStatus);
     return true;
 }
 
 bool emberAfGroupKeyManagementClusterKeySetReadCallback(
     chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
     const chip::app::Clusters::GroupKeyManagement::Commands::KeySetRead::DecodableType & commandData)
 {
-    auto fabric     = commandObj->GetAccessingFabricIndex();
-    auto * provider = GetGroupDataProvider();
+    Credentials::GroupDataProvider * provider = nullptr;
+    const FabricInfo * fabric                 = nullptr;
 
-    if (nullptr == provider)
+    if (!GetProviderAndFabric(commandObj, commandPath, &provider, &fabric))
     {
-        commandObj->AddStatus(commandPath, Status::Failure);
+        // Command will already have status populated from validation.
         return true;
     }
 
+    FabricIndex fabricIndex = fabric->GetFabricIndex();
     GroupDataProvider::KeySet keyset;
-    if (CHIP_NO_ERROR != provider->GetKeySet(fabric, commandData.groupKeySetID, keyset))
+    if (CHIP_NO_ERROR != provider->GetKeySet(fabricIndex, commandData.groupKeySetID, keyset))
     {
         // KeySet ID not found
-        commandObj->AddStatus(commandPath, Status::NotFound);
+        commandObj->AddStatusAndLogIfFailure(commandPath, Status::NotFound, "Keyset ID not found in KeySetRead");
         return true;
     }
 
@@ -592,30 +616,40 @@ bool emberAfGroupKeyManagementClusterKeySetRemoveCallback(
     const chip::app::Clusters::GroupKeyManagement::Commands::KeySetRemove::DecodableType & commandData)
 
 {
-    auto fabric     = commandObj->GetAccessingFabricIndex();
-    auto * provider = GetGroupDataProvider();
-    Status status   = Status::Failure;
+    Credentials::GroupDataProvider * provider = nullptr;
+    const FabricInfo * fabric                 = nullptr;
 
-    if (nullptr != provider)
+    if (!GetProviderAndFabric(commandObj, commandPath, &provider, &fabric))
     {
-        // Remove keyset
-        CHIP_ERROR err = provider->RemoveKeySet(fabric, commandData.groupKeySetID);
-        if (CHIP_ERROR_KEY_NOT_FOUND == err)
-        {
-            status = Status::NotFound;
-        }
-        else if (CHIP_NO_ERROR == err)
-        {
-            status = Status::Success;
-        }
+        // Command will already have status populated from validation.
+        return true;
     }
 
-    // Send response
-    CHIP_ERROR send_err = commandObj->AddStatus(commandPath, status);
-    if (CHIP_NO_ERROR != send_err)
+    if (commandData.groupKeySetID == GroupDataProvider::kIdentityProtectionKeySetId)
+    {
+        // SPEC: This command SHALL fail with an INVALID_COMMAND status code back to the initiator if the GroupKeySetID being
+        // removed is 0, which is the Key Set associated with the Identity Protection Key (IPK).
+        commandObj->AddStatusAndLogIfFailure(commandPath, Status::InvalidCommand,
+                                             "Attempted to KeySetRemove the identity protection key!");
+        return true;
+    }
+
+    // Remove keyset
+    FabricIndex fabricIndex = fabric->GetFabricIndex();
+    CHIP_ERROR err          = provider->RemoveKeySet(fabricIndex, commandData.groupKeySetID);
+
+    Status status = Status::Success;
+    if (CHIP_ERROR_KEY_NOT_FOUND == err)
+    {
+        status = Status::NotFound;
+    }
+    else if (CHIP_NO_ERROR != err)
     {
-        ChipLogDetail(Zcl, "GroupKeyManagementCluster: KeySetRemove failed: %" CHIP_ERROR_FORMAT, send_err.Format());
+        status = Status::Failure;
     }
+
+    // Send status response.
+    commandObj->AddStatusAndLogIfFailure(commandPath, status, "KeySetRemove failed");
     return true;
 }
 
@@ -654,19 +688,20 @@ bool emberAfGroupKeyManagementClusterKeySetReadAllIndicesCallback(
     chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
     const chip::app::Clusters::GroupKeyManagement::Commands::KeySetReadAllIndices::DecodableType & commandData)
 {
-    auto fabric     = commandObj->GetAccessingFabricIndex();
-    auto * provider = GetGroupDataProvider();
+    Credentials::GroupDataProvider * provider = nullptr;
+    const FabricInfo * fabric                 = nullptr;
 
-    if (nullptr == provider)
+    if (!GetProviderAndFabric(commandObj, commandPath, &provider, &fabric))
     {
-        commandObj->AddStatus(commandPath, Status::Failure);
+        // Command will already have status populated from validation.
         return true;
     }
 
-    auto keysIt = provider->IterateKeySets(fabric);
+    FabricIndex fabricIndex = fabric->GetFabricIndex();
+    auto keysIt             = provider->IterateKeySets(fabricIndex);
     if (nullptr == keysIt)
     {
-        commandObj->AddStatus(commandPath, Status::Failure);
+        commandObj->AddStatusAndLogIfFailure(commandPath, Status::Failure, "Failed iteration of key set indices!");
         return true;
     }
 

src/app/clusters/mode-base-server/mode-base-server.cpp

@@ -368,6 +368,11 @@ void Instance::HandleChangeToMode(HandlerContext & ctx, const Commands::ChangeTo
 
     Commands::ChangeToModeResponse::Type response;
 
+    // If the NewMode field doesn't match the Mode field of any entry of the SupportedModes list,
+    // the ChangeToModeResponse command's Status field SHALL indicate UnsupportedMode and
+    // the StatusText field SHALL be included and MAY be used to indicate the issue, with a human readable string,
+    // or include an empty string.
+    // We are leaving the StatusText empty since the Status is descriptive enough.
     if (!IsSupportedMode(newMode))
     {
         ChipLogError(Zcl, "ModeBase: Failed to find the option with mode %u", newMode);
@@ -376,6 +381,17 @@ void Instance::HandleChangeToMode(HandlerContext & ctx, const Commands::ChangeTo
         return;
     }
 
+    // If the NewMode field is the same as the value of the CurrentMode attribute
+    // the ChangeToModeResponse command SHALL have the Status field set to Success and
+    // the StatusText field MAY be supplied with a human readable string or include an empty string.
+    // We are leaving the StatusText empty since the Status is descriptive enough.
+    if (newMode == GetCurrentMode())
+    {
+        response.status = to_underlying(ModeBase::StatusCode::kSuccess);
+        ctx.mCommandHandler.AddResponse(ctx.mRequestPath, response);
+        return;
+    }
+
     mDelegate->HandleChangeToMode(newMode, response);
 
     if (response.status == to_underlying(StatusCode::kSuccess))

src/app/tests/suites/TestGroupKeyManagementCluster.yaml

@@ -556,6 +556,15 @@ tests:
       response:
           error: RESOURCE_EXHAUSTED
 
+    - label: "Try to remove KeySet index 0 should fail"
+      command: "KeySetRemove"
+      arguments:
+          values:
+              - name: "GroupKeySetID"
+                value: 0
+      response:
+          error: INVALID_COMMAND
+
     - label: "Write Group Keys (invalid)"
       command: "writeAttribute"
       attribute: "GroupKeyMap"

src/credentials/FabricTable.cpp

@@ -510,6 +510,11 @@ FabricInfo * FabricTable::GetMutableFabricByIndex(FabricIndex fabricIndex)
 
 const FabricInfo * FabricTable::FindFabricWithIndex(FabricIndex fabricIndex) const
 {
+    if (fabricIndex == kUndefinedFabricIndex)
+    {
+        return nullptr;
+    }
+
     // Try to match pending fabric first if available
     if (HasPendingFabricUpdate() && (mPendingFabric.GetFabricIndex() == fabricIndex))
     {

src/credentials/tests/TestFabricTable.cpp

@@ -2391,6 +2391,11 @@ void TestFabricLookup(nlTestSuite * inSuite, void * inContext)
         NL_TEST_ASSERT(inSuite, fabricInfo->GetFabricIndex() == 2);
         NL_TEST_ASSERT(inSuite, !fabricInfo->ShouldAdvertiseIdentity());
     }
+
+    // Attempt lookup of FabricIndex 0 --> should always fail.
+    {
+        NL_TEST_ASSERT(inSuite, fabricTable.FindFabricWithIndex(0) == nullptr);
+    }
 }
 
 void TestFetchCATs(nlTestSuite * inSuite, void * inContext)

src/platform/ESP32/ConnectivityManagerImpl.h

@@ -167,6 +167,12 @@ class ConnectivityManagerImpl final : public ConnectivityManager,
     void OnStationIPv6AddressAvailable(const ip_event_got_ip6_t & got_ip);
 #endif // CHIP_DEVICE_CONFIG_ENABLE_WIFI
 
+#if CHIP_DEVICE_CONFIG_ENABLE_ETHERNET
+    void OnEthernetIPv4AddressAvailable(const ip_event_got_ip_t & got_ip);
+    void OnEthernetIPv4AddressLost(void);
+    void OnEthernetIPv6AddressAvailable(const ip_event_got_ip6_t & got_ip);
+#endif // CHIP_DEVICE_CONFIG_ENABLE_ETHERNET
+
     // ===== Members for internal use by the following friends.
 
     friend ConnectivityManager & ConnectivityMgr(void);

src/platform/ESP32/ConnectivityManagerImpl_Ethernet.cpp

@@ -55,15 +55,52 @@ CHIP_ERROR ConnectivityManagerImpl::InitEthernet()
     return CHIP_NO_ERROR;
 }
 
+void ConnectivityManagerImpl::OnEthernetIPv4AddressAvailable(const ip_event_got_ip_t & got_ip)
+{
+    ChipLogProgress(DeviceLayer, "IPv4 address available on Ethernet interface: " IPSTR "/" IPSTR " gateway " IPSTR,
+                    IP2STR(&got_ip.ip_info.ip), IP2STR(&got_ip.ip_info.netmask), IP2STR(&got_ip.ip_info.gw));
+
+    ChipDeviceEvent event;
+    event.Type                           = DeviceEventType::kInterfaceIpAddressChanged;
+    event.InterfaceIpAddressChanged.Type = InterfaceIpChangeType::kIpV4_Assigned;
+    PlatformMgr().PostEventOrDie(&event);
+}
+
+void ConnectivityManagerImpl::OnEthernetIPv4AddressLost(void)
+{
+    ChipLogProgress(DeviceLayer, "IPv4 address lost on Ethernet interface");
+
+    ChipDeviceEvent event;
+    event.Type                           = DeviceEventType::kInterfaceIpAddressChanged;
+    event.InterfaceIpAddressChanged.Type = InterfaceIpChangeType::kIpV4_Lost;
+    PlatformMgr().PostEventOrDie(&event);
+}
+
+void ConnectivityManagerImpl::OnEthernetIPv6AddressAvailable(const ip_event_got_ip6_t & got_ip)
+{
+    ChipLogProgress(DeviceLayer, "IPv6 address available on Ethernet interface: " IPV6STR, IPV62STR(got_ip.ip6_info.ip));
+
+    ChipDeviceEvent event;
+    event.Type                           = DeviceEventType::kInterfaceIpAddressChanged;
+    event.InterfaceIpAddressChanged.Type = InterfaceIpChangeType::kIpV6_Assigned;
+    PlatformMgr().PostEventOrDie(&event);
+}
+
 void ConnectivityManagerImpl::OnEthernetPlatformEvent(const ChipDeviceEvent * event)
 {
     switch (event->Platform.ESPSystemEvent.Id)
     {
     case IP_EVENT_ETH_GOT_IP:
-        ChipLogProgress(DeviceLayer, "Ethernet Link Up");
+        OnEthernetIPv4AddressAvailable(event->Platform.ESPSystemEvent.Data.IpGotIp);
         break;
     case IP_EVENT_ETH_LOST_IP:
-        ChipLogProgress(DeviceLayer, "Ethernet Link Down");
+        OnEthernetIPv4AddressLost();
+        break;
+    case IP_EVENT_GOT_IP6:
+        if (strcmp(esp_netif_get_ifkey(event->Platform.ESPSystemEvent.Data.IpGotIp6.esp_netif), "ETH_DEF") == 0)
+        {
+            OnEthernetIPv6AddressAvailable(event->Platform.ESPSystemEvent.Data.IpGotIp6);
+        }
         break;
     case ETHERNET_EVENT_START:
         ChipLogProgress(DeviceLayer, "Ethernet Started");