Skip to content

Support greater profiling for parent/child switch devices #2153

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

hcarter-775
Copy link
Contributor

@hcarter-775 hcarter-775 commented May 28, 2025

Description of Change

This PR aims to improve the dynamic profile handling of switch devices, since we have seen cases of the fingerprint being incorrect in some cases, specifically for devices that are created as parent/child.

Adding to this, it also improves the ability of devices to profile with energy cluster support. This PR handles 2 main cases that we have seen in the field (all are supported with different unit tests):

  1. The device supports the electrical sensor device types (or at least the associated cluster(s)) on specific plug endpoints. This is useful for outlets with energy measurements on each plug.
  2. The device supports the electrical sensor device type on a non-plug endpoint, whether it be on the root node or on its own (there are examples of each of these). In this case, the current precedent is that the energy handling capabilities are attached to the first plug/light endpoint profile.

Summary of Completed Tests

Unit tests updated to support case 1. Otherwise, other tests handle these cases.
Tested on physical plug energy device.
Tested on physical multi-plug energy device.

Copy link

Copy link

github-actions bot commented May 28, 2025

Test Results

   67 files    440 suites   0s ⏱️
2 250 tests 2 250 ✅ 0 💤 0 ❌
3 841 runs  3 841 ✅ 0 💤 0 ❌

Results for commit f4892df.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented May 28, 2025

File Coverage
All files 92%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-switch/src/init.lua 93%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-switch/src/embedded-cluster-utils.lua 38%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-switch/src/eve-energy/init.lua 92%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-switch/src/aqara-cube/init.lua 96%
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/matter-switch/src/third-reality-mk1/init.lua 95%

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against f4892df

@nickolas-deboom
Copy link
Contributor

I like the changes here! I think it will pair well with modular profiles, which should take care of profiling for the main device but not child devices. assign_switch_profile will also still be used for the parent device for environments that don't support modular profiles.

@nickolas-deboom
Copy link
Contributor

Did you test with any plug devices that support energy/power clusters?

table.sort(electrical_energy_eps)
table.sort(electrical_power_eps)
table.sort(switch_eps)
for i, ep in ipairs(switch_eps) do
Copy link
Contributor

Choose a reason for hiding this comment

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

This loop makes an assumption that the nth electrical ep will correspond to the nth switch ep, but what if only some eps support electrical power/energy, and some don't? For example, a 6 plug device with 3 outlets that support power and 1 that supports power and energy, one that supports only energy, and one that supports none. I would assume this is not a likely scenario, but a simpler combination of these factors could happen in a real device.

Overall it seems like it should not be on us as the client to try to make assumptions about which electrical endpoint corresponds to which switch endpoint. Shouldn't the electrical device type that is relevant to a given switch/plug be combined on the same endpoint? I might need to refresh myself on the spec for clarity, but it seems ambiguous if this convention is not used.

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