Isolate the eve-energy subdriver to private cluster usage#3019
Isolate the eve-energy subdriver to private cluster usage#3019samadDotDev wants to merge 3 commits into
Conversation
samadDotDev
requested review from
ctowns,
hcarter-775,
nickolas-deboom and
tpmanley
|
Invitation URL: |
Test Results 73 files 515 suites 0s ⏱️ Results for commit d141a47. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against d141a47 |
| test.register_coroutine_test( | ||
| "Eve Energy sub-driver can_handle should return false for devices with Electrical Sensor device type", | ||
| function() | ||
| local eve_energy_can_handle = require("sub_drivers.eve_energy.can_handle") | ||
| local result = eve_energy_can_handle(nil, nil, mock_eve_device_using_electrical_sensor) | ||
| assert(result == false, "can_handle should return false for Eve device with electrical sensor") | ||
| end, | ||
| { | ||
| min_api_version = 17 | ||
| } | ||
| ) |
There was a problem hiding this comment.
this is already tested in this file
There was a problem hiding this comment.
Yeah this is indirectly tested by a larger test but I wanted to add specific tests for can_handle in all 3 combinations (with private cluster, electrical sensor device type, and both). I can remove this one though.
| -- Helper function to add get_endpoints method to mock devices for can_handle testing | ||
| local function add_get_endpoints_to_mock(device) | ||
| device.get_endpoints = function(self, cluster_id, opts) | ||
| opts = opts or {} | ||
| local eps = {} | ||
| for _, ep in ipairs(self.endpoints) do | ||
| for _, cluster in ipairs(ep.clusters or {}) do | ||
| if cluster.cluster_id == cluster_id then | ||
| -- Check feature_bitmap if specified | ||
| if opts.feature_bitmap == nil or | ||
| (cluster.feature_map and (cluster.feature_map & opts.feature_bitmap) == opts.feature_bitmap) then | ||
| table.insert(eps, ep.endpoint_id) | ||
| break | ||
| end | ||
| end | ||
| end | ||
| end | ||
| return eps | ||
| end | ||
| return device | ||
| end | ||
|
|
There was a problem hiding this comment.
this feels like an overcomplicated solution. I might suggest we just add one more mock device with both clusters present.
There was a problem hiding this comment.
I actually started with that i.e. by adding this util function to can_handle and that'd just work best for mock device in tests, but didn't want to influence the functional code just because of test infra not providing device::get_endpoints:
diff --git a/drivers/SmartThings/matter-switch/src/sub_drivers/eve_energy/can_handle.lua b/drivers/SmartThings/matter-switch/s
rc/sub_drivers/eve_energy/can_handle.lua
index 02bc9e06..b5b536a4 100644
--- a/drivers/SmartThings/matter-switch/src/sub_drivers/eve_energy/can_handle.lua
+++ b/drivers/SmartThings/matter-switch/src/sub_drivers/eve_energy/can_handle.lua
@@ -5,13 +5,24 @@ local device_lib = require "st.device"
local fields = require "switch_utils.fields"
local switch_utils = require "switch_utils.utils"
+local function has_cluster(device, cluster_id)
+ for _, ep in ipairs(device.endpoints) do
+ for _, cluster in ipairs(ep.clusters or {}) do
+ if cluster.cluster_id == cluster_id then
+ return true
+ end
+ end
+ end
+ return false
+end
+
return function(opts, driver, device)
local EVE_PRIVATE_CLUSTER_ID = 0x130AFC01
-- this sub driver loads for devices that:
-- 1. Contain the Eve Private Cluster (0x130AFC01)
-- 2. Do NOT have the Standard Electrical Sensor device type
if device.network_type == device_lib.NETWORK_TYPE_MATTER and
- #device:get_endpoints(EVE_PRIVATE_CLUSTER_ID) > 0 and
+ has_cluster(device, EVE_PRIVATE_CLUSTER_ID) and
#switch_utils.get_endpoints_by_device_type(device, fields.DEVICE_TYPE_ID.ELECTRICAL_SENSOR) == 0 then
return true, require("sub_drivers.eve_energy")
endLet me know what you prefer, and curious about thoughts from @ctowns as well here!
There was a problem hiding this comment.
I made that change in 5d2a4d2 but LMK if you were thinking something different.
There was a problem hiding this comment.
Ah, I see the issue you're mentioning. Not to spend too much more time on this, but perhaps it would be better to implicitly test can_handle by just adding an init test instead of an explicit can_handle test? It is a little annoying that the current infrastructure doesn't well support direct can_handle tests, so this is more or less the way that other drivers do it.
There was a problem hiding this comment.
If you have issues with that, I do think it would be better to revert to the first version you made than to affect the actual can_handle code.
There was a problem hiding this comment.
Yeah at that point we're exercising what existing tests are already doing implicitly and seems odd to cover all cases of specifically can_handle. I have reverted the change in d141a47
| local PRIVATE_ATTR_ID_ACCUMULATED_CONTROL_POINT = 0x130A000E | ||
|
|
||
| -- Helper function to add get_endpoints method to mock devices | ||
| local function add_get_endpoints_to_mock(device) |
There was a problem hiding this comment.
Just a note: technically this does not match the lua library definition of the get_endpoints function, but I think it covers the test appropriately. The alternative would be to manually register each of the new mock devices using their own test init function so that the device function metatable is populated with functions like get_endpoints from the lua libs, but I think that is too much overhead for just doing a quick can_handle test for each of these mock devices, and this works well for the purposes of this test.
4 participants
Type of Change
Checklist
Description of Change
The eve-energy sub-driver was being loaded even for other Eve devices that don't contain the private cluster (which eve energy subdriver uses for energy/power measurements pre-1.3). This change limits the scope of that subdriver to just the devices that contains such private cluster (id of which has the encoding for Eve's vendor id already) AND when device doesn't have the standard device type that was later introduced in Matter.
Summary of Completed Tests