Skip to content

Commit

Permalink
Stop using weak functions for Aliro command callbacks. (#33931)
Browse files Browse the repository at this point in the history 
We have a delegate; we should just make use of the delegate.  This is not a
breaking change because nothing implements Aliro bits yet.

Also:

* Fixes some incorrect early returns that did not respond to commands.
* Fixes some validity checks that were not quite correct per spec.
* Fixes some logging levels for errors.
* Adds missing "mark these attributes dirty" bits.
* Marks various functions that don't touch our members as static.
  • Loading branch information
@bzbarsky-apple @pull
bzbarsky-apple authored and pull[bot] committed Oct 7, 2024
1 parent 5ffcdfa commit a58741d
Show file tree
Hide file tree
Showing 4 changed files with 108 additions and 75 deletions.
18 changes: 18 additions & 0 deletions src/app/clusters/door-lock-server/door-lock-delegate.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,24 @@ class Delegate
* @return The max number of Aliro endpoint keys supported.
*/
virtual uint16_t GetNumberOfAliroEndpointKeysSupported() = 0;

/**
* Set the Aliro reader configuration for the lock. The various arguments
* have already been checked for constraints and consistency with the
* FeatureMap.
*
* @param[in] signingKey Signing key component of the Reader's key pair.
* @param[in] verificationKey Verification key component of the Reader's key pair.
* @param[in] groupIdentifier Reader group identifier for the lock.
* @param[in] groupResolvingKey Group resolving key for the lock if Aliro BLE UWB feature is supported
*/
virtual CHIP_ERROR SetAliroReaderConfig(const ByteSpan & signingKey, const ByteSpan & verificationKey,
const ByteSpan & groupIdentifier, const Optional<ByteSpan> & groupResolvingKey) = 0;

/**
* Clear the Aliro reader configuration for the lock.
*/
virtual CHIP_ERROR ClearAliroReaderConfig() = 0;
};

} // namespace DoorLock
Expand Down
12 changes: 0 additions & 12 deletions src/app/clusters/door-lock-server/door-lock-server-callback.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -242,15 +242,3 @@ emberAfPluginDoorLockGetFaceCredentialLengthConstraints(chip::EndpointId endpoin
{
return false;
}

bool __attribute__((weak))
emberAfPluginDoorLockSetAliroReaderConfig(EndpointId endpointId, const ByteSpan & signingKey, const ByteSpan & verificationKey,
const ByteSpan & groupIdentifier, const Optional<ByteSpan> groupResolvingKey)
{
return false;
}

bool __attribute__((weak)) emberAfPluginDoorLockClearAliroReaderConfig(chip::EndpointId endpointId)
{
return false;
}
84 changes: 67 additions & 17 deletions src/app/clusters/door-lock-server/door-lock-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ class DoorLockClusterFabricDelegate : public chip::FabricTable::Delegate
{
void OnFabricRemoved(const FabricTable & fabricTable, FabricIndex fabricIndex) override
{
for (auto endpointId : EnabledEndpointsWithServerCluster(chip::app::Clusters::DoorLock::Id))
for (auto endpointId : EnabledEndpointsWithServerCluster(Clusters::DoorLock::Id))
{
if (!DoorLockServer::Instance().OnFabricRemoved(endpointId, fabricIndex))
{
Expand Down Expand Up @@ -3641,7 +3641,7 @@ void DoorLockServer::SendEvent(chip::EndpointId endpointId, T & event)

template <typename T>
bool DoorLockServer::GetAttribute(chip::EndpointId endpointId, chip::AttributeId attributeId,
Status (*getFn)(chip::EndpointId endpointId, T * value), T & value) const
Status (*getFn)(chip::EndpointId endpointId, T * value), T & value)
{
Status status = getFn(endpointId, &value);
bool success = (Status::Success == status);
Expand Down Expand Up @@ -3927,12 +3927,26 @@ void DoorLockServer::setAliroReaderConfigCommandHandler(CommandHandler * command
}

Delegate * delegate = GetDelegate(endpointID);
VerifyOrReturn(delegate != nullptr, ChipLogError(Zcl, "Delegate is null"));
if (!delegate)
{
ChipLogError(Zcl, "Door Lock delegate is null");
commandObj->AddStatus(commandPath, Status::Failure);
return;
}

// If Aliro BLE UWB feature is supported and groupResolvingKey is not provided in the command, return INVALID_COMMAND.
if (SupportsAliroBLEUWB(endpointID) && !groupResolvingKey.HasValue())
// The GroupResolvingKey must be provided if and only if the Aliro BLE UWB
// feature is supported. Otherwise, return INVALID_COMMAND
const bool supportsAliroBLEUWB = SupportsAliroBLEUWB(endpointID);
if (supportsAliroBLEUWB != groupResolvingKey.HasValue())
{
ChipLogProgress(Zcl, "[SetAliroReaderConfig] Aliro BLE UWB supported but Group Resolving Key is not provided");
if (supportsAliroBLEUWB)
{
ChipLogError(Zcl, "[SetAliroReaderConfig] Aliro BLE UWB supported but Group Resolving Key is not provided");
}
else
{
ChipLogError(Zcl, "[SetAliroReaderConfig] Aliro BLE UWB not supported but Group Resolving Key is provided");
}
commandObj->AddStatus(commandPath, Status::InvalidCommand);
return;
}
Expand All @@ -3959,19 +3973,27 @@ void DoorLockServer::setAliroReaderConfigCommandHandler(CommandHandler * command
// INVALID_IN_STATE.
if (err != CHIP_NO_ERROR || !readerVerificationKey.empty())
{
ChipLogProgress(
Zcl, "[SetAliroReaderConfig] Aliro reader verification key was not read or is not null. Return INVALID_IN_STATE");
ChipLogError(Zcl, "[SetAliroReaderConfig] Aliro reader verification key was not read or is not null.");
commandObj->AddStatus(commandPath, Status::InvalidInState);
return;
}

Status status = Status::Success;
if (!emberAfPluginDoorLockSetAliroReaderConfig(endpointID, signingKey, verificationKey, groupIdentifier, groupResolvingKey))
err = delegate->SetAliroReaderConfig(signingKey, verificationKey, groupIdentifier, groupResolvingKey);
if (err != CHIP_NO_ERROR)
{
ChipLogProgress(Zcl, "[SetAliroReaderConfig] Unable to set aliro reader config [endpointId=%d]", endpointID);
status = Status::Failure;
}
sendClusterResponse(commandObj, commandPath, ClusterStatusCode(status));
else
{
// Various attributes changed; mark them dirty.
MatterReportingAttributeChangeCallback(endpointID, Clusters::DoorLock::Id, AliroReaderVerificationKey::Id);
MatterReportingAttributeChangeCallback(endpointID, Clusters::DoorLock::Id, AliroReaderGroupIdentifier::Id);
if (supportsAliroBLEUWB)
{
MatterReportingAttributeChangeCallback(endpointID, Clusters::DoorLock::Id, AliroGroupResolvingKey::Id);
}
}
sendClusterResponse(commandObj, commandPath, ClusterStatusCode(StatusIB(err).mStatus));
}

void DoorLockServer::clearAliroReaderConfigCommandHandler(CommandHandler * commandObj, const ConcreteCommandPath & commandPath)
Expand All @@ -3987,13 +4009,41 @@ void DoorLockServer::clearAliroReaderConfigCommandHandler(CommandHandler * comma
return;
}

Status status = Status::Success;
if (!emberAfPluginDoorLockClearAliroReaderConfig(endpointID))
Delegate * delegate = GetDelegate(endpointID);
if (!delegate)
{
ChipLogProgress(Zcl, "[SetAliroReaderConfig] Unable to set aliro reader config [endpointId=%d]", endpointID);
status = Status::Failure;
ChipLogError(Zcl, "Door Lock delegate is null");
commandObj->AddStatus(commandPath, Status::Failure);
return;
}

uint8_t buffer[kAliroReaderVerificationKeySize];
MutableByteSpan readerVerificationKey(buffer);
CHIP_ERROR err = delegate->GetAliroReaderVerificationKey(readerVerificationKey);
if (err != CHIP_NO_ERROR && readerVerificationKey.empty())
{
// No reader config to start with. Just return without marking any
// attributes as dirty.
commandObj->AddStatus(commandPath, Status::Success);
return;
}

err = delegate->ClearAliroReaderConfig();
if (err != CHIP_NO_ERROR)
{
ChipLogError(Zcl, "[SetAliroReaderConfig] Unable to set aliro reader config [endpointId=%d]", endpointID);
}
else
{
// Various attributes changed; mark them dirty.
MatterReportingAttributeChangeCallback(endpointID, Clusters::DoorLock::Id, AliroReaderVerificationKey::Id);
MatterReportingAttributeChangeCallback(endpointID, Clusters::DoorLock::Id, AliroReaderGroupIdentifier::Id);
if (SupportsAliroBLEUWB(endpointID))
{
MatterReportingAttributeChangeCallback(endpointID, Clusters::DoorLock::Id, AliroGroupResolvingKey::Id);
}
}
sendClusterResponse(commandObj, commandPath, ClusterStatusCode(status));
sendClusterResponse(commandObj, commandPath, ClusterStatusCode(StatusIB(err).mStatus));
}

// =============================================================================
Expand Down
69 changes: 23 additions & 46 deletions src/app/clusters/door-lock-server/door-lock-server.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
#include <app/AttributeAccessInterface.h>
#include <app/CommandHandler.h>
#include <app/ConcreteCommandPath.h>
#include <app/reporting/reporting.h>
#include <app/util/attribute-storage.h>
#include <app/util/config.h>
#include <platform/CHIPDeviceConfig.h>
Expand Down Expand Up @@ -174,57 +175,60 @@ class DoorLockServer : public chip::app::AttributeAccessInterface

bool SendLockAlarmEvent(chip::EndpointId endpointId, AlarmCodeEnum alarmCode);

chip::BitFlags<Feature> GetFeatures(chip::EndpointId endpointId);
static chip::BitFlags<Feature> GetFeatures(chip::EndpointId endpointId);

inline bool SupportsPIN(chip::EndpointId endpointId) { return GetFeatures(endpointId).Has(Feature::kPinCredential); }
static inline bool SupportsPIN(chip::EndpointId endpointId) { return GetFeatures(endpointId).Has(Feature::kPinCredential); }

inline bool SupportsRFID(chip::EndpointId endpointId) { return GetFeatures(endpointId).Has(Feature::kRfidCredential); }
static inline bool SupportsRFID(chip::EndpointId endpointId) { return GetFeatures(endpointId).Has(Feature::kRfidCredential); }

inline bool SupportsFingers(chip::EndpointId endpointId) { return GetFeatures(endpointId).Has(Feature::kFingerCredentials); }
static inline bool SupportsFingers(chip::EndpointId endpointId)
{
return GetFeatures(endpointId).Has(Feature::kFingerCredentials);
}

inline bool SupportsFace(chip::EndpointId endpointId) { return GetFeatures(endpointId).Has(Feature::kFaceCredentials); }
static inline bool SupportsFace(chip::EndpointId endpointId) { return GetFeatures(endpointId).Has(Feature::kFaceCredentials); }

inline bool SupportsWeekDaySchedules(chip::EndpointId endpointId)
static inline bool SupportsWeekDaySchedules(chip::EndpointId endpointId)
{
return GetFeatures(endpointId).Has(Feature::kWeekDayAccessSchedules);
}

inline bool SupportsYearDaySchedules(chip::EndpointId endpointId)
static inline bool SupportsYearDaySchedules(chip::EndpointId endpointId)
{
return GetFeatures(endpointId).Has(Feature::kYearDayAccessSchedules);
}

inline bool SupportsHolidaySchedules(chip::EndpointId endpointId)
static inline bool SupportsHolidaySchedules(chip::EndpointId endpointId)
{
return GetFeatures(endpointId).Has(Feature::kHolidaySchedules);
}

inline bool SupportsAnyCredential(chip::EndpointId endpointId)
static inline bool SupportsAnyCredential(chip::EndpointId endpointId)
{
return GetFeatures(endpointId)
.HasAny(Feature::kPinCredential, Feature::kRfidCredential, Feature::kFingerCredentials, Feature::kFaceCredentials,
Feature::kAliroProvisioning);
}

inline bool SupportsCredentialsOTA(chip::EndpointId endpointId)
static inline bool SupportsCredentialsOTA(chip::EndpointId endpointId)
{
return GetFeatures(endpointId).Has(Feature::kCredentialsOverTheAirAccess);
}

inline bool SupportsUSR(chip::EndpointId endpointId)
static inline bool SupportsUSR(chip::EndpointId endpointId)
{
// appclusters, 5.2.2: USR feature has conformance [PIN | RID | FGP | FACE]
return GetFeatures(endpointId).Has(Feature::kUser) && SupportsAnyCredential(endpointId);
}

inline bool SupportsUnbolt(chip::EndpointId endpointId) { return GetFeatures(endpointId).Has(Feature::kUnbolt); }
static bool SupportsUnbolt(chip::EndpointId endpointId) { return GetFeatures(endpointId).Has(Feature::kUnbolt); }

/**
* @brief Checks if Aliro Provisioning feature is supported on the given endpoint
*
* @param endpointId endpointId ID of the endpoint which contains the lock.
*/
inline bool SupportsAliroProvisioning(chip::EndpointId endpointId)
static inline bool SupportsAliroProvisioning(chip::EndpointId endpointId)
{
return GetFeatures(endpointId).Has(Feature::kAliroProvisioning);
}
Expand All @@ -234,7 +238,10 @@ class DoorLockServer : public chip::app::AttributeAccessInterface
*
* @param endpointId endpointId ID of the endpoint which contains the lock.
*/
inline bool SupportsAliroBLEUWB(chip::EndpointId endpointId) { return GetFeatures(endpointId).Has(Feature::kAliroBLEUWB); }
static inline bool SupportsAliroBLEUWB(chip::EndpointId endpointId)
{
return GetFeatures(endpointId).Has(Feature::kAliroBLEUWB);
}

/**
* @brief Allows the application to register a custom callback which will be called after the default DoorLock
Expand Down Expand Up @@ -545,8 +552,8 @@ class DoorLockServer : public chip::app::AttributeAccessInterface
* @return false if attribute reading failed (value is kept unchanged)
*/
template <typename T>
bool GetAttribute(chip::EndpointId endpointId, chip::AttributeId attributeId,
chip::Protocols::InteractionModel::Status (*getFn)(chip::EndpointId endpointId, T * value), T & value) const;
static bool GetAttribute(chip::EndpointId endpointId, chip::AttributeId attributeId,
chip::Protocols::InteractionModel::Status (*getFn)(chip::EndpointId endpointId, T * value), T & value);

/**
* @brief Set generic attribute value
Expand Down Expand Up @@ -1273,33 +1280,3 @@ bool emberAfPluginDoorLockGetFingerVeinCredentialLengthConstraints(chip::Endpoin
* @return false on failure, true on success.
*/
bool emberAfPluginDoorLockGetFaceCredentialLengthConstraints(chip::EndpointId endpointId, uint8_t & minLen, uint8_t & maxLen);

/**
* @brief This callback is called when Door Lock cluster needs to communicate the Aliro reader configuration to the door lock.
*
* @note This function is used for communicating the Aliro signing key, verification key, group identifier and group resolving key
* to the lock.
*
* @param endpointId ID of the endpoint which contains the lock.
* @param[in] signingKey Signing key component of the Reader's key pair.
* @param[in] verificationKey Verification key component of the Reader's key pair.
* @param[in] groupIdentifier Reader group identifier for the lock.
* @param[in] groupResolvingKey Group resolving key for the lock if Aliro BLE UWB feature is supported
*
* @retval true, if the Aliro reader config was successfully communicated to the door lock.
* @retval false, if error occurred while communicating the Aliro reader config.
*/
bool emberAfPluginDoorLockSetAliroReaderConfig(chip::EndpointId endpointId, const chip::ByteSpan & signingKey,
const chip::ByteSpan & verificationKey, const chip::ByteSpan & groupIdentifier,
const Optional<chip::ByteSpan> & groupResolvingKey);

/**
* @brief This callback is called when Door Lock cluster needs to clear an existing Aliro reader configuration from the door lock.
*
*
* @param endpointId ID of the endpoint which contains the lock.
*
* @retval true, if the Aliro reader config was successfully cleared from the door lock.
* @retval false, if error occurred while clearing the Aliro reader config.
*/
bool emberAfPluginDoorLockClearAliroReaderConfig(chip::EndpointId endpointId);

0 comments on commit a58741d

Please sign in to comment.