Fix door lock endpoint initialization. (#34270)

* Fix door lock endpoint initialization. A few issues here: * DoorLockServer::InitServer was resetting endpoint state for all endpoints when initializing any endpoint. * We had two separate arrays of per-endpoint data, one for the endpoint context, one for the delegate. The fix is to not touch the state of other endpoints when initializing an endpoint and to put all the per-endpoint state in one place. * Apply suggestions from code review Co-authored-by: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com> --------- Co-authored-by: Nivi Sarkar <55898241+nivi-apple@users.noreply.github.com>
project-chip · Aug 15, 2024 · 2450464 · 2450464
1 parent aa6c001
commit 2450464
Show file tree

Hide file tree

Showing 2 changed files with 93 additions and 50 deletions.
diff --git a/src/app/clusters/door-lock-server/door-lock-server.cpp b/src/app/clusters/door-lock-server/door-lock-server.cpp
@@ -51,39 +51,6 @@ static constexpr uint8_t DOOR_LOCK_ALIRO_CREDENTIAL_SIZE = 65;
 
 static constexpr uint32_t DOOR_LOCK_MAX_LOCK_TIMEOUT_SEC = MAX_INT32U_VALUE / MILLISECOND_TICKS_PER_SECOND;
 
-static constexpr size_t kDoorLockDelegateTableSize =
-    MATTER_DM_DOOR_LOCK_CLUSTER_SERVER_ENDPOINT_COUNT + CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT;
-
-static_assert(kDoorLockDelegateTableSize <= kEmberInvalidEndpointIndex, "Door Lock Delegate table size error");
-
-namespace chip {
-namespace app {
-namespace Clusters {
-namespace DoorLock {
-
-Delegate * gDelegateTable[kDoorLockDelegateTableSize] = { nullptr };
-
-Delegate * GetDelegate(EndpointId endpoint)
-{
-    uint16_t ep = emberAfGetClusterServerEndpointIndex(endpoint, DoorLock::Id, MATTER_DM_DOOR_LOCK_CLUSTER_SERVER_ENDPOINT_COUNT);
-    return (ep >= kDoorLockDelegateTableSize ? nullptr : gDelegateTable[ep]);
-}
-
-void SetDefaultDelegate(EndpointId endpoint, Delegate * delegate)
-{
-    uint16_t ep = emberAfGetClusterServerEndpointIndex(endpoint, DoorLock::Id, MATTER_DM_DOOR_LOCK_CLUSTER_SERVER_ENDPOINT_COUNT);
-    // if endpoint is found
-    if (ep < ArraySize(gDelegateTable))
-    {
-        gDelegateTable[ep] = delegate;
-    }
-}
-
-} // namespace DoorLock
-} // namespace Clusters
-} // namespace app
-} // namespace chip
-
 DoorLockServer DoorLockServer::instance;
 
 class DoorLockClusterFabricDelegate : public chip::FabricTable::Delegate
@@ -117,7 +84,18 @@ DoorLockServer & DoorLockServer::Instance()
  *
  * @param endpointId
  */
-void DoorLockServer::InitServer(chip::EndpointId endpointId)
+void DoorLockServer::InitServer(EndpointId endpointId)
+{
+    CHIP_ERROR err = InitEndpoint(endpointId);
+
+    // We have no way to communicate this error, so just log it.
+    if (err != CHIP_NO_ERROR)
+    {
+        ChipLogError(Zcl, "Door Lock cluster initialization on endpoint %d failed: %" CHIP_ERROR_FORMAT, endpointId, err.Format());
+    }
+}
+
+CHIP_ERROR DoorLockServer::InitEndpoint(EndpointId endpointId, Delegate * delegate)
 {
     ChipLogProgress(Zcl, "Door Lock cluster initialized at endpoint #%u", endpointId);
 
@@ -128,11 +106,48 @@ void DoorLockServer::InitServer(chip::EndpointId endpointId)
     }
     SetActuatorEnabled(endpointId, true);
 
-    for (auto & ep : mEndpointCtx)
+    auto * endpointContext = getContext(endpointId);
+    if (!endpointContext)
+    {
+        ChipLogError(Zcl, "Invalid endpoint %d for initializing lock server: no endpoint context available", endpointId);
+        return CHIP_ERROR_INVALID_ARGUMENT;
+    }
+
+    endpointContext->lockoutEndTimestamp    = endpointContext->lockoutEndTimestamp.zero();
+    endpointContext->wrongCodeEntryAttempts = 0;
+    endpointContext->delegate               = delegate;
+    return CHIP_NO_ERROR;
+}
+
+void DoorLockServer::ShutdownEndpoint(EndpointId endpointId)
+{
+    auto * endpointContext = getContext(endpointId);
+    if (!endpointContext)
     {
-        ep.lockoutEndTimestamp    = ep.lockoutEndTimestamp.zero();
-        ep.wrongCodeEntryAttempts = 0;
+        ChipLogError(Zcl, "Invalid endpoint %d for shutting down lock server: no endpoint context available", endpointId);
+        return;
     }
+
+    endpointContext->delegate = nullptr;
+}
+
+CHIP_ERROR DoorLockServer::SetDelegate(chip::EndpointId endpointId, chip::app::Clusters::DoorLock::Delegate * delegate)
+{
+    if (!delegate)
+    {
+        ChipLogError(Zcl, "Trying to set a null DoorLock::Delegate on endpoint %d", endpointId);
+        return CHIP_ERROR_INVALID_ARGUMENT;
+    }
+
+    auto * endpointContext = getContext(endpointId);
+    if (!endpointContext)
+    {
+        ChipLogError(Zcl, "Invalid endpoint %d for setting a delegate: no endpoint context available", endpointId);
+        return CHIP_ERROR_INVALID_ARGUMENT;
+    }
+
+    endpointContext->delegate = delegate;
+    return CHIP_NO_ERROR;
 }
 
 bool DoorLockServer::SetLockState(chip::EndpointId endpointId, DlLockState newLockState)
@@ -3445,6 +3460,17 @@ EmberAfDoorLockEndpointContext * DoorLockServer::getContext(chip::EndpointId end
     return nullptr;
 }
 
+Delegate * DoorLockServer::GetDelegate(EndpointId endpointId)
+{
+    auto * endpointContext = getContext(endpointId);
+    if (!endpointContext)
+    {
+        return nullptr;
+    }
+
+    return endpointContext->delegate;
+}
+
 bool DoorLockServer::HandleRemoteLockOperation(chip::app::CommandHandler * commandObj,
                                                const chip::app::ConcreteCommandPath & commandPath, LockOperationTypeEnum opType,
                                                RemoteLockOpHandler opHandler, const Optional<ByteSpan> & pinCode)

diff --git a/src/app/clusters/door-lock-server/door-lock-server.h b/src/app/clusters/door-lock-server/door-lock-server.h
@@ -86,20 +86,9 @@ struct EmberAfDoorLockEndpointContext
 {
     chip::System::Clock::Timestamp lockoutEndTimestamp;
     int wrongCodeEntryAttempts;
+    chip::app::Clusters::DoorLock::Delegate * delegate = nullptr;
 };
 
-namespace chip {
-namespace app {
-namespace Clusters {
-namespace DoorLock {
-
-void SetDefaultDelegate(EndpointId endpoint, Delegate * delegate);
-
-} // namespace DoorLock
-} // namespace Clusters
-} // namespace app
-} // namespace chip
-
 /**
  * @brief Door Lock Server Plugin class.
  */
@@ -112,7 +101,28 @@ class DoorLockServer : public chip::app::AttributeAccessInterface
     using Feature                       = chip::app::Clusters::DoorLock::Feature;
     using OnFabricRemovedCustomCallback = void (*)(chip::EndpointId endpointId, chip::FabricIndex fabricIndex);
 
-    void InitServer(chip::EndpointId endpointId);
+    /**
+     * Multiple InitEndpoint calls can happen for different endpoints.  Calling
+     * InitEndpoint twice for the same endpoint requires a ShutdownEndpoint call
+     * for that endpoint in between.
+     *
+     * A DoorLock::Delegate is optional, but needs to be provided in either
+     * InitEndpoint or in a separate SetDelegate call for Aliro features, and
+     * possibly other new features, to work.
+     */
+    CHIP_ERROR InitEndpoint(chip::EndpointId endpointId, chip::app::Clusters::DoorLock::Delegate * delegate = nullptr);
+
+    void ShutdownEndpoint(chip::EndpointId endpointId);
+
+    // InitServer is a deprecated alias for InitEndpoint with no delegate.
+    void InitServer(chip::EndpointId endpointid);
+
+    /**
+     * Delegate is not supposed to be null. Removing a delegate
+     * should only happen when shutting down the door lock cluster on the
+     * endpoint, via ShutdownEndpoint.
+     */
+    CHIP_ERROR SetDelegate(chip::EndpointId endpointId, chip::app::Clusters::DoorLock::Delegate * delegate);
 
     /**
      * Updates the LockState attribute with new value and sends LockOperation event.
@@ -488,6 +498,13 @@ class DoorLockServer : public chip::app::AttributeAccessInterface
     static void sendClusterResponse(chip::app::CommandHandler * commandObj, const chip::app::ConcreteCommandPath & commandPath,
                                     chip::Protocols::InteractionModel::ClusterStatusCode status);
 
+    /**
+     * Get the DoorLock::Delegate for the given endpoint, if any. Will return
+     * null if there is no door lock server initialized on that endpoint or if
+     * there is no delegate associated with the initialized server.
+     */
+    chip::app::Clusters::DoorLock::Delegate * GetDelegate(chip::EndpointId endpointId);
+
     /**
      * @brief Common handler for LockDoor, UnlockDoor, UnlockWithTimeout commands
      *