Fix incorrect Battery and Wired feature usage in Power Source cluster (CON-1728) #1464
Conversation
|
|
github-actions
bot
changed the title
…cluster as disallowed by conformance
| 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"); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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);
There was a problem hiding this comment.
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.
| 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"); |
| 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); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Only one should be present, not both wired or battery
This commit fixes a bug in the Power Source cluster feature implementation where the Battery and Wired feature checks were incorrectly swapped:
In the Wired feature section:
In the Battery feature section:
This also fixes setting both Wired and Battery features in Power Source cluster in battery storage end device as disallowed by conformance. A power source cluster can not have both wired and battery features under one endpoint. Therefore for a battery storage device only battery feature should be enabled while creating battery storage endpoint.
Impact:
This fix ensures that the feature validation logic correctly identifies when a cluster already supports the respective Battery or Wired features, preventing incorrect error messages and ensuring proper feature management in the ESP Matter framework.
Fixes #1462