Adding support for Hager WAASYS devices#2858
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Channel deleted. |
Test Results 73 files 518 suites 0s ⏱️ Results for commit 4252039. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 4252039 |
|
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. |
JanJakubiszyn
force-pushed
the
Hager_WAASYS_device_support
branch
from
46b4bc2 to
1373485
Compare
hcarter-775
left a comment
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.
JanJakubiszyn
force-pushed
the
Hager_WAASYS_device_support
branch
2 times, most recently
from
eae58ac to
ea664a3
Compare
JanJakubiszyn
force-pushed
the
Hager_WAASYS_device_support
branch
3 times, most recently
from
4fa8d9f to
d11f453
Compare
JanJakubiszyn
force-pushed
the
Hager_WAASYS_device_support
branch
from
ef0035d to
139d305
Compare
hcarter-775
left a comment
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! |
JanJakubiszyn
force-pushed
the
Hager_WAASYS_device_support
branch
from
2a5117d to
7a6bee8
Compare
JanJakubiszyn
force-pushed
the
Hager_WAASYS_device_support
branch
6 times, most recently
from
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.
JanJakubiszyn
force-pushed
the
Hager_WAASYS_device_support
branch
from
d08ddc4 to
3b0e21e
Compare
JanJakubiszyn
force-pushed
the
Hager_WAASYS_device_support
branch
from
3b0e21e to
ca6a3d2
Compare
| 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?
JanJakubiszyn
force-pushed
the
Hager_WAASYS_device_support
branch
from
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
| 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.
There was a problem hiding this comment.
The reason for custom subscription handling in my driver is that after an endpoint is deactivated and/or activated, the device's JSON might not update accurately. It won't show all the available endpoints, making the device.endpoints call miss some of them. The subscription through cluster_base eliminates the possibility of such behaviour by ensuring that all of the dynamic endpoints receive the subscription regardless of the cached device JSON.
I will change those reads to subscriptions.
There was a problem hiding this comment.
Ah, that makes sense. I am still wondering though if we even need these at all? Don't you already subscribe to these attributes elsewhere?
There was a problem hiding this comment.
Yes, these reads were likely added to get initial state faster before subscriptions arrive. Since I'm already subscribing to these clusters elsewhere, they're redundant. I'll verify with the devices and remove them.
There was a problem hiding this comment.
ahh, sure that also makes sense. In the main driver we also update the subscription upon an infoChanged event. In that case, I might say we should make these subscribes, and then remove the other place where the subscription is happening then (in the descriptor cluster handlers)
There was a problem hiding this comment.
@JanJakubiszyn In your current subscription implementation, one concern I have is whether the subscription will be re-initialized correctly when the hub is restart. This will delete the current subscription cache, and so this will need to be re-populated somehow. For this reason, I think you may want to include the main driver's subscription method. The primary issue with this at the moment is related to the fact that this is a subdriver. For that reason, I might suggest you look at the work I did in this PR and implement that subscription method here in utils.lua. Then, you can add a subscriptions.lua file like in this PR. Then you can just call "device:subscribe()" the same way that it's done in the main driver. You can also use the example in the current Matter Camera driver as an example of what to do instead, though I'm partial to this method in PR.
But basically, the issue now is that when the hub restarts, your subscription will be removed and you currently have no way of re-creating it.
JanJakubiszyn
force-pushed
the
Hager_WAASYS_device_support
branch
from
316082b to
c56c96c
Compare
|
Hello @hcarter-775 , I've implemented the changes to the code and tested it with the physical devices. Please let me know if this state is acceptable. After that, I will adjust the unit tests. |
| device:set_field(fields.profiling_data.POWER_TOPOLOGY, false, { persist = true }) | ||
| device:set_field(fields.profiling_data.BATTERY_SUPPORT, fields.battery_support.NO_BATTERY, { persist = true }) |
There was a problem hiding this comment.
Since you're no longer using the main driver's logic, you shouldn't need these field sets.
| device:set_field(fields.profiling_data.POWER_TOPOLOGY, false, { persist = true }) | ||
| device:set_field(fields.profiling_data.BATTERY_SUPPORT, fields.battery_support.NO_BATTERY, { persist = true }) | ||
|
|
||
| if device:get_parent_device() ~= nil then |
There was a problem hiding this comment.
Since you have:
if device.network_type ~= device_lib.NETWORK_TYPE_MATTER then
return
end
there will never be a parent device at this point. If you're trying to focus in on child devices with this, I might suggest you add an "if device.network_type == device_lib.EDGE_CHILD" check at the top or bottom of init and do this logic.
There was a problem hiding this comment.
then you don't need the "else" either, as you will know you're interacting with the primary matter device.
There was a problem hiding this comment.
This guard handles the Matter child device in a parent-child architecture. There is always one parent device (MATTER type, no parent) and one Matter child device (MATTER type, with parent reference). which require a different initialization.
Additional EDGE_CHILD devices are filtered out by the NETWORK_TYPE_MATTER check.
| end | ||
|
|
||
| local function info_changed(driver, device, event, args) | ||
| if device.profile.id ~= args.old_st_store.profile.id or device.network_type == device_lib.NETWORK_TYPE_CHILD then |
There was a problem hiding this comment.
| if device.profile.id ~= args.old_st_store.profile.id or device.network_type == device_lib.NETWORK_TYPE_CHILD then | |
| if device.profile.id ~= args.old_st_store.profile.id then |
I don't think you should just blanket allow all this to happen if it's a child device. The initial "device.profile.id" should be good enough, since the old profile would be non-existent if this is a new device.
There was a problem hiding this comment.
I've noticed that child devices don't pass the profile change condition on creation. The component-endpoint mapping is primarily for the matter_device's command routing. Child devices route commands through the parent by default, so the mapping field has no impact on their behavior. Testing confirms this works correctly.
| if device.profile.id ~= args.old_st_store.profile.id or device.network_type == device_lib.NETWORK_TYPE_CHILD then | ||
| local parent = device:get_parent_device() | ||
| local map = {} | ||
| device.thread:call_with_delay(2, function() |
There was a problem hiding this comment.
you don't need this delay
There was a problem hiding this comment.
During testing, I observed that removing this delay causes subscriptions to be sent before component mapping completes for 1G and PIR devices. This results in devices remaining in 'connected' state with no initial state reported until the first attribute change occurs.
|
|
||
| if ep_id == 3 then | ||
| if device.manufacturer_info.product_id == PRODUCT_ID.SWITCH_1G then | ||
| matter_device:try_update_metadata({ profile = "2-button" }) |
There was a problem hiding this comment.
One thing I find strange about this logic is that you are able to call try_update_metadata more than once for the same device. I might try and re-work this logic to ensure that "try_update_metadata" is only ever called once in this handler, to limit calls.
Also, we don't necessarily want to try this call every time we get a descriptor cluster response (like if the device or hub ever go offline, for example).
| elseif #lvl_eps > 0 and product_id == PRODUCT_ID.SWITCH_1G then | ||
| device:try_update_metadata({ profile = "light-level" }) | ||
| end | ||
| device:set_field(BUTTON_EPS, bt_eps) |
There was a problem hiding this comment.
| device:set_field(BUTTON_EPS, bt_eps) | |
| device:set_field(BUTTON_EPS, bt_eps, { persist = true }) |
I think you want this
JanJakubiszyn
force-pushed
the
Hager_WAASYS_device_support
branch
from
c56c96c to
4252039
Compare
2 participants
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests
Tests performed on the physical device.