Add driver support for Matter cameras #2503
Conversation
nickolas-deboom
requested review from
ctowns,
hcarter-775,
samadDotDev and
z-michel
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 71 files 467 suites 0s ⏱️ Results for commit aacdf01. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against aacdf01 |
hcarter-775
left a comment
hcarter-775
left a comment
There was a problem hiding this comment.
Just an initial review but I figured I'd get these thoughts out there
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
| if child_profile then | ||
| num_floodlight_eps = num_floodlight_eps + 1 | ||
| local name = string.format("%s %d", "Floodlight", num_floodlight_eps) | ||
| driver:try_create_device( |
There was a problem hiding this comment.
I don't think we should be trying to create new children every init
There was a problem hiding this comment.
It's gated by CAMERA_INITIALIZED
There was a problem hiding this comment.
why do we have this in init at all is my question then? Why not have it in do_configure if it's meant to be a one-time call
There was a problem hiding this comment.
We could definitely move it to configure. It was placed here during testing because it was easier to just delete the inventory db to clear CAMERA_INITIALIZED and get a new profile update rather than having to re-onboard.
|
|
||
| local function info_changed(driver, device, event, args) | ||
| -- resubscribe and initialize relevant camera capabilities if a modular update has occurred | ||
| if not compare_components(device.profile.components, args.old_st_store.profile.components) then |
There was a problem hiding this comment.
not compare_components is kinda odd wording... maybe are_profiles_equal or something like that? And then we could just pass in device.profile if we wanted to be clearer with the now.
Also, this is a useful function, I might suggest moving it to switch_utils.
There was a problem hiding this comment.
I updated the name to profile_changed to kind of match the pretty similar function optional_capabilities_list_changed
There was a problem hiding this comment.
I might prefer to keep this function here for now unless it's needed elsewhere at some point in which case we could move it into a shared module
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
nickolas-deboom
force-pushed
the
add-matter-camera-support
branch
2 times, most recently
from
66f0b0a to
31c2294
Compare
| }, | ||
| supported_capabilities = fields.supported_capabilities, | ||
| sub_drivers = { | ||
| require("sub_drivers.aqara_cube"), |
There was a problem hiding this comment.
lazy loading for other subdrivers is implemented in #2525
nickolas-deboom
force-pushed
the
add-matter-camera-support
branch
from
31c2294 to
5662469
Compare
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/can_handle.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/can_handle.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
| end | ||
| end | ||
|
|
||
| local function init_ptz(device) |
There was a problem hiding this comment.
like this comment (https://github.com/SmartThingsCommunity/SmartThingsEdgeDrivers/pull/2503/files/88d7861905a705553ec6a8203b20708bc1d93ca3#r2515880941), I think something similar can be done here
There was a problem hiding this comment.
The link doesn't work, that commit may have been overwritten - could you re-link?
There was a problem hiding this comment.
Basically just saying we could do something like your changes here: 61ee699
for most/all these init functions (at least the ones where it makes sense)
There was a problem hiding this comment.
I'm not sure what the benefit would be?
There was a problem hiding this comment.
Just cause we need to be checking for the presence of the capabilities here, not the clusters
Compare the newly generated component capabilities list against the current device profile rather than saving this information in a persisted field.
nickolas-deboom
force-pushed
the
add-matter-camera-support
branch
from
7a82290 to
9639115
Compare
This commit splits the subdriver into separate modules for device configuration, fields, and utils.
These changes were implemented in 7a82290 but overwritten by accident with a force-push
| @@ -0,0 +1,128 @@ | |||
| name: matter-camera | |||
There was a problem hiding this comment.
been thinking, maybe this should just be called camera?
drivers/SmartThings/matter-switch/src/sub_drivers/camera/init.lua
Outdated
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/device_configuration.lua
Outdated
Show resolved
Hide resolved
| end | ||
| if #device:get_endpoints(clusters.OccupancySensing.ID) > 0 then | ||
| table.insert(main_component_capabilities, capabilities.motionSensor.ID) | ||
| for _, device_ep in pairs(device.endpoints) do |
There was a problem hiding this comment.
we don't really have to loop through all the endpoints, right? we can gate by the device type
| [clusters.OccupancySensing.ID] = false | ||
| } | ||
|
|
||
| if #device:get_endpoints(clusters.WebRTCTransportProvider.ID, {cluster_type = "SERVER"}) > 0 and |
There was a problem hiding this comment.
I'd say, even without introducing new variables or anything like that, we could still put the TransportProvider one in the check below, and if it's found, do a check for the Client requester across the whole device. Therefore we're only doing one of these, and it stays in the below logic block
| local microphone_component_capabilities = {} | ||
| local doorbell_component_capabilities = {} | ||
|
|
||
| local cluster_processed = { |
There was a problem hiding this comment.
are there going to be >1 of these clusters on a device?
There was a problem hiding this comment.
Probably not but there could be
| if cap.id ~= "firmwareUpdate" and cap.id ~= "refresh" then | ||
| insert_if_missing(main_component_capabilities, cap.id) | ||
| end | ||
| elseif idx == "statusLed" then |
There was a problem hiding this comment.
| elseif idx == "statusLed" then | |
| elseif idx == camera_fields.profile_components.statusLed then |
and the same to the others
drivers/SmartThings/matter-switch/src/sub_drivers/camera/device_configuration.lua
Show resolved
Hide resolved
drivers/SmartThings/matter-switch/src/sub_drivers/camera/device_configuration.lua
Outdated
Show resolved
Hide resolved
| local zone_management_ep_ids = device:get_endpoints(clusters.ZoneManagement.ID) | ||
| if #zone_management_ep_ids > 0 then |
There was a problem hiding this comment.
this can be assumed to exist since the capability zoneManagement is first checked.
| local function init_local_media_storage(device) | ||
| if device:supports_capability(capabilities.localMediaStorage) then | ||
| local supported_attributes = {} | ||
| if camera_utils.feature_supported(device, clusters.CameraAvStreamManagement.ID, clusters.CameraAvStreamManagement.types.Feature.LOCAL_STORAGE) then |
There was a problem hiding this comment.
the local storage check can be removed, since localMediaStorage capability is first checked for
| button_event = capabilities.button.button(string.format("pushed_%dx", press_value), {state_change = true}) | ||
| end | ||
|
|
||
| print("COMPONENT: ", device:endpoint_to_component(ib.endpoint_id)) |
|
|
||
| -- Event Handlers | ||
|
|
||
| local CameraEventHandlers = {} |
There was a problem hiding this comment.
oops didn't even notice there were unique events before when I made the previous suggestion- maybe let's move these to their own file as well to make everything cohesive? though it's only a small bit of code, it's a bit confusing otherwise I think?
Also, do you think we should make a new folder sub_drivers.camera.camera_handlers and put the attribute, capability, and event files in there or do you think leaving them at this level is fine? starting to look a bit crowded, but it's also in a subdriver folder so idk if it matters as much. If we did, we could also make a camera_utils folder and put all the other files in there as well, which might be nice. Not sure, what do you think?
| local CameraLifecycleHandlers = {} | ||
|
|
||
| function CameraLifecycleHandlers.device_init(driver, device) | ||
| camera_utils.update_camera_component_map(device) |
There was a problem hiding this comment.
this was my bad, but this should probably go in the if not device:get_field(camera_fields.CAMERA_INITIALIZED) block since it only needs to be done once atm
| local num_floodlight_eps = 0 | ||
| local parent_child_device = false | ||
| for _, ep in ipairs(device.endpoints or {}) do | ||
| if device:supports_server_cluster(clusters.OnOff.ID, ep.endpoint_id) then | ||
| local child_profile = device_cfg.SwitchCfg.assign_profile_for_onoff_ep(device, ep.endpoint_id) | ||
| if child_profile then | ||
| num_floodlight_eps = num_floodlight_eps + 1 | ||
| local name = string.format("%s %d", "Floodlight", num_floodlight_eps) | ||
| driver:try_create_device( | ||
| { | ||
| type = "EDGE_CHILD", | ||
| label = name, | ||
| profile = child_profile, | ||
| parent_device_id = device.id, | ||
| parent_assigned_child_key = string.format("%d", ep.endpoint_id), | ||
| vendor_provided_label = name | ||
| } | ||
| ) | ||
| parent_child_device = true | ||
| end | ||
| end | ||
| end | ||
| if parent_child_device then | ||
| device:set_field(fields.IS_PARENT_CHILD_DEVICE, true, {persist = true}) | ||
| end |
There was a problem hiding this comment.
maybe this should be made into its own DeviceConfiguration helper function? Might de-clutter this a bit more. Also, maybe we should put the helper into match_profile itself, mirroring how its done in the main driver?
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests