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

Cluster: Add Temperature Measurement commands to chip-tool and src/app #3009

Conversation

vivien-apple
Copy link
Contributor

Problem

In order to have an minimal example application that would serve as a baseline for memory consumption, it would be nice to use a simple cluster that has only attributes.
The current PR add a command in chip-tool and the corresponding method on src/app to read the current temperature from the Temperature Measurement server.

Summary of Changes

  • Add a new command in chip-tool
  • Add the corresponding command in src/app
  • Fix attributes matching for chip-tool as previously only the first attribute of the list of commands was checked
  • Fix attribute read command in src/app/encoder.cpp. It was using an uint8_t for attribute id while it is really an uint16_t

Fixes #2982, #2981

@rwalker-apple rwalker-apple merged commit 6712c1b into project-chip:master Oct 2, 2020
case 0x29: { // "Int16"
int16_t value;
CHECK_MESSAGE_LENGTH(messageLen == 2);
value = chip::Encoding::LittleEndian::Read16(message);
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't really right. Read16 will read a uint16_t, and this assignment is undefined behavior if the value is outside the int16_t range. Now hopefully that will never happen in practice, but we need a better setup for this long-term, and definitely before we enable -Wconversion tree-wide.

int16_t value;
CHECK_MESSAGE_LENGTH(messageLen == 2);
value = chip::Encoding::LittleEndian::Read16(message);
ChipLogProgress(chipTool, "Attribute value: '0x%04x'", value);
Copy link
Contributor

Choose a reason for hiding this comment

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

That format string doesn't really make that much sense for a signed int16_t, right?

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.

Add temperature sensor read attribute commands to chiptool
5 participants