Skip to content

Commit

Permalink
Apply suggestions from code review
Browse files Browse the repository at this point in the history 
* Avoid mixing code-backed and Attribute-store-backed attributes
* Avoid global singletons to maintain state
* Initialize the cluster from the application
* Report features based on code, not on ZAP-configured values

Signed-off-by: Oliver Fan <[email protected]>
  • Loading branch information
@OliverFan1
OliverFan1 committed Jul 17, 2024
1 parent c98dba9 commit 01b03ce
Show file tree
Hide file tree
Showing 4 changed files with 172 additions and 133 deletions.
61 changes: 61 additions & 0 deletions examples/all-clusters-app/all-clusters-common/src/occupancy-sensing-stub.cpp
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
/*
*
* Copyright (c) 2024 Project CHIP Authors
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

#include <app/clusters/occupancy-sensor-server/occupancy-sensor-server.h>
#include <app/clusters/occupancy-sensor-server/occupancy-hal.h>
#include <app/CommandHandler.h>
#include <platform/CHIPDeviceLayer.h>


using namespace chip;
using namespace chip::app::Clusters;
using namespace chip::app::Clusters::OccupancySensing;
using namespace chip::app::Clusters::OccupancySensing::Structs;
using namespace chip::DeviceLayer;

using chip::Protocols::InteractionModel::Status;

static std::unique_ptr<OccupancySensingAttrAccess> gAttrAccess[MATTER_DM_OCCUPANCY_SENSING_CLUSTER_SERVER_ENDPOINT_COUNT + CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT];


void emberAfOccupancySensingClusterInitCallback(chip::EndpointId endpointId)
{

VerifyOrDie(!gAttrAccess[endpointId]);

gAttrAccess[endpointId] = std::make_unique<OccupancySensingAttrAccess>(
BitMask<OccupancySensing::Feature, uint32_t>(OccupancySensing::Feature::kOther));


OccupancySensing::Structs::HoldTimeLimitsStruct::Type holdTimeLimits = {
.holdTimeMin = 1,
.holdTimeMax = 300,
.holdTimeDefault = 10,
};

uint16_t holdTime = 10;

if (gAttrAccess[endpointId])
{
gAttrAccess[endpointId]->Init();

SetHoldTimeLimits(endpointId, holdTimeLimits);

SetHoldTime(endpointId, holdTime);
}

}
1 change: 1 addition & 0 deletions examples/all-clusters-app/linux/BUILD.gn
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ source_set("chip-all-clusters-common") {
"${chip_root}/examples/all-clusters-app/all-clusters-common/src/laundry-washer-controls-delegate-impl.cpp",
"${chip_root}/examples/all-clusters-app/all-clusters-common/src/laundry-washer-mode.cpp",
"${chip_root}/examples/all-clusters-app/all-clusters-common/src/microwave-oven-mode.cpp",
"${chip_root}/examples/all-clusters-app/all-clusters-common/src/occupancy-sensing-stub.cpp",
"${chip_root}/examples/all-clusters-app/all-clusters-common/src/operational-state-delegate-impl.cpp",
"${chip_root}/examples/all-clusters-app/all-clusters-common/src/oven-modes.cpp",
"${chip_root}/examples/all-clusters-app/all-clusters-common/src/oven-operational-state-delegate.cpp",
Expand Down
175 changes: 94 additions & 81 deletions src/app/clusters/occupancy-sensor-server/occupancy-sensor-server.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
#include <app/AttributeAccessInterfaceRegistry.h>
#include <app/EventLogging.h>
#include <app/data-model/Encode.h>
#include <app/reporting/reporting.h>
#include <app/util/attribute-storage.h>
#include <lib/core/CHIPError.h>

Expand All @@ -31,117 +32,133 @@ namespace app {
namespace Clusters {
namespace OccupancySensing {

Structs::HoldTimeLimitsStruct::Type * HoldTimeLimitsManager::HoldTimeLimits::GetHoldTimeLimitsStruct(EndpointId endpoint)
{
size_t endpointIndex = 0;
Structs::HoldTimeLimitsStruct::Type * holdTimeLimitsStruct = nullptr;
CHIP_ERROR status = FindHoldTimeLimitsIndex(endpoint, endpointIndex);
if (CHIP_NO_ERROR == status)
{
holdTimeLimitsStruct = &mHoldTimeLimitsStructs[endpointIndex];
}
return holdTimeLimitsStruct;
}

CHIP_ERROR
HoldTimeLimitsManager::HoldTimeLimits::SetHoldTimeLimitsStruct(EndpointId endpoint,
Structs::HoldTimeLimitsStruct::Type & holdTimeLimitsStruct)
{
VerifyOrReturnError(kInvalidEndpointId != endpoint, CHIP_ERROR_INVALID_ARGUMENT);

size_t endpointIndex = 0;
ReturnErrorOnFailure(FindHoldTimeLimitsIndex(endpoint, endpointIndex));
Structs::HoldTimeLimitsStruct::Type mHoldTimeLimitsStructs[MATTER_DM_OCCUPANCY_SENSING_CLUSTER_SERVER_ENDPOINT_COUNT + CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT];

mHoldTimeLimitsStructs[endpointIndex] = holdTimeLimitsStruct;
uint16_t mHoldTime[MATTER_DM_OCCUPANCY_SENSING_CLUSTER_SERVER_ENDPOINT_COUNT + CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT];

CHIP_ERROR OccupancySensingAttrAccess::Init()
{
VerifyOrReturnError(registerAttributeAccessOverride(this), CHIP_ERROR_INCORRECT_STATE);
return CHIP_NO_ERROR;
}

/// @brief Returns the index of the HoldTimeLimits associated to an endpoint
/// @param[in] endpoint target endpoint
/// @param[out] endpointIndex index of the corresponding HoldTimeLimits for an endpoint
/// @return CHIP_NO_ERROR or CHIP_ERROR_NOT_FOUND, CHIP_ERROR_INVALID_ARGUMENT if invalid endpoint
CHIP_ERROR HoldTimeLimitsManager::HoldTimeLimits::FindHoldTimeLimitsIndex(EndpointId endpoint, size_t & endpointIndex)
void OccupancySensingAttrAccess::Shutdown()
{
VerifyOrReturnError(kInvalidEndpointId != endpoint, CHIP_ERROR_INVALID_ARGUMENT);
unregisterAttributeAccessOverride(this);
}

uint16_t index = emberAfGetClusterServerEndpointIndex(endpoint, OccupancySensing::Id,
MATTER_DM_OCCUPANCY_SENSING_CLUSTER_SERVER_ENDPOINT_COUNT);

if (index < ArraySize(mHoldTimeLimitsStructs))
CHIP_ERROR OccupancySensingAttrAccess::Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder)
{
VerifyOrDie(aPath.mClusterId == app::Clusters::OccupancySensing::Id);

switch (aPath.mAttributeId)
{
endpointIndex = index;
case Attributes::FeatureMap::Id:
ReturnErrorOnFailure(aEncoder.Encode(mFeature));
break;
case Attributes::HoldTime::Id: {

uint16_t * holdTime = GetHoldTimeForEndpoint(aPath.mEndpointId);

if (holdTime == nullptr)
{
return CHIP_ERROR_NOT_FOUND;
}

return aEncoder.Encode(*holdTime);
}
case Attributes::HoldTimeLimits::Id: {

Structs::HoldTimeLimitsStruct::Type * holdTimeLimitsStruct = GetHoldTimeLimitsForEndpoint(aPath.mEndpointId);

if (holdTimeLimitsStruct == nullptr)
{
return CHIP_ERROR_NOT_FOUND;
}

return aEncoder.Encode(*holdTimeLimitsStruct);
}
default:
return CHIP_NO_ERROR;
}
return CHIP_ERROR_NOT_FOUND;

return CHIP_NO_ERROR;
}

HoldTimeLimitsManager HoldTimeLimitsManager::mInstance;

HoldTimeLimitsManager & HoldTimeLimitsManager::Instance()
bool OccupancySensingAttrAccess::HasFeature(Feature aFeature) const
{
return mInstance;
return mFeature.Has(aFeature);
}

CHIP_ERROR HoldTimeLimitsManager::Init()
Structs::HoldTimeLimitsStruct::Type * GetHoldTimeLimitsForEndpoint(EndpointId endpoint)
{
// Prevents re-initializing
VerifyOrReturnError(!mIsInitialized, CHIP_ERROR_INCORRECT_STATE);
auto index = emberAfGetClusterServerEndpointIndex(endpoint, app::Clusters::OccupancySensing::Id,
MATTER_DM_OCCUPANCY_SENSING_CLUSTER_SERVER_ENDPOINT_COUNT);

if (index == kEmberInvalidEndpointIndex)
{
return nullptr;
}

for (size_t i = 0; i <= kOccupancySensingServerMaxEndpointCount; i++)
if (index >= ArraySize(mHoldTimeLimitsStructs))
{
if (emberAfContainsServer(EndpointId(i), OccupancySensing::Id))
{
Structs::HoldTimeLimitsStruct::Type holdTimeLimitsInit;
ChipLogError(NotSpecified, "Internal error: invalid/unexpected hold time limits index.");
return nullptr;
}
return &mHoldTimeLimitsStructs[index];
}

// Set up some sane initial values for hold time limits structures
holdTimeLimitsInit.holdTimeMin = 1;
holdTimeLimitsInit.holdTimeMax = 300;
holdTimeLimitsInit.holdTimeDefault = 10;
CHIP_ERROR SetHoldTimeLimits(EndpointId endpointId, const Structs::HoldTimeLimitsStruct::Type & holdTimeLimits)
{

HoldTimeLimitsManager::Instance().SetHoldTimeLimitsStruct(EndpointId(i), holdTimeLimitsInit);
}
}
VerifyOrReturnError(kInvalidEndpointId != endpointId, CHIP_ERROR_INVALID_ARGUMENT);

VerifyOrReturnError(registerAttributeAccessOverride(this), CHIP_ERROR_INCORRECT_STATE);
Structs::HoldTimeLimitsStruct::Type * holdTimeLimitsForEndpoint = GetHoldTimeLimitsForEndpoint(endpointId);
VerifyOrReturnError(holdTimeLimitsForEndpoint != nullptr, CHIP_ERROR_INVALID_ARGUMENT);

holdTimeLimitsForEndpoint->holdTimeMin = holdTimeLimits.holdTimeMin;
holdTimeLimitsForEndpoint->holdTimeMax = holdTimeLimits.holdTimeMax;
holdTimeLimitsForEndpoint->holdTimeDefault = holdTimeLimits.holdTimeDefault;

MatterReportingAttributeChangeCallback(endpointId, OccupancySensing::Id, Attributes::HoldTimeLimits::Id);

mIsInitialized = true;
return CHIP_NO_ERROR;
}

// AttributeAccessInterface
CHIP_ERROR HoldTimeLimitsManager::Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder)
uint16_t * GetHoldTimeForEndpoint(EndpointId endpoint)
{
switch (aPath.mAttributeId)
auto index = emberAfGetClusterServerEndpointIndex(endpoint, app::Clusters::OccupancySensing::Id,
MATTER_DM_OCCUPANCY_SENSING_CLUSTER_SERVER_ENDPOINT_COUNT);

if (index == kEmberInvalidEndpointIndex)
{
case Attributes::HoldTimeLimits::Id: {

Structs::HoldTimeLimitsStruct::Type * holdTimeLimitsStruct =
HoldTimeLimitsManager::Instance().mHoldTimeLimits.GetHoldTimeLimitsStruct(aPath.mEndpointId);
Structs::HoldTimeLimitsStruct::Type res;
res.holdTimeMin = holdTimeLimitsStruct->holdTimeMin;
res.holdTimeMax = holdTimeLimitsStruct->holdTimeMax;
res.holdTimeDefault = holdTimeLimitsStruct->holdTimeDefault;
return aEncoder.Encode(res);
return nullptr;
}
default:
return CHIP_NO_ERROR;

if (index >= ArraySize(mHoldTimeLimitsStructs))
{
ChipLogError(NotSpecified, "Internal error: invalid/unexpected hold time index.");
return nullptr;
}
return &mHoldTime[index];
}

Structs::HoldTimeLimitsStruct::Type * HoldTimeLimitsManager::GetHoldTimeLimitsStruct(EndpointId endpoint)
CHIP_ERROR SetHoldTime(EndpointId endpointId, const uint16_t & holdTime)
{
Structs::HoldTimeLimitsStruct::Type * holdTimeLimitsStruct = mHoldTimeLimits.GetHoldTimeLimitsStruct(endpoint);
return holdTimeLimitsStruct;
}
VerifyOrReturnError(kInvalidEndpointId != endpointId, CHIP_ERROR_INVALID_ARGUMENT);

uint16_t * holdTimeForEndpoint = GetHoldTimeForEndpoint(endpointId);
VerifyOrReturnError(holdTimeForEndpoint != nullptr, CHIP_ERROR_INVALID_ARGUMENT);

*holdTimeForEndpoint = holdTime;

MatterReportingAttributeChangeCallback(endpointId, OccupancySensing::Id, Attributes::HoldTime::Id);

CHIP_ERROR HoldTimeLimitsManager::SetHoldTimeLimitsStruct(EndpointId endpoint,
Structs::HoldTimeLimitsStruct::Type & holdTimeLimitsStruct)
{
ReturnErrorOnFailure(mHoldTimeLimits.SetHoldTimeLimitsStruct(endpoint, holdTimeLimitsStruct));
return CHIP_NO_ERROR;
}


} // namespace OccupancySensing
} // namespace Clusters
} // namespace app
Expand Down Expand Up @@ -213,11 +230,7 @@ HalOccupancySensorType __attribute__((weak)) halOccupancyGetSensorType(EndpointI
return HAL_OCCUPANCY_SENSOR_TYPE_PIR;
}


void MatterOccupancySensingPluginServerInitCallback()
{
CHIP_ERROR err = HoldTimeLimitsManager::Instance().Init();
if (err != CHIP_NO_ERROR)
{
ChipLogError(Zcl, "HoldTimeLimitsManager::Instance().Init() error: %" CHIP_ERROR_FORMAT, err.Format());
}
}
{}

68 changes: 16 additions & 52 deletions src/app/clusters/occupancy-sensor-server/occupancy-sensor-server.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,73 +30,37 @@ namespace app {
namespace Clusters {
namespace OccupancySensing {

/**
* Interface to help manage the Hold Time Limits of the Occupancy Sensing Cluster.
*/
class HoldTimeLimitsManager : public AttributeAccessInterface

class OccupancySensingAttrAccess : public AttributeAccessInterface
{
public:
static constexpr size_t kOccupancySensingServerMaxEndpointCount =
MATTER_DM_OCCUPANCY_SENSING_CLUSTER_SERVER_ENDPOINT_COUNT + CHIP_DEVICE_CONFIG_DYNAMIC_ENDPOINT_COUNT;
static_assert(kOccupancySensingServerMaxEndpointCount <= kInvalidEndpointId, "Occupancy Sensing endpoint count error");

// HoldTimeLimits
class HoldTimeLimits
{
public:
Structs::HoldTimeLimitsStruct::Type * GetHoldTimeLimitsStruct(EndpointId endpoint);
CHIP_ERROR SetHoldTimeLimitsStruct(EndpointId endpoint, Structs::HoldTimeLimitsStruct::Type & holdTimeLimitsStruct);
OccupancySensingAttrAccess(BitMask<Feature> aFeature) :
app::AttributeAccessInterface(Optional<EndpointId>::Missing(), app::Clusters::OccupancySensing::Id), mFeature(aFeature)
{}

~OccupancySensingAttrAccess() { Shutdown(); }

private:
/// @brief Returns the index of the HoldTimeLimits associated to an endpoint
/// @param[in] endpoint target endpoint
/// @param[out] endpointIndex index of the corresponding HoldTimeLimits for an endpoint
/// @return CHIP_NO_ERROR or CHIP_ERROR_NOT_FOUND, CHIP_ERROR_INVALID_ARGUMENT if invalid endpoint
CHIP_ERROR FindHoldTimeLimitsIndex(EndpointId endpoint, size_t & endpointIndex);

Structs::HoldTimeLimitsStruct::Type mHoldTimeLimitsStructs[kOccupancySensingServerMaxEndpointCount];
};

static HoldTimeLimitsManager & Instance();

CHIP_ERROR Init();
void Shutdown();

// AttributeAccessInterface
CHIP_ERROR Read(const ConcreteReadAttributePath & aPath, AttributeValueEncoder & aEncoder) override;

// HoldTimeLimitsStruct Accessors
Structs::HoldTimeLimitsStruct::Type * GetHoldTimeLimitsStruct(EndpointId endpoint);
CHIP_ERROR SetHoldTimeLimitsStruct(EndpointId endpoint, Structs::HoldTimeLimitsStruct::Type & holdTimeLimitsStruct);
bool HasFeature(Feature aFeature) const;

private:
HoldTimeLimitsManager() : AttributeAccessInterface(Optional<EndpointId>(), Id) {}
~HoldTimeLimitsManager() {}
BitMask<Feature> mFeature;

};

bool mIsInitialized = false;
CHIP_ERROR SetHoldTimeLimits(EndpointId endpointId, const Structs::HoldTimeLimitsStruct::Type & holdTimeLimits);

// HoldTimeLimits
HoldTimeLimits mHoldTimeLimits;
CHIP_ERROR SetHoldTime(EndpointId endpointId, const uint16_t & holdTime);

// Instance
static HoldTimeLimitsManager mInstance;
};
Structs::HoldTimeLimitsStruct::Type * GetHoldTimeLimitsForEndpoint(EndpointId endpoint);

inline bool HasFeature(EndpointId ep, Feature feature)
{
uint32_t map;
bool success = (Attributes::FeatureMap::Get(ep, &map) == Protocols::InteractionModel::Status::Success);
return success ? (map & to_underlying(feature)) : false;
}
uint16_t * GetHoldTimeForEndpoint(EndpointId endpoint);

/** @brief Occupancy Cluster Server Post Init
*
* Following resolution of the Occupancy state at startup for this endpoint,
* perform any additional initialization needed; e.g., synchronize hardware
* state.
*
* @param endpoint Endpoint that is being initialized Ver.: always
*/
void emberAfPluginOccupancyClusterServerPostInitCallback(chip::EndpointId endpoint);

} // namespace OccupancySensing
} // namespace Clusters
Expand Down

0 comments on commit 01b03ce

Please sign in to comment.