Skip to content

Fix incorrect Battery and Wired feature usage in Power Source cluster (CON-1728) #1464

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: release/v1.4
Choose a base branch
from
Open

Fix incorrect Battery and Wired feature usage in Power Source cluster (CON-1728) #1464

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e5b4722
Fix incorrect Battery and Wired feature usage in Power Source cluster
bilalahmaddev Jun 17, 2025
43f8597
Fix: Prevent setting both Wired and Battery features in Power Source …
bilalahmaddev Jun 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion components/esp_matter/esp_matter_endpoint.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2120,7 +2120,6 @@ esp_err_t add(endpoint_t *endpoint, config_t *config)
power_source_device::add(endpoint, &config->power_source_device);

cluster_t *power_source_cluster = cluster::get(endpoint, PowerSource::Id);
power_source::feature::wired::add(power_source_cluster, &config->power_source_device.power_source.wired);
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the bug in the specification PTAL https://github.com/CHIP-Specifications/connectedhomeip-spec/issues/10912, one of the wired and battery feature should be present on one endpoint. Once the spec is fixed then we should change this code.

Copy link
Author

Choose a reason for hiding this comment

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

Only one should be present, not both wired or battery

Copy link
Contributor

Choose a reason for hiding this comment

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

Has it been corrected now?

power_source::feature::battery::add(power_source_cluster, &config->power_source_device.power_source.battery);

power_source::attribute::create_bat_voltage(power_source_cluster, config->bat_voltage, 0x00, 0xFFFF);
Expand Down
12 changes: 6 additions & 6 deletions components/esp_matter/esp_matter_feature.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,9 +234,9 @@ uint32_t get_id()
esp_err_t add(cluster_t *cluster, config_t *config)
{
VerifyOrReturnError(cluster, ESP_ERR_INVALID_ARG, ESP_LOGE(TAG, "Cluster cannot be NULL"));
uint32_t battery_feature_map = feature::battery::get_id();
if ((get_feature_map_value(cluster) & battery_feature_map) == battery_feature_map) {
ESP_LOGE(TAG, "Cluster already supports Battery feature");
uint32_t wired_feature_map = feature::wired::get_id();
if ((get_feature_map_value(cluster) & wired_feature_map) == wired_feature_map) {
ESP_LOGE(TAG, "Cluster already supports Wired feature");
Comment on lines +237 to +239
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not how it works, the battery and wired features for Power Source cluster are mutually exclusive, only one of them can be present on an endpoint. This check was to prevent addition of both features on the same endpoint.

Copy link
Author

Choose a reason for hiding this comment

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

This mutually exclusive check is not correct, you can try to create a battery storage device then it will not add battery feature to power source at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

The check is correct, but the device type requirement is incorrect as mentioned in the Spec issue.

you can try to create a battery storage device then it will not add battery feature to power source at all.

Because you already have added wired feature, only one should be there on an endpoint.

Copy link
Author

Choose a reason for hiding this comment

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

This means it is mandatory to remove this from battery storage endpoint to add battery feature to power source cluster:

power_source::feature::wired::add(power_source_cluster, &config->power_source_device.power_source.wired);

Copy link
Contributor

Choose a reason for hiding this comment

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

We cannot break the spec compliance, for your use case please maintain a patch to allow battery feature on the Battery Storage device type. Once the spec is updated, we will update the sdk accordingly.

return ESP_ERR_NOT_SUPPORTED;
}
update_feature_map(cluster, get_id());
Expand All @@ -259,9 +259,9 @@ uint32_t get_id()
esp_err_t add(cluster_t *cluster, config_t *config)
{
VerifyOrReturnError(cluster, ESP_ERR_INVALID_ARG, ESP_LOGE(TAG, "Cluster cannot be NULL"));
uint32_t wired_feature_map = feature::wired::get_id();
if ((get_feature_map_value(cluster) & wired_feature_map) == wired_feature_map) {
ESP_LOGE(TAG, "Cluster already supports Wired feature");
uint32_t battery_feature_map = feature::battery::get_id();
if ((get_feature_map_value(cluster) & battery_feature_map) == battery_feature_map) {
ESP_LOGE(TAG, "Cluster already supports Battery feature");
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

return ESP_ERR_NOT_SUPPORTED;
}
update_feature_map(cluster, get_id());
Expand Down