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

components/esp-matter: added features support to power source device … (CON-1498) #1230

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bnadim
Copy link

@bnadim bnadim commented Jan 6, 2025

Description

Based on matter spec 1.3, power source endpoint should support features Wired, Battery, Rechargeable, Replaceable.
This is supported in the cluster creation and in endpoint config through descriptors, but on endpoint creation, descriptor feature are not passed to cluster at creation resulting in ignoring feature attributes.

This pr fix this by passing the features to cluster creation which was an omission.

Although it doesn't introduce breaking changes strictly speaking, if a user did provided feature in descriptor and did not update attributes, it may introduce a change.

Related

Testing

Tested on a matter setup with an Apple homepod device and Apple home app showing low battery level.


Checklist

Before submitting a Pull Request, please ensure the following:

  • 🚨 This PR does not introduce breaking changes.
  • All CI checks (GH Actions) pass.
  • Documentation is updated as needed.
  • Tests are updated or added as necessary.
  • Code is well-commented, especially in complex areas.
  • Git history is clean — commits are squashed to the minimum necessary.

@CLAassistant
Copy link

CLAassistant commented Jan 6, 2025

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot changed the title components/esp-matter: added features support to power source device … components/esp-matter: added features support to power source device … (CON-1498) Jan 6, 2025
@@ -212,7 +212,7 @@ esp_err_t add(endpoint_t *endpoint, config_t *config)
return err;
}

cluster_t *cluster = power_source::create(endpoint, &(config->power_source), CLUSTER_FLAG_SERVER, ESP_MATTER_NONE_FEATURE_ID);
cluster_t *cluster = power_source::create(endpoint, &(config->power_source), CLUSTER_FLAG_SERVER, config->descriptor.features);
Copy link
Contributor

Choose a reason for hiding this comment

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

It should have been the feature flags of the Power Source cluster and not of the Descriptor cluster.

Ideally, power_source::config_t should have the features member variable, and that should be set when creating the device type, then that value has to be used in here instead of the descriptor cluster's.

We are in the process of adding the feature_flags member variable to every cluster's config_t and will be available soon, and then that can be populated when creating the device type.

I'd suggest please wait till then.

Copy link
Author

Choose a reason for hiding this comment

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

Hey thank you for your feedback.
I understand. So i will wait until features member is added and update my pr.

@bnadim bnadim marked this pull request as draft January 10, 2025 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants