Adding support for Hager WAASYS devices#2858
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 73 files 516 suites 0s ⏱️ Results for commit 316082b. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 316082b |
|
One way in which I think we could quickly simplify some of the Hager subdriver would be by require-ing some fields and functions from the main driver, and changing their inputs as necessary in the overriden driver, rather than rewriting everything completely from scratch. |
46b4bc2 to
1373485
Compare
hcarter-775
left a comment
There was a problem hiding this comment.
Thank you for implementing more of the pre-existing functionality. At this point, my main concern is with the way this device intersects with the main driver, all of which I've commented on at this point. As far as I can tell, we may not really have to interact with the main driver at all, and if so that would be my preference, to not include as many vendor checks in otherwise generic-ish code.
eae58ac to
ea664a3
Compare
4fa8d9f to
d11f453
Compare
ef0035d to
139d305
Compare
hcarter-775
left a comment
There was a problem hiding this comment.
There are two more minor comments that should be addressed imo, otherwise LGTM!
|
And reminder, make sure to squash your commits before merging. Thanks! |
2a5117d to
7a6bee8
Compare
912c0c8 to
d08ddc4
Compare
| local function occupancy_measured_value_handler(driver, device, ib, response) | ||
| if ib.data.value ~= nil then | ||
| device:emit_event_for_endpoint(ib.endpoint_id, ib.data.value == 0x01 and | ||
| capabilities.motionSensor.motion.active() or | ||
| capabilities.motionSensor.motion.inactive()) | ||
| end | ||
| end |
There was a problem hiding this comment.
we shouldn't need this unique handler, since it is already handled in the main driver?
There was a problem hiding this comment.
Its due to the reason that the main driver utilizes ":emit_event" instead of "emit_event_for_endpoint" in the occupancy attr handler so it would emit on the "Parent" device instead of the matter type device which is bridged to it.
I need to evaluate if implementing an emit_event override on the matter device would introduce additional complexity. Will take a look tomorrow.
There was a problem hiding this comment.
Oh sure, in that case then I'd say just change the main driver to use emit_event_on_endpoint. There's no real reason why it can't be that. Then we can remove this override
| local parent = device:get_parent_device() | ||
| device:extend_device("send", function(self, message) | ||
| return parent:send(message) | ||
| end) |
There was a problem hiding this comment.
if a device is a child, send is already handled by the parent?
There was a problem hiding this comment.
Yes, that's correct, but there is also a matter device which requires this override.
There are following devices handled by my driver:
- Parent (matter hub).
- Matter type device (this one is the reason for such override as I keep the communication through the parent device due to the limitation related to the endpoint activation and deactivation).
- Child devices.
| device.thread:call_with_delay(6, function() | ||
| if device:supports_capability(capabilities.switchLevel) then | ||
| device:set_field(MAIN_ONOFF_EP, 4, { persist = true }) | ||
| end | ||
| end) |
There was a problem hiding this comment.
Are you hoping to check this after the profile has been altered or something? Wondering due to the delay call.
There was a problem hiding this comment.
I'm not sure if I can recall the reason right now. Tomorrow I will be able to confirm it with the device.
d08ddc4 to
3b0e21e
Compare
3b0e21e to
ca6a3d2
Compare
| end | ||
|
|
||
| local function handle_set_preset(driver, device, cmd) | ||
| local endpoint_id = device:component_to_endpoint(cmd.component) |
There was a problem hiding this comment.
Hi, I hadn't noticed before, but all of this is written with tab spacing. As you can see elsewhere, other Matter drivers use 2-space spacing instead.
There was a problem hiding this comment.
Sure, I will fix it right away.
| local removed_eps, added_eps = detect_endpoint_changes(device, new_eps) | ||
|
|
||
| device:set_field(ACTIVE_EPS, new_eps, { persist = true }) |
There was a problem hiding this comment.
I'm wondering, should the endpoint change detection be comparing the device and the new endpoints every time? Because we store the new eps into ACTIVE_EPS, wouldn't this mean the ACTIVE_EPS should be compared against in the future, not the device itself?
ca6a3d2 to
316082b
Compare
| local wc_eps = device:get_endpoints(clusters.WindowCovering.ID) | ||
| local oc_eps = device:get_endpoints(clusters.OccupancySensing.ID) | ||
| local bt_eps = device:get_endpoints(clusters.Switch.ID) | ||
| local lvl_eps = device:get_endpoints(clusters.LevelControl.ID) | ||
| local product_id = device.manufacturer_info.product_id | ||
| local profile_configured = device:get_field(HAS_PROFILE) or {} | ||
|
|
||
| table.sort(wc_eps) | ||
| table.sort(oc_eps) | ||
| table.sort(lvl_eps) | ||
|
|
||
| if device:get_parent_device() ~= nil then | ||
| link_matter_device_and_parent(device) | ||
| local parent = device:get_parent_device() | ||
| device:extend_device("send", function(self, message) | ||
| return parent:send(message) | ||
| end) | ||
|
|
||
| local main_onOff_at_join = device:get_field(MAIN_ONOFF_EP) | ||
| if main_onOff_at_join and (product_id == PRODUCT_ID.SWITCH_1G or product_id == PRODUCT_ID.SWITCH_2G) then | ||
| device:set_field(MAIN_ONOFF_EP, 3, { persist = true }) | ||
| if device:supports_capability(capabilities.switchLevel) then | ||
| device:set_field(MAIN_ONOFF_EP, 4, { persist = true }) | ||
| end | ||
| end | ||
|
|
||
| if profile_configured ~= true then | ||
| if #oc_eps > 0 then | ||
| device:try_update_metadata({ profile = "motion-illuminance" }) | ||
| elseif #wc_eps > 0 and product_id == PRODUCT_ID.SWITCH_1G then | ||
| device:try_update_metadata({ profile = "window-covering" }) | ||
| device:set_field(MAIN_WC_EP, wc_eps[1]) | ||
| elseif #bt_eps == 4 then | ||
| device:try_update_metadata({ profile = "4-button" }) | ||
| elseif #bt_eps == 2 then | ||
| device:try_update_metadata({ profile = "2-button" }) | ||
| elseif #lvl_eps > 0 and product_id == PRODUCT_ID.SWITCH_1G then | ||
| device:try_update_metadata({ profile = "light-level" }) | ||
| end | ||
| device:set_field(HAS_PROFILE, true, { persist = true }) | ||
| end | ||
| else | ||
| subscribe(device, 2, clusters.Descriptor.ID, clusters.Descriptor.attributes.PartsList.ID) | ||
| local matter_device = get_matter_device(device) | ||
| device:extend_device("emit_event_for_endpoint", function(self, ep_info, event) | ||
| local endpoint_id = type(ep_info) == "number" and ep_info or ep_info.endpoint_id | ||
| local child = self:get_child_by_parent_assigned_key(string.format("%d", endpoint_id)) | ||
|
|
||
| if child then | ||
| return child:emit_event(event) | ||
| end | ||
| if matter_device then | ||
| return matter_device:emit_event_for_endpoint(ep_info, event) | ||
| end | ||
| end) | ||
| end |
There was a problem hiding this comment.
why is this happening in init? Seems like it should be happening in doConfigure (which would automatically have this happen only once), and then subsequent updates would occur in the descriptor cluster handlers
|
|
||
| for _, ep_id in ipairs(added_eps or {}) do | ||
| subscribe(parent, ep_id, clusters.Descriptor.ID, clusters.Descriptor.attributes.DeviceTypeList.ID, nil) | ||
| if device.network_type == device_lib.NETWORK_TYPE_MATTER then |
There was a problem hiding this comment.
this will always be a matter device, since it is a device report
| local parent = get_parent(driver, device) | ||
| local map = {} | ||
| if device.network_type == device_lib.NETWORK_TYPE_MATTER and device.profile.id ~= args.old_st_store.profile.id then | ||
| device.thread:call_with_delay(5, function() |
There was a problem hiding this comment.
why is this a delayed call? Does not appear like that would be necessary
| local function info_changed(driver, device, event, args) | ||
| local parent = get_parent(driver, device) | ||
| local map = {} | ||
| if device.network_type == device_lib.NETWORK_TYPE_MATTER and device.profile.id ~= args.old_st_store.profile.id then |
There was a problem hiding this comment.
I'm confused, why would we only update these things for the main device and not any children (EDGE_CHILD)? This appears to just be basic configuration logic, no? Is the idea that these would have been configured elsewhere?
| parent:send(clusters.IlluminanceMeasurement.attributes.MeasuredValue:read(parent)) | ||
| elseif device:supports_capability(capabilities.windowShadeLevel) then | ||
| map = { main = 5 } | ||
| parent:send(clusters.WindowCovering.attributes.CurrentPositionLiftPercent100ths:read(parent)) |
There was a problem hiding this comment.
I see all these reads, I do not think that is necessary. We should just set up a subscription and the same thing will basically happen. Looking at it more, I also strongly suggest simply using the subscribe logic from the main driver here (where we extend subscribe to use the custom logic in Matter Switch). It is specifically built to ensure subscriptions are created as expected for parent/child devices of varying types, like this.
| local parent = get_parent(driver, device) | ||
| local map = {} | ||
| if device.network_type == device_lib.NETWORK_TYPE_MATTER and device.profile.id ~= args.old_st_store.profile.id then |
There was a problem hiding this comment.
When this is true device.network_type == device_lib.NETWORK_TYPE_MATTER you will never have a parent. The parent only comes via being an EDGE_CHILD device.
| elseif args.old_st_store.preferences.reverse ~= device.preferences.reverse then | ||
| if device.preferences.reverse then | ||
| device:set_field(REVERSE_POLARITY, true, { persist = true }) | ||
| else | ||
| device:set_field(REVERSE_POLARITY, false, { persist = true }) | ||
| end |
There was a problem hiding this comment.
Do we really need this field, or this info_changed logic at all? I'm sure this is how its implemented in the window covering driver, but I feel like we could just directly use device.preferences.reverse in place of adding driver state to track preference state?
| elseif args.old_st_store.preferences.presetPosition ~= device.preferences.presetPosition then | ||
| local new_preset_value = device.preferences.presetPosition | ||
| device:set_field(PRESET_LEVEL_KEY, new_preset_value, { persist = true }) | ||
| end |
There was a problem hiding this comment.
similar thing here... it's not like device.preferences.presetPosition disappears, so why not just use that directly?
| return | ||
| elseif ep_id == 4 and device.manufacturer_info.product_id == PRODUCT_ID.SWITCH_2G then | ||
| if switch_utils.tbl_contains(active_eps, 3) then | ||
| local ep3 = parent:get_child_by_parent_assigned_key("3") or nil |
There was a problem hiding this comment.
| local ep3 = parent:get_child_by_parent_assigned_key("3") or nil | |
| local ep3 = parent:get_child_by_parent_assigned_key("3") |
nit: not necessary
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests
Tests performed on the physical device.