-
Notifications
You must be signed in to change notification settings - Fork 469
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
Adding Support for Inovelli VZM31-SN Blue 2-1 Dimmer Switch #1497
Conversation
drivers/SmartThings/zigbee-switch/src/inovelli-vzm31-sn/init.lua
Outdated
Show resolved
Hide resolved
- name: "parameter1" | ||
title: "1. Dimming Speed (Remote)" | ||
description: "This changes the speed that the light dims up when controlled from the hub. A setting of '0' turns the light immediately on. Increasing the value slows down the transition speed. Value is multiplied by 100ms. | ||
Default=25 (2500ms or 2.5s)" | ||
required: false | ||
preferenceType: number | ||
definition: | ||
minimum: 0 | ||
maximum: 126 | ||
default: 25 | ||
- name: "parameter2" | ||
title: "2. Dimming Speed (Local)" | ||
description: "This changes the speed that the light dims up when controlled at the switch. A setting of '0' turns the light immediately on. Increasing the value slows down the transition speed. Value is multiplied by 100ms. | ||
(i.e 25 = 2500ms or 2.5s) Default=127 (Sync with parameter 1)" | ||
required: false | ||
preferenceType: number | ||
definition: | ||
minimum: 0 | ||
maximum: 127 | ||
default: 127 | ||
- name: "parameter3" | ||
title: "3. Ramp Rate (Remote)" | ||
description: "This changes the speed that the light turns on when controlled from the hub. A setting of '0' turns the light immediately on. Increasing the value slows down the transition speed. Value is multiplied by 100ms. | ||
(i.e 25 = 2500ms or 2.5s) Default=127 (Sync with parameter 1)" | ||
required: false | ||
preferenceType: number | ||
definition: | ||
minimum: 0 | ||
maximum: 127 | ||
default: 127 | ||
- name: "parameter4" | ||
title: "4. Ramp Rate (Local)" | ||
description: "This changes the speed that the light turns on when controlled at the switch. A setting of '0' turns the light immediately on. Increasing the value slows down the transition speed. Value is multiplied by 100ms. | ||
(i.e 25 = 2500ms or 2.5s) Default=127 (Sync with parameter 3)" | ||
required: false | ||
preferenceType: number | ||
definition: | ||
minimum: 0 | ||
maximum: 127 | ||
default: 127 |
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.
One suggestion I would make would just be to unify all 4 of these parameters into 1 that controls the transition speed in all scenarios.
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.
We actually have 8 settings for this functionality so I have already consolidated it somewhat. I was told to try to use the same settings as what is in our red series driver that is already in the repo:
SmartThingsEdgeDrivers/drivers/SmartThings/zwave-switch/profiles/inovelli-dimmer-power-energy.yml
Line 48 in 2125c84
- name: "dimmingSpeed" |
SmartThingsEdgeDrivers/drivers/SmartThings/zwave-switch/profiles/inovelli-dimmer-power-energy.yml
Line 98 in 2125c84
- name: "dimmingSpeedZWave" |
SmartThingsEdgeDrivers/drivers/SmartThings/zwave-switch/profiles/inovelli-dimmer-power-energy.yml
Line 108 in 2125c84
- name: "rampRate" |
SmartThingsEdgeDrivers/drivers/SmartThings/zwave-switch/profiles/inovelli-dimmer-power-energy.yml
Line 118 in 2125c84
- name: "rampRateZWave" |
SmartThingsEdgeDrivers/drivers/SmartThings/zwave-switch/profiles/inovelli-dimmer-power-energy.yml
Line 155 in 2125c84
- name: "autoOffTimer" |
- name: "parameter12" | ||
title: "12. Auto Off Timer" | ||
description: "Automatically turns the switch off after this many seconds. When the switch is turned on a timer is started. When the timer expires the switch turns off. | ||
0=Auto Off Disabled." | ||
required: false | ||
preferenceType: number | ||
definition: | ||
minimum: 0 | ||
maximum: 32767 | ||
default: 0 |
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.
We'd prefer if users used our rules and automations for this sort of behavior.
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.
Sure no problem. I can remove it.
SmartThings routines
local function configuration_handler(driver, device, zb_rx) | ||
for i,v in ipairs(zb_rx.body.zcl_body.attr_records) do | ||
if (v.attr_id.value == 0x000D) then | ||
elseif (v.attr_id.value == 0x0015) then | ||
else | ||
end | ||
end | ||
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.
this function doesn't seem to do anything
local version_handler = function(driver, device, value, zb_rx) | ||
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.
nor this one. Is this intentional?
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.
Just left over after stripping functionality from the driver. It was left on accident.
else | ||
-- "info_changed event duplicate detected. Not performing any actions." |
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'd just remove this branch if you're not logging
0xfc31, | ||
0x01, | ||
0x122f, |
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 pull the cluster_id and mfg_code out and put them at the top of the file?
[clusters.SimpleMetering.ID] = { | ||
[clusters.SimpleMetering.attributes.InstantaneousDemand.ID] = power_meter_handler, | ||
[clusters.SimpleMetering.attributes.CurrentSummationDelivered.ID] = energy_meter_handler | ||
}, | ||
[clusters.ElectricalMeasurement.ID] = { | ||
[clusters.ElectricalMeasurement.attributes.ActivePower.ID] = power_meter_handler | ||
} | ||
}, | ||
global = { | ||
[0xFC31] = { | ||
[0x01] = configuration_handler | ||
} | ||
}, | ||
cluster = { | ||
[0xFC31] = { | ||
[0x00] = scene_handler, | ||
} | ||
} | ||
}, | ||
capability_handlers = { | ||
[capabilities.switch.ID] = { | ||
[capabilities.switch.commands.on.NAME] = on_handler, | ||
[capabilities.switch.commands.off.NAME] = off_handler, |
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.
nit: some mixed tabs and spaces here
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.
Could you rename this to match the other related profiles rgbw-bulb-[min_color_temp]-[max_color_temp].yml
and add in the firmwareUpdate
capability?
Hey @InovelliUSA Can you check out the open comment above on this PR please at your nearest opportunity? Thank you! |
Sure, I will check it out right now. |
I believe I have adjusted everything requested. I did find a bug that I addressed in info_changed as well. Other than that, I just changed what was asked. |
Duplicate profile check: Passed - no duplicate profiles detected. |
Test Results 64 files 406 suites 0s ⏱️ Results for commit 7c502a5. ♻️ This comment has been updated with latest results. |
Minimum allowed coverage is Generated by 🐒 cobertura-action against 7c502a5 |
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 need to git rm
this file
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.
K, I deleted it.
|
Hey @InovelliUSA Thanks so much. We noticed that there arent yet Unit Tests written for this driver. We should have called this out a bit sooner, so apologies. I will recommend that we continue with the WWST process and we can review the driver tests in parallel. To learn more about unit driver tests, please see https://developer.smartthings.com/docs/devices/hub-connected/test-your-driver |
This still needs to be fixed in the meantime as well. |
This should be fixed now. |
Hey @InovelliUSA - Can you let me know if you have any questions on the above message please? Thank you. |
I have always tested with the device itself, so I haven't created the unit tests. I will have to research how to do that. |
Channel deleted. |
@InovelliUSA Can you please review and sign the CLA? Thank you! |
Hello - I am going to close this PR. If it needs to be reopened, it can be. Thank you. |
Channel deleted. |
Hello @InovelliUSA We can merge this PR and deploy, however we need a signature on the CLA. Can you please review at your nearest opportunity? Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution. |
Sorry about that, I think I just completed it. |
It appears that @erocm123 needs to still sign, are they available to sign the CLA? |
No description provided.