Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Part 1: feature/temperature measurement cluster #7026

Merged
merged 11 commits into from May 31, 2021
Merged

Part 1: feature/temperature measurement cluster #7026

merged 11 commits into from May 31, 2021

Conversation

ghost
Copy link

@ghost ghost commented May 21, 2021

Problem

Update the temperature measurement cluster implementation. It still used the old emberAfPlugin API.

Change overview

  • added emberAfTemperatureMeasurementClusterServerInitCallback support
  • update temperature-measurement-server.cpp impl.
  • best effort fix of temperature-example app (not tested, because lack of HW)

Testing

Tested on custom accessory with real temperature sensor. Test setup was similar to TE2.

HW Platform

Nordic nRF52840

Notes

Related PRs

status = emberAfWriteAttribute(endpointId, ZCL_TEMP_MEASUREMENT_CLUSTER_ID, ZCL_CURRENT_TEMPERATURE_ATTRIBUTE_ID,
CLUSTER_MASK_SERVER, (uint8_t *) &newValue, ZCL_INT16S_ATTRIBUTE_TYPE);
if (status != EMBER_ZCL_STATUS_SUCCESS)
EmberAfStatus status = emberAfWriteAttribute(endpoint, ZCL_TEMP_MEASUREMENT_CLUSTER_ID, ZCL_TEMP_MEASURED_VALUE_ATTRIBUTE_ID,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we check that value is between MinMeasureValue and MaxMeasuredValue or 0x8000 (unknown) ?

One way to get this feature without any efforts would be to update the EmberAfAttributeMetadata of the attribute to points to the value of MinMeasureValue and MaxMeasureValue, based on the definition from the XML cluster definition, but I don't think this is supported at the moment.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should leverage the GENERATED_ATTRIBUTES table which is generated in endpoints_config.h for the default min max values. And also add this checks as part of the general code in the attribute storage?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should leverage the GENERATED_ATTRIBUTES table which is generated in endpoints_config.h for the default min max values. And also add this checks as part of the general code in the attribute storage?

I do agree. But at the moment the min/max value from the attribute store only support absolute values, not values relative to others attributes.

@Damian-Nordic Damian-Nordic self-requested a review May 26, 2021 09:46
@ghost ghost marked this pull request as ready for review May 26, 2021 15:45
Copy link
Contributor

@vivien-apple vivien-apple left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving for TE3.

Note that I think src/app/clusters/temperature-measurement-server should be removed completely. It seems like the result of this PR is mostly to expose a set of accessors to facilitate the manipulation of the temperature measurement cluster attributes.
It makes perfect sense to facilitate it imho, but I would rather prefer to autogenerate helpers code such as what is proposed in #7183, instead of having a lot of new clusters introduced into src/app/clusters.

var endpointClusterWithInit =
[ 'Basic', 'Identify', 'Groups', 'Scenes', 'Occupancy Sensing', 'On/off', 'Level Control', 'Color Control', 'IAS Zone' ];
var endpointClusterWithInit = [
'Basic', 'Temperature Measurement', 'Identify', 'Groups', 'Scenes', 'Occupancy Sensing', 'On/off', 'Level Control',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need to add it here if there is not in fact an init callback.

Dimitri Mizenko and others added 9 commits May 28, 2021 16:33
@github-actions
Copy link

Size increase report for "esp32-example-build" from b4d7048

File Section File VM
chip-all-clusters-app.elf .flash.rodata -24 -24
chip-all-clusters-app.elf .flash.text -84 -84
Full report output
BLOAT REPORT

Files found only in the build output:
    report.csv

Comparing ./master_artifact/chip-pigweed-app.elf and ./pull_artifact/chip-pigweed-app.elf:

sections,vmsize,filesize

Comparing ./master_artifact/chip-all-clusters-app.elf and ./pull_artifact/chip-all-clusters-app.elf:

sections,vmsize,filesize
.debug_str,0,266
.debug_loc,0,172
.debug_line,0,55
.debug_frame,0,48
.debug_info,0,37
[Unmapped],0,24
.debug_aranges,0,16
.debug_ranges,0,16
.xt.prop._ZTVN4chip11DeviceLayer37DeviceNetworkProvisioningDelegateImplE,0,-1
.flash.rodata,-24,-24
.symtab,0,-32
.flash.text,-84,-84
.strtab,0,-116
.debug_abbrev,0,-149


@andy31415 andy31415 merged commit a87bc4f into project-chip:master May 31, 2021
nikita-s-wrk pushed a commit to nikita-s-wrk/connectedhomeip that referenced this pull request Sep 23, 2021
* format endpointClusterWithInit for easier pull requests

* add support for emberAfTemperatureMeasurementClusterServerInitCallback

# Conflicts:
#	src/app/zap-templates/templates/app/helper.js

* update temperature-measurement-server impl.

* fix temperature-measurement-server.cpp

use correct values when calling
- emberAfTemperatureMeasurementClusterSetMaxMeasuredValueCallback
- emberAfTemperatureMeasurementClusterSetMinMeasuredValueCallback

* fix temperature-measurement example app

* Restyled by clang-format

* apply fixes according to @vivien-apple

* remove emberAfTemperatureMeasurementClusterServerInitCallback impl.

The default values are populated by the attribute storage.

* fix year in copyright

* Regenerate generated files

* Remove unnecessary init function for Temperature Measurement cluster

Co-authored-by: Restyled.io <[email protected]>
Co-authored-by: Boris Zbarsky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants