Skip to content

Commit

Permalink
Make sure we're not using non-type-safe attribute reads. (#25029)
Browse files Browse the repository at this point in the history
Gets rid of the emberAfReadServerAttribute define, and adds a lint to restrict
emberAfReadAttribute use to the implementation of the type-safe reads.
  • Loading branch information
bzbarsky-apple authored and pull[bot] committed Jul 26, 2023
1 parent 51db30d commit 1801140
Show file tree
Hide file tree
Showing 7 changed files with 943 additions and 1,008 deletions.
9 changes: 9 additions & 0 deletions .github/workflows/lint.yml
Original file line number Diff line number Diff line change
Expand Up @@ -193,3 +193,12 @@ jobs:
if: always()
run: |
git grep -n 'NSLog(' -- src/darwin/Framework/CHIP && exit 1 || exit 0
# git grep exits with 0 if it finds a match, but we want
# to fail (exit nonzero) on match. And we want to exclude this file,
# to avoid our grep regexp matching itself, as well as excluding the files
# that implement the type-safe accessors
- name: Check for use of 'emberAfReadAttribute' instead of the type-safe getters
if: always()
run: |
git grep -n 'emberAfReadAttribute' -- './*' ':(exclude).github/workflows/lint.yml' ':(exclude)src/app/util/af.h' ':(exclude)zzz_generated/app-common/app-common/zap-generated/attributes/Accessors.cpp' ':(exclude)src/app/zap-templates/templates/app/attributes/Accessors-src.zapt' ':(exclude)src/app/util/attribute-table.cpp' && exit 1 || exit 0
2 changes: 1 addition & 1 deletion examples/contact-sensor-app/telink/src/AppTask.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -601,7 +601,7 @@ void AppTask::UpdateDeviceStateInternal(intptr_t arg)
/* get boolean state attribute value */
(void) app::Clusters::BooleanState::Attributes::StateValue::Get(1, &stateValueAttrValue);

ChipLogProgress(NotSpecified, "emberAfReadAttribute : %d", stateValueAttrValue);
ChipLogProgress(NotSpecified, "StateValue::Get : %d", stateValueAttrValue);
#if CONFIG_CHIP_ENABLE_APPLICATION_STATUS_LED
sContactSensorLED.Set(stateValueAttrValue);
#endif
Expand Down
13 changes: 7 additions & 6 deletions examples/lighting-app/esp32/main/DeviceCallbacks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@

#include <app/util/util.h>

#include <app-common/zap-generated/attributes/Accessors.h>
#include <app-common/zap-generated/ids/Attributes.h>
#include <app-common/zap-generated/ids/Clusters.h>
#include <app/ConcreteAttributePath.h>
Expand Down Expand Up @@ -102,23 +103,23 @@ void AppDeviceCallbacks::OnLevelControlAttributeChangeCallback(EndpointId endpoi
#if CONFIG_LED_TYPE_RMT
void AppDeviceCallbacks::OnColorControlAttributeChangeCallback(EndpointId endpointId, AttributeId attributeId, uint8_t * value)
{
using namespace ColorControl::Attributes;

uint8_t hue, saturation;

VerifyOrExit(attributeId == ColorControl::Attributes::CurrentHue::Id ||
attributeId == ColorControl::Attributes::CurrentSaturation::Id,
VerifyOrExit(attributeId == CurrentHue::Id || attributeId == CurrentSaturation::Id,
ESP_LOGI(TAG, "Unhandled AttributeId ID: '0x%" PRIx32 "'", attributeId));
VerifyOrExit(endpointId == 1, ESP_LOGE(TAG, "Unexpected EndPoint ID: `0x%02x'", endpointId));

if (attributeId == ColorControl::Attributes::CurrentHue::Id)
if (attributeId == CurrentHue::Id)
{
hue = *value;
emberAfReadServerAttribute(endpointId, ColorControl::Id, ColorControl::Attributes::CurrentSaturation::Id, &saturation,
sizeof(uint8_t));
CurrentSaturation::Get(&saturation);
}
else
{
saturation = *value;
emberAfReadServerAttribute(endpointId, ColorControl::Id, ColorControl::Attributes::CurrentHue::Id, &hue, sizeof(uint8_t));
CurrentHue::Get(&hue);
}
AppLED.SetColor(hue, saturation);

Expand Down
4 changes: 2 additions & 2 deletions src/app/clusters/thermostat-server/thermostat-server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -393,7 +393,7 @@ int16_t EnforceHeatingSetpointLimits(int16_t HeatingSetpoint, EndpointId endpoin
// min/max are user imposed min/max

// Note that the limits are initialized above per the spec limits
// if they are not present emberAfReadAttribute() will not update the value so the defaults are used
// if they are not present Get() will not update the value so the defaults are used
EmberAfStatus status;

// https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/3724
Expand Down Expand Up @@ -464,7 +464,7 @@ int16_t EnforceCoolingSetpointLimits(int16_t CoolingSetpoint, EndpointId endpoin
// min/max are user imposed min/max

// Note that the limits are initialized above per the spec limits
// if they are not present emberAfReadAttribute() will not update the value so the defaults are used
// if they are not present Get() will not update the value so the defaults are used
EmberAfStatus status;

// https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/3724
Expand Down
5 changes: 0 additions & 5 deletions src/app/util/af.h
Original file line number Diff line number Diff line change
Expand Up @@ -123,11 +123,6 @@ EmberAfStatus emberAfWriteAttribute(chip::EndpointId endpoint, chip::ClusterId c
EmberAfStatus emberAfReadAttribute(chip::EndpointId endpoint, chip::ClusterId cluster, chip::AttributeId attributeID,
uint8_t * dataPtr, uint16_t readLength);

// For now, just define emberAfReadServerAttribute to emberAfReadAttribute, to
// minimize code churn.
// TODO: Remove this define.
#define emberAfReadServerAttribute emberAfReadAttribute

/**
* @brief this function returns the size of the ZCL data in bytes.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ EmberAfStatus Get(chip::EndpointId endpoint, {{accessorGetterType this}} value)
{{~#if (isString type)}}
{{~#*inline "lengthType"}}uint{{#if (isShortString type)}}8{{else}}16{{/if}}_t{{/inline}}
uint8_t zclString[{{maxLength}} + {{>sizingBytes}}];
EmberAfStatus status = emberAfReadServerAttribute(endpoint, {{>clusterId}}, Id, zclString, sizeof(zclString));
EmberAfStatus status = emberAfReadAttribute(endpoint, {{>clusterId}}, Id, zclString, sizeof(zclString));
VerifyOrReturnError(EMBER_ZCL_STATUS_SUCCESS == status, status);
size_t length = emberAf{{#if (isLongString type)}}Long{{/if}}StringLength(zclString);
if (length == NumericAttributeTraits<{{>lengthType}}>::kNullValue)
Expand All @@ -61,7 +61,7 @@ EmberAfStatus Get(chip::EndpointId endpoint, {{accessorGetterType this}} value)
using Traits = NumericAttributeTraits<{{accessorTraitType type}}>;
Traits::StorageType temp;
uint8_t * readable = Traits::ToAttributeStoreRepresentation(temp);
EmberAfStatus status = emberAfReadServerAttribute(endpoint, {{>clusterId}}, Id, readable, sizeof(temp));
EmberAfStatus status = emberAfReadAttribute(endpoint, {{>clusterId}}, Id, readable, sizeof(temp));
VerifyOrReturnError(EMBER_ZCL_STATUS_SUCCESS == status, status);
{{#if isNullable}}
if (Traits::IsNullValue(temp))
Expand Down
Loading

0 comments on commit 1801140

Please sign in to comment.