add firstled-io_M4S4BAC#2971
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 73 files 516 suites 0s ⏱️ Results for commit 364204d. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 364204d |
d0d3e68 to
09fa419
Compare
cjswedes
left a comment
There was a problem hiding this comment.
Test failures will need to be resolved prior to merging.
| { visibility = { displayed = false } })) | ||
|
|
||
| device:send(cluster_base.write_manufacturer_specific_attribute(device, | ||
| PRIVATE_CLUSTER_ID, PRIVATE_ATTRIBUTE_ID, MFG_CODE, data_types.Uint8, 0x01)) -- private |
There was a problem hiding this comment.
I dont really know what this attribute write does, but is it something that should be done in device init? The difference being it would be written anytime the hub restarts or the driver is updated
There was a problem hiding this comment.
Looks like this was moved to infoChanged to handle preference changes. It seems like it might still be needed in the added handler so that the initial preference default value is written.
There was a problem hiding this comment.
When the device is entering the pairing process, it will restore the data to the default values, and both ends will have the same default values.
2b92e15 to
29ea878
Compare
|
Can code coverage be improved in |
541f830 to
59e191b
Compare
The maximum improvement can only reach 88%. What suggestions do you have? |
You can run coverage locally and get the report yourself. When I do it I see that none of the child button behavior is tested. To run it locally first run your test file with coveraged enabled (note you need luacov installed) ## run test with coverage enabled from zigbee-switch/src
lua -lluacov test/test_firstled_switch.lua
## create the report from the stats output
luacov -c ./luacov.stats.out --config ~/SmartThingsEdgeDrivers/tools/config.luacovThis will create a |
927cc97 to
90c7984
Compare
d5ff53c to
aca4e88
Compare
|
Hi @thinkaName , it looks like this PR branch is out of date with main. Please update it with the latest main and push the result. Then we can re-review. |
aca4e88 to
364204d
Compare
cjswedes
left a comment
There was a problem hiding this comment.
It seems like there was a lot of stuff added since the last reviews wrt child devices, but we cant tell because you are obfuscating the commit history by always force pushing; this makes it hard to review this PR and others you have submitted. Please keep incremental changes in their own commits until we have approved the PR.
| -- Licensed under the Apache License, Version 2.0 | ||
|
|
||
| return { | ||
| { mfr = "FIRSTLED", model = "M4S4BAC", buttons = 4 , children = 4, child_profile = "switch-button-wireless" } |
There was a problem hiding this comment.
How are the number of buttons and children used? Please describe the way the device is intended to be integrated, since it is not clear from the code in its current state. My assumption is that there is a single zigbee device with 4 buttons, and you are trying to have a child device for each button.
What is the goal of using child devices to represent individual buttons? The approach we take on the platform is to use a multi component profile that has a component for each button.
| return parent:get_child_by_parent_assigned_key(string.format("%02X", ep_id)) | ||
| end | ||
|
|
||
| local function device_added(driver, device) |
There was a problem hiding this comment.
When a new device is joined what is the expectation for children devices?
|
|
||
| -- Create Button if necessary | ||
| local button_amount = get_button_amount(device) | ||
| if button_amount >= 1 then |
There was a problem hiding this comment.
This is always true, or is a nil error.
| -- Only create children for the actual Zigbee device and not the children | ||
| if device.network_type == device_lib.NETWORK_TYPE_ZIGBEE then | ||
| local children_amount = get_children_amount(device) | ||
| if children_amount >= 2 then |
There was a problem hiding this comment.
This is always true or is a nil error.
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests