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

Update ember library to have a better way to address a specific element of an attribute while reading/writing to it. #5896

Closed
vivien-apple opened this issue Apr 9, 2021 · 3 comments

Comments

@vivien-apple
Copy link
Contributor

While working on #5020 and in order to minimise the amount of changes, an index argument has been added to some of the methods related to attributes reading/writing.

The goal of this argument is to address a particular element of the attribute. At the moment it is represented as an int32 where -1 represents read/write all the List, 0 represents read/write the actual number of elements in this list, and n read/write the nth element of the list.

Because of this actual element indexing starts at 1. There should be a better way to express that.

Follow is Boris comment, asking for a followup.

Please document the semantics of this "index" arg and why we're adding "1" to it. The fact that -1 becomes "read or write the length" and non-negative values become entry writes is extremely non-obvious.

In terms of naming, readWritePartOfListAttribute (and similar for the separate writeAttribute/readAttribute methods) might be closer to what this thing really does. And document really carefully, then file a followup to have a sane way of doing this. :( Will need some surgery on the ember stuff.

Originally posted by @bzbarsky-apple in #5020 (comment)

@turon
Copy link
Contributor

turon commented Feb 1, 2022

Referenced comment/code:

[src/app/clusters/descriptor/descriptor.cpp:43]

    record.manufacturerCode = EMBER_AF_NULL_MANUFACTURER_CODE;
    record.attributeId      = attributeId;

    return emAfReadOrWriteAttribute(&record, NULL, (uint8_t *) buffer, 0, write, index + 1);
@bzbarsky-apple bzbarsky-apple on Apr 8, 2021
Please document the semantics of this "index" arg and why we're adding "1" to it. The fact that -1 becomes "read or write the length" and non-negative values become entry writes is extremely non-obvious.

In terms of naming, readWritePartOfListAttribute (and similar for the separate writeAttribute/readAttribute methods) might be closer to what this thing really does. And document really carefully, then file a followup to have a sane way of doing this. :( Will need some surgery on the ember stuff.

Author @vivien-apple vivien-apple on Apr 9, 2021
#5896

@turon
Copy link
Contributor

turon commented Feb 1, 2022

Older issue. The referenced code no longer calls emAfReadOrWriteAttribute.
Marking stale to be closed after validation by author or reviewer.
Removing tag v1_triage_split_4

@turon turon added stale Stale issue or PR and removed v1_triage_split_4 labels Feb 1, 2022
@stale stale bot removed the stale Stale issue or PR label Feb 1, 2022
@bzbarsky-apple
Copy link
Contributor

The index arg went away in #12664.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants