-
Notifications
You must be signed in to change notification settings - Fork 521
WWSTCERT-8461 Aqara dimmer controller t1 #2468
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
WWSTCERT-8461 Aqara dimmer controller t1 #2468
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Channel deleted. |
Test Results 71 files 468 suites 0s ⏱️ Results for commit 260854a. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against 260854a |
|
|
||
| local devices = { | ||
| AQARA_LIGHT = { | ||
| MATCHING_MATRIX = { mfr = "LUMI", model = "lumi.light.acn004" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should just be able to add the new model here rather than duplicating this map entirely
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the late confirmation. It's been updated—please check again.
| local function init(self, device) | ||
| local min = 2700 | ||
| local max = 6000 | ||
|
|
||
| if device:get_model() == "lumi.light.cwacn1" then | ||
| max = 6500 | ||
| end | ||
| device:emit_event(capabilities.colorTemperature.colorTemperatureRange({ minimum = min, maximum = max })) | ||
| end | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand the instinct here so that existing devices get this change, but we do not want this event to fire every time the driver starts.
I would add this event to the added handler as well, and a follow-up PR a week later should be created to remove this code here in init. I'd also add gating to check for an existing colorTemperatureRange value before emitting this event.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I'm a bit confused. Should I add this event to the added handler and remove the one in init in a follow-up PR a week later—instead of doing it now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So we want to make sure that existing already-installed devices get this change, which is (I assumed) why you placed it in init, since that is called every time the driver starts, rather than in added, which is only called once in the lifecycle of the device.
In general, this code should only be in the added handler, but for a week, I recommend having it in both added and init so that already-installed devices will update.
After that there should be a follow-up PR to remove the code from init since it will no longer be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, got it. I'll add this event to the added handler first and remove the code from init in a follow-up PR later. Thanks!
| local matrix = device.MATCHING_MATRIX | ||
| if not matrix then goto continue end | ||
|
|
||
| -- Single table format: { mfr = "...", model = "..." } | ||
| if matrix.mfr and matrix.model then | ||
| if matrix.mfr == mfr and matrix.model == model then | ||
| return device.PARAMETERS | ||
| end | ||
| goto continue | ||
| end | ||
| -- Array format: { { mfr = "...", model = "..." }, ... } | ||
| if type(matrix) == "table" then | ||
| for _, match in ipairs(matrix) do | ||
| if match.mfr == mfr and match.model == model then | ||
| return device.PARAMETERS | ||
| end | ||
| end | ||
| end | ||
| ::continue:: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was the bug here that required this rewrite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I add the new model here rather than duplicating the entire map, preferences won't work properly because the existing get_device_parameters function can't handle the Array format for the model.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've left some suggestions that seem a lot simpler than this rewrite, based on how this is handled elsewhere in the code base.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the suggestions, I've updated the code.
| } | ||
| }, | ||
| AQARA_LIGHT_BULB = { | ||
| MATCHING_MATRIX = { mfr = "Aqara", model = "lumi.light.acn014" }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| MATCHING_MATRIX = { | |
| { mfr = "Aqara", model = "lumi.light.acn014" }, | |
| } |
| if device:get_model() == "lumi.light.cwacn1" then | ||
| max = 6500 | ||
| end | ||
| device:emit_event(capabilities.colorTemperature.colorTemperatureRange({ minimum = min, maximum = max })) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add the emit_event_if_latest_state_missing check for this code, since it runs every time the driver starts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, done.
|
You will need to rebase now, as we did a major reconfiguration of the zigbee switch driver code to reduce memory usage |
ce4156c to
b13bde7
Compare
I've rebased. |
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests