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 e0a5cfa. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against e0a5cfa |
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.
There was a problem hiding this comment.
Since this is the first product submitted, a series of devices will be added to reuse this driver. Currently, this is the 4+4 model, consisting of 4 switches that can be set to button mode, plus 4 pure buttons. The number of child devices to be created is determined by "fingerprints.children", and the number of buttons to be created is determined by "fingerprints.buttons". Subsequently, models such as 3+3, 2+2, and 1+1 will be added. Additionally, there will be models without the button, and the setting for fingerprints will be fingerprints.buttons=0.
There was a problem hiding this comment.
Thank you, this clears up a lot of my confusion with what is going on with the child devices. You may want to add a comment at the top of this subdriver explaining how this device works.
Because it was previously informed that a device's PR could only have one commit, so the forced override push was carried out. |
I apologize for the confusion. We do want there to only be a single commit when we merge, but it is useful to see how feedback is addressed by keeping commits separate during the review period. |
718fdbe to
e0a5cfa
Compare
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests