Skip to content

Commit

Permalink
Looking to remove some redundant and unused functions from the ember …
Browse files Browse the repository at this point in the history 
…attribute storage/table (#31613)

* Remove some redundant functions

* Update include

* Fix the include yet again. Do not rely on af.h

* Remove argument support for the argument nobody uses

* Another restyle

* Drop the just test argument to emAfWriteAttribute

* Remove emberAfAllowNetworkWriteAttributeCallback

* Restyle

* Add missing semicolon

* Restyle

* updated based on review comments

* Update include file

* restyle

* Fix af.h placement in includes, drop EZSP_HOST

* Move back emberAfWriteAttributeExternal, but rename as internal

* Fix typos in comment

* Add back function declare position

* restyle

---------

Co-authored-by: Andrei Litvin <[email protected]>
  • Loading branch information
@andy31415 @andreilitvin
andy31415 and andreilitvin authored Jan 30, 2024
1 parent 783c3e0 commit f5ca5ad
Show file tree
Hide file tree
Showing 9 changed files with 100 additions and 194 deletions.
10 changes: 4 additions & 6 deletions examples/all-clusters-app/nxp/mw320/main.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1152,12 +1152,10 @@ void task_test_main(void * param)
PRINTF("--> update CurrentPosition [%d] \r\n", value);
Clusters::Switch::Attributes::CurrentPosition::Set(1, value);
#ifdef SUPPORT_MANUAL_CTRL
#error \
"This code thinks it's setting the OnOff attribute, but it's actually setting the NumberOfPositions attribute! And passing the wrong size for either case. Figure out what it's trying to do."
// sync-up the Light attribute (for test event, OO.M.ManuallyControlled)
PRINTF("--> update [Clusters::Switch::Id]: OnOff::Id [%d] \r\n", value);
emAfWriteAttribute(1, Clusters::Switch::Id, Clusters::OnOff::Attributes::OnOff::Id, (uint8_t *) &value, sizeof(value),
true, false);
#error "Not implemented"
// TODO: previous code was trying to write a OnOff cluster attribute id to a switch attribute, generally
// not working. Determine if this should maybe be
// OnOff::Attributes::OnOff::Set(1, is_on) or similar
#endif // SUPPORT_MANUAL_CTRL

need2sync_sw_attr = false;
Expand Down
15 changes: 4 additions & 11 deletions src/app/util/af.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,17 +22,6 @@
#endif
#include CONFIGURATION_HEADER

#ifdef EZSP_HOST
// Includes needed for ember related functions for the EZSP host
#include "app/util/ezsp/ezsp-protocol.h"
#include "app/util/ezsp/ezsp-utils.h"
#include "app/util/ezsp/ezsp.h"
#include "app/util/ezsp/serial-interface.h"
#include "stack/include/ember-random-api.h"
#include "stack/include/ember-types.h"
#include "stack/include/error.h"
#endif // EZSP_HOST

#include <app/util/af-types.h>

#include <app/util/endpoint-config-api.h>
Expand Down Expand Up @@ -101,6 +90,10 @@ bool emberAfContainsClient(chip::EndpointId endpoint, chip::ClusterId clusterId)
* over the air. Because this function is being called locally
* it assumes that the device knows what it is doing and has permission
* to perform the given operation.
*
* This function also does NOT check that the input dataType matches the expected
* data type (as Accessors.h/cpp have this correct by default).
* TODO: this not checking seems off - what if this is run without Accessors.h ?
*/
EmberAfStatus emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID,
uint8_t * dataPtr, EmberAfAttributeType dataType);
Expand Down
11 changes: 0 additions & 11 deletions src/app/util/attribute-metadata.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -105,17 +105,6 @@ union EmberAfDefaultOrMinMaxAttributeValue
const EmberAfAttributeMinMaxValue * ptrToMinMaxValue;
};

enum class EmberAfAttributeWritePermission
{
DenyWrite = 0,
AllowWriteNormal = 1,
AllowWriteOfReadOnly = 2,
UnsupportedAttribute = 0x86, // Protocols::InteractionModel::Status::UnsupportedAttribute
InvalidValue = 0x87, // Protocols::InteractionModel::Status::ConstraintError
ReadOnly = 0x88, // Protocols::InteractionModel::Status::UnsupportedWrite
InvalidDataType = 0x8d, // Protocols::InteractionModel::Status::InvalidDataType
};

// Attribute masks modify how attributes are used by the framework
//
// Attribute that has this mask is NOT read-only
Expand Down
4 changes: 0 additions & 4 deletions src/app/util/attribute-storage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -938,10 +938,6 @@ bool emberAfEndpointEnableDisable(EndpointId endpoint, bool enable)
emAfEndpoints[index].bitmask.Set(EmberAfEndpointOptions::isEnabled);
}

#if defined(EZSP_HOST)
ezspSetEndpointFlags(endpoint, (enable ? EZSP_ENDPOINT_ENABLED : EZSP_ENDPOINT_DISABLED));
#endif

if (currentlyEnabled != enable)
{
if (enable)
Expand Down
168 changes: 51 additions & 117 deletions src/app/util/attribute-table.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,36 +32,16 @@

using namespace chip;

EmberAfStatus emberAfWriteAttributeExternal(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
EmberAfAttributeType dataType)
EmberAfStatus emAfWriteAttributeExternal(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
EmberAfAttributeType dataType)
{
EmberAfAttributeWritePermission extWritePermission =
emberAfAllowNetworkWriteAttributeCallback(endpoint, cluster, attributeID, dataPtr, dataType);
switch (extWritePermission)
{
case EmberAfAttributeWritePermission::DenyWrite:
return EMBER_ZCL_STATUS_FAILURE;
case EmberAfAttributeWritePermission::AllowWriteNormal:
case EmberAfAttributeWritePermission::AllowWriteOfReadOnly:
return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType,
(extWritePermission == EmberAfAttributeWritePermission::AllowWriteOfReadOnly), false);
default:
return (EmberAfStatus) extWritePermission;
}
return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, false /* override read-only */);
}

EmberAfStatus emberAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
EmberAfAttributeType dataType)
{
return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType,
true, // override read-only?
false); // just test?
}

EmberAfStatus emberAfReadAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
uint16_t readLength)
{
return emAfReadAttribute(endpoint, cluster, attributeID, dataPtr, readLength, nullptr);
return emAfWriteAttribute(endpoint, cluster, attributeID, dataPtr, dataType, true /* override read-only */);
}

//------------------------------------------------------------------------------
Expand Down Expand Up @@ -123,30 +103,8 @@ static bool IsNullValue(const uint8_t * data, uint16_t dataLen, bool isAttribute
return false;
}

// writes an attribute (identified by clusterID and attrID to the given value.
// this returns:
// - EMBER_ZCL_STATUS_UNSUPPORTED_ENDPOINT: if endpoint isn't supported by the device.
// - EMBER_ZCL_STATUS_UNSUPPORTED_CLUSTER: if cluster isn't supported on the endpoint.
// - EMBER_ZCL_STATUS_UNSUPPORTED_ATTRIBUTE: if attribute isn't supported in the cluster.
// - EMBER_ZCL_STATUS_INVALID_DATA_TYPE: if the data type passed in doesnt match the type
// stored in the attribute table
// - EMBER_ZCL_STATUS_UNSUPPORTED_WRITE: if the attribute isnt writable
// - EMBER_ZCL_STATUS_CONSTRAINT_ERROR: if the value is set out of the allowable range for
// the attribute
// - EMBER_ZCL_STATUS_SUCCESS: if the attribute was found and successfully written
//
// if true is passed in for overrideReadOnlyAndDataType then the data type is
// not checked and the read-only flag is ignored. This mode is meant for
// testing or setting the initial value of the attribute on the device.
//
// if true is passed for justTest, then the type is not written but all
// checks are done to see if the type could be written
// reads the attribute specified, returns false if the attribute is not in
// the table or the data is too large, returns true and writes to dataPtr
// if the attribute is supported and the readLength specified is less than
// the length of the data.
EmberAfStatus emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * data,
EmberAfAttributeType dataType, bool overrideReadOnlyAndDataType, bool justTest)
EmberAfAttributeType dataType, bool overrideReadOnlyAndDataType)
{
const EmberAfAttributeMetadata * metadata = nullptr;
EmberAfAttributeSearchRecord record;
Expand Down Expand Up @@ -224,77 +182,63 @@ EmberAfStatus emAfWriteAttribute(EndpointId endpoint, ClusterId cluster, Attribu
}
}

// write the data unless this is only a test
if (!justTest)
{
const app::ConcreteAttributePath attributePath(endpoint, cluster, attributeID);
const app::ConcreteAttributePath attributePath(endpoint, cluster, attributeID);

// Pre write attribute callback for all attribute changes,
// regardless of cluster.
Protocols::InteractionModel::Status imStatus =
MatterPreAttributeChangeCallback(attributePath, dataType, emberAfAttributeSize(metadata), data);
if (imStatus != Protocols::InteractionModel::Status::Success)
{
return app::ToEmberAfStatus(imStatus);
}
// Pre write attribute callback for all attribute changes,
// regardless of cluster.
Protocols::InteractionModel::Status imStatus =
MatterPreAttributeChangeCallback(attributePath, dataType, emberAfAttributeSize(metadata), data);
if (imStatus != Protocols::InteractionModel::Status::Success)
{
return app::ToEmberAfStatus(imStatus);
}

// Pre-write attribute callback specific
// to the cluster that the attribute lives in.
status = emAfClusterPreAttributeChangedCallback(attributePath, dataType, emberAfAttributeSize(metadata), data);
// Pre-write attribute callback specific
// to the cluster that the attribute lives in.
status = emAfClusterPreAttributeChangedCallback(attributePath, dataType, emberAfAttributeSize(metadata), data);

// Ignore the following write operation and return success
if (status == EMBER_ZCL_STATUS_WRITE_IGNORED)
{
return EMBER_ZCL_STATUS_SUCCESS;
}
// Ignore the following write operation and return success
if (status == EMBER_ZCL_STATUS_WRITE_IGNORED)
{
return EMBER_ZCL_STATUS_SUCCESS;
}

if (status != EMBER_ZCL_STATUS_SUCCESS)
{
return status;
}
if (status != EMBER_ZCL_STATUS_SUCCESS)
{
return status;
}

// write the attribute
status = emAfReadOrWriteAttribute(&record,
nullptr, // metadata
data,
0, // buffer size - unused
true); // write?
// write the attribute
status = emAfReadOrWriteAttribute(&record,
nullptr, // metadata
data,
0, // buffer size - unused
true); // write?

if (status != EMBER_ZCL_STATUS_SUCCESS)
{
return status;
}
if (status != EMBER_ZCL_STATUS_SUCCESS)
{
return status;
}

// Save the attribute to persistent storage if needed
// The callee will weed out attributes that do not need to be stored.
emAfSaveAttributeToStorageIfNeeded(data, endpoint, cluster, metadata);
// Save the attribute to persistent storage if needed
// The callee will weed out attributes that do not need to be stored.
emAfSaveAttributeToStorageIfNeeded(data, endpoint, cluster, metadata);

MatterReportingAttributeChangeCallback(endpoint, cluster, attributeID);
MatterReportingAttributeChangeCallback(endpoint, cluster, attributeID);

// Post write attribute callback for all attributes changes, regardless
// of cluster.
MatterPostAttributeChangeCallback(attributePath, dataType, emberAfAttributeSize(metadata), data);
// Post write attribute callback for all attributes changes, regardless
// of cluster.
MatterPostAttributeChangeCallback(attributePath, dataType, emberAfAttributeSize(metadata), data);

// Post-write attribute callback specific
// to the cluster that the attribute lives in.
emAfClusterAttributeChangedCallback(attributePath);
}
else
{
// bug: 11618, we are not handling properly external attributes
// in this case... We need to do something. We don't really
// know if it will succeed.
ChipLogProgress(Zcl, "WRITE: no write, just a test");
}
// Post-write attribute callback specific
// to the cluster that the attribute lives in.
emAfClusterAttributeChangedCallback(attributePath);

return EMBER_ZCL_STATUS_SUCCESS;
}

// If dataPtr is NULL, no data is copied to the caller.
// readLength should be 0 in that case.

EmberAfStatus emAfReadAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
uint16_t readLength, EmberAfAttributeType * dataType)
EmberAfStatus emberAfReadAttribute(EndpointId endpoint, ClusterId cluster, AttributeId attributeID, uint8_t * dataPtr,
uint16_t readLength)
{
const EmberAfAttributeMetadata * metadata = nullptr;
EmberAfAttributeSearchRecord record;
Expand All @@ -305,20 +249,10 @@ EmberAfStatus emAfReadAttribute(EndpointId endpoint, ClusterId cluster, Attribut
status = emAfReadOrWriteAttribute(&record, &metadata, dataPtr, readLength,
false); // write?

if (status == EMBER_ZCL_STATUS_SUCCESS)
// failed, print debug info
if (status == EMBER_ZCL_STATUS_RESOURCE_EXHAUSTED)
{
// It worked! If the user asked for the type, set it before returning.
if (dataType != nullptr)
{
(*dataType) = metadata->attributeType;
}
}
else
{ // failed, print debug info
if (status == EMBER_ZCL_STATUS_RESOURCE_EXHAUSTED)
{
ChipLogProgress(Zcl, "READ: attribute size too large for caller");
}
ChipLogProgress(Zcl, "READ: attribute size too large for caller");
}

return status;
Expand Down
46 changes: 39 additions & 7 deletions src/app/util/attribute-table.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,44 @@

#include <app/util/af.h>

// Remote devices writing attributes of local device
EmberAfStatus emberAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID,
uint8_t * dataPtr, EmberAfAttributeType dataType);
/**
* Write an attribute for a request arriving from external sources.
*
* This will check attribute writeability and that
* the provided data type matches the expected data type.
*/
EmberAfStatus emAfWriteAttributeExternal(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID,
uint8_t * dataPtr, EmberAfAttributeType dataType);

/**
* @brief write an attribute, performing all the checks.
*
* This function will attempt to write the attribute value from
* the provided pointer. This function will only check that the
* attribute exists. If it does it will write the value into
* the attribute table for the given attribute.
*
* This function will not check to see if the attribute is
* writable since the read only / writable characteristic
* of an attribute only pertains to external devices writing
* over the air. Because this function is being called locally
* it assumes that the device knows what it is doing and has permission
* to perform the given operation.
*
* if true is passed in for overrideReadOnlyAndDataType then the data type is
* not checked and the read-only flag is ignored. This mode is meant for
* testing or setting the initial value of the attribute on the device.
*
* this returns:
* - EMBER_ZCL_STATUS_UNSUPPORTED_ENDPOINT: if endpoint isn't supported by the device.
* - EMBER_ZCL_STATUS_UNSUPPORTED_CLUSTER: if cluster isn't supported on the endpoint.
* - EMBER_ZCL_STATUS_UNSUPPORTED_ATTRIBUTE: if attribute isn't supported in the cluster.
* - EMBER_ZCL_STATUS_INVALID_DATA_TYPE: if the data type passed in doesnt match the type
* stored in the attribute table
* - EMBER_ZCL_STATUS_UNSUPPORTED_WRITE: if the attribute isnt writable
* - EMBER_ZCL_STATUS_CONSTRAINT_ERROR: if the value is set out of the allowable range for
* the attribute
* - EMBER_ZCL_STATUS_SUCCESS: if the attribute was found and successfully written
*/
EmberAfStatus emAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID, uint8_t * data,
EmberAfAttributeType dataType, bool overrideReadOnlyAndDataType, bool justTest);

EmberAfStatus emAfReadAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID,
uint8_t * dataPtr, uint16_t readLength, EmberAfAttributeType * dataType);
EmberAfAttributeType dataType, bool overrideReadOnlyAndDataType);
4 changes: 2 additions & 2 deletions src/app/util/ember-compatibility-functions.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1051,8 +1051,8 @@ CHIP_ERROR WriteSingleClusterData(const SubjectDescriptor & aSubjectDescriptor,
return apWriteHandler->AddStatus(aPath, Protocols::InteractionModel::Status::InvalidValue);
}

auto status = ToInteractionModelStatus(emberAfWriteAttributeExternal(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId,
attributeData, attributeMetadata->attributeType));
auto status = ToInteractionModelStatus(emAfWriteAttributeExternal(aPath.mEndpointId, aPath.mClusterId, aPath.mAttributeId,
attributeData, attributeMetadata->attributeType));
return apWriteHandler->AddStatus(aPath, status);
}

Expand Down
7 changes: 0 additions & 7 deletions src/app/util/generic-callback-stubs.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,6 @@ using namespace chip;

// This file contains overridable callbacks that are not cluster specific.

EmberAfAttributeWritePermission __attribute__((weak))
emberAfAllowNetworkWriteAttributeCallback(EndpointId endpoint, ClusterId clusterId, AttributeId attributeId, uint8_t * value,
uint8_t type)
{
return EmberAfAttributeWritePermission::AllowWriteNormal; // Default
}

bool __attribute__((weak)) emberAfAttributeReadAccessCallback(EndpointId endpoint, ClusterId clusterId, AttributeId attributeId)
{
return true;
Expand Down
Loading

0 comments on commit f5ca5ad

Please sign in to comment.