Requset to add a fingerprints for Kichler Zigbee fan light #1278

seungkwan-choi · 2024-03-19T08:07:21Z

This is new project for Kichler.
It's a Zigbee fan and the model no. is KICHLER-FANLIGHT-Z-301.
I want to add Kichler to the fingerprints of a Zigbee fan.
The function of the option has been added to the fan light
Please review it.

Add fingerprints for KICHLER

Create Profiles for kichler-fan-light

Add update init for Kichler

Add update configurations for Kichler

Add create init for Kichler fan light

Add create test kichler fan light

github-actions · 2024-03-19T08:08:03Z

Duplicate profile check: Passed - no duplicate profiles detected.

github-actions · 2024-03-19T08:08:51Z

Invitation URL:
https://bestow-regional.api.smartthings.com/invite/1PlYJ9aDXpje

github-actions · 2024-03-19T08:09:15Z

Test Results

56 files 363 suites 0s ⏱️
1 757 tests 1 757 ✅ 0 💤 0 ❌
3 049 runs 3 049 ✅ 0 💤 0 ❌

Results for commit 584c4a9.

♻️ This comment has been updated with latest results.

github-actions · 2024-03-19T08:09:16Z

File	Coverage
All files	`97%`	✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-fan/src/fan-light/init.lua	`95%`	✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-fan/src/configurations.lua	`97%`	✅
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-fan/src/kichler-fan-light/init.lua	`97%`	✅

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against 584c4a9

greens · 2024-03-22T21:02:49Z

drivers/SmartThings/zigbee-fan/src/kichler-fan-light/init.lua

+  local last_level = device:get_field('LAST_DIM_LEVEL') or 100
+  local level = math.floor((last_level/100.0) * 254 )


nit: these should be moved inside the if statement

I will change it.

greens · 2024-03-22T21:05:06Z

drivers/SmartThings/zigbee-fan/src/kichler-fan-light/init.lua

+
+-- CAPABILITY HANDLERS
+local function on_handler(driver, device, command)
+  local last_level = device:get_field('LAST_DIM_LEVEL') or 100


as far as I can tell, you maintain the level value even when receiving an off report. Unless the device separately reports a 0 level when turning off, you should be able to use device:get_latest_state instead of setting an additional field here.

When i apply the code that was guided to me, if i turn on the light after turning it off, i can't retrieve the last value (off), and instead retrieve the off state. so, I will continue to use the set/get_field as you are now.

were you getting the latest value of the switch capability or the switchLevel capability?

device:get_latest_state("main", capabilities.switchLevel.ID, capabilities.switchLevel.level.NAME)

The use of the switchlevel is because it is synchronized with the main switch.
The proposed get_lastest_state maintains "off" if you turn it on after turning it off, as the last state value is "off".
Therefore, we intend to use set/get field instead of using get_lastest_state.

greens · 2024-03-22T21:06:57Z

drivers/SmartThings/zigbee-fan/src/kichler-fan-light/init.lua

+    device:emit_component_event(device.profile.components.light, capabilities.switchLevel.level(command.args.level))
+    device:send_to_component('light', Level.server.commands.MoveToLevelWithOnOff(device, level, command.args.rate or 0xFFFF))
+    device:emit_component_event(device.profile.components.light, capabilities.switchLevel.level(trim_level))


What's the intent behind these two level events from the same component here?

This is to use the Trim function. This trim is used to limit the minimum value of dimming.

I understand the point of the trim function, but it seems like you'd want to only emit the trimmed value, not both the untrimmed and trimmed value.

I would like to refresh the value below the trim setting value so that it is not reflected in the UI. If you set it to trim 25% and control it to 10%, it will be 25%.

So why send both the 10% event and the 25% event? Wouldn't you want to just send the 25% event?

Set Trim to 25%.
If the user has changed to 10% in the UI, the intention is to express it as 25%.
However, if you adjust 25% and then adjust it to less than that (e.g., 20%), it is to inform you of the event that it has been changed to go back to 25%

greens · 2024-03-22T21:11:29Z

Please work to raise the code coverage to at least 90%

Update init file

greens · 2024-03-26T21:28:06Z

drivers/SmartThings/zigbee-fan/src/kichler-fan-light/init.lua

+      if old_trim < new_trim then
+        if new_trim >= current_level then
+          local newlevel = math.floor((new_trim/100.0) * 254 )
+          device:send_to_component('light', Level.server.commands.MoveToLevelWithOnOff(device, newlevel, 0))
+        else
+          local newlevel = math.floor((current_level/100.0) * 254 )
+          device:send_to_component('light', Level.server.commands.MoveToLevelWithOnOff(device, newlevel, 0))
+        end
+      else
+        local newlevel = math.floor((current_level/100.0) * 254 )
+        device:send_to_component('light', Level.server.commands.MoveToLevelWithOnOff(device, newlevel, 0))
+      end


Suggested change

if old_trim < new_trim then

if new_trim >= current_level then

local newlevel = math.floor((new_trim/100.0) * 254 )

device:send_to_component('light', Level.server.commands.MoveToLevelWithOnOff(device, newlevel, 0))

else

local newlevel = math.floor((current_level/100.0) * 254 )

device:send_to_component('light', Level.server.commands.MoveToLevelWithOnOff(device, newlevel, 0))

end

else

local newlevel = math.floor((current_level/100.0) * 254 )

device:send_to_component('light', Level.server.commands.MoveToLevelWithOnOff(device, newlevel, 0))

end

local newlevel = math.floor((current_level/100.0) * 254 )

if old_trim < new_trim then

if new_trim >= current_level then

local newlevel = math.floor((new_trim/100.0) * 254 )

end

device:send_to_component('light', Level.server.commands.MoveToLevelWithOnOff(device, newlevel, 0))

I modified it with the code you suggested

Edit with suggested code

greens · 2024-03-27T17:51:36Z

drivers/SmartThings/zigbee-fan/src/kichler-fan-light/init.lua

+      if old_trim < new_trim then
+        if new_trim >= current_level then
+          local newlevel = math.floor((new_trim/100.0) * 254 )
+          device:send_to_component('light', Level.server.commands.MoveToLevelWithOnOff(device, newlevel, 0))
+        else
+          device:send_to_component('light', Level.server.commands.MoveToLevelWithOnOff(device, newlevel, 0))
+        end
+      else
+        device:send_to_component('light', Level.server.commands.MoveToLevelWithOnOff(device, newlevel, 0))
+      end


Suggested change

if old_trim < new_trim then

if new_trim >= current_level then

local newlevel = math.floor((new_trim/100.0) * 254 )

device:send_to_component('light', Level.server.commands.MoveToLevelWithOnOff(device, newlevel, 0))

else

device:send_to_component('light', Level.server.commands.MoveToLevelWithOnOff(device, newlevel, 0))

end

else

device:send_to_component('light', Level.server.commands.MoveToLevelWithOnOff(device, newlevel, 0))

end

if old_trim < new_trim then

if new_trim >= current_level then

newlevel = math.floor((new_trim/100.0) * 254 )

end

end

device:send_to_component('light', Level.server.commands.MoveToLevelWithOnOff(device, newlevel, 0))

I changed it with the code you suggested.

greens · 2024-03-27T20:38:01Z

drivers/SmartThings/zigbee-fan/profiles/kichler-fan-light.yml

+  - name: "trim"
+    title: "Minimum Dimming Level"
+    description: "Set the lowest dimming level"
+    required: false
+    preferenceType: enumeration
+    definition:
+      options:
+        10 : "10%"
+        15 : "15%"
+        20 : "20%"
+        25 : "25%"
+      default: 10


In relation to this preference, I'm happy to direct you to this new feature we added, which is the ability to set min/max values for the switch level attribute. You may find that using this is simpler than having to do the trimming yourself.

Here's an example of how to emit this event:

https://github.com/SmartThingsCommunity/SmartThingsEdgeDrivers/pull/1220/files#diff-7ab55ddbc6a9760ea5b6f928abee244c3142c74389743ce05b895b938ad81f10R372

The min/max should be displayed and carried across automations as well.

It is expected that there will be models derived from this model. We will try to use the code you suggested at that time. Thank you.

I changed it with the code you suggested.

greens

This looks good but could you write a test that verifies the setlevel command after setting the trim above 0?

Similarly a test that checks the events that are sent after the trim is set above the current level of the device?

We prefer to have code coverage over 90% and you're currently at 87%.

updated test code

Update init code

seungkwan-choi · 2024-03-29T06:28:46Z

The test code has been modified.

greens · 2024-04-01T20:33:07Z

drivers/SmartThings/zigbee-fan/src/kichler-fan-light/init.lua

+      local time_difference = os.difftime(current_fandirection_time, send_fandirection_time) or 0
+      if time_difference >= 10 or send_fandirection_time == 0 then
+        device:send(FanControl.attributes.FanMode:write(device, 6))
+        device:set_field('FANDIRECTION_SENDTIME', current_fandirection_time, {persist = true})


this value doesn't need to persist

it takes more than 10s for the hub to reboot

The purpose of using the following code, regardless of the hub, is to prevent sending commands if a command has been sent within 10 seconds. (This is related to protecting the fan driving circuit.)

I understand, but the persist = true makes the value durable through hub restarts.

Because hub restarts take more than 10s, you do not need to have this flag persist, because it is guaranteed to have been more than 10s since you last sent the write on hub restart. Just keeping it in memory should be enough.

greens · 2024-04-01T20:34:16Z

drivers/SmartThings/zigbee-fan/src/kichler-fan-light/init.lua

+        device:send(FanControl.attributes.FanMode:write(device, 6))
+        device:set_field('FANDIRECTION_SENDTIME', current_fandirection_time, {persist = true})
+      else
+        device:set_field('FANDIRECTION_FLAG', flag_direction + 1 , {persist = true})


it's unclear what this flag does. you don't use the value except here where you increment it

The test code has been modified.

240402_Update init

Update init

240403 update init file

seungkwan-choi added 6 commits March 19, 2024 12:03

Update fingerprints.yml

40b7ee0

Add fingerprints for KICHLER

Create kichler-fan-light.yml

539e8a0

Create Profiles for kichler-fan-light

Update init.lua

97cc748

Add update init for Kichler

Update configurations.lua

80b525a

Add update configurations for Kichler

Create init.lua

54b7e0b

Add create init for Kichler fan light

Create test_kichler_fan_light.lua

d67eda5

Add create test kichler fan light

seungkwan-choi changed the title ~~Requset to add a fingerpirnts for Kichler Zigbee fan light~~ Requset to add a fingerprints for Kichler Zigbee fan light Mar 19, 2024

lelandblue added the do not merge label Mar 19, 2024

lelandblue requested review from greens, cbaumler and wkhenon March 22, 2024 13:37

lelandblue added the WWST label Mar 22, 2024

greens reviewed Mar 22, 2024

View reviewed changes

Update init.lua

0d63cf1

Update init file

greens reviewed Mar 26, 2024

View reviewed changes

Update init.lua

a8d81ff

Edit with suggested code

greens reviewed Mar 27, 2024

View reviewed changes

Update init.lua

3427a9d

I changed it with the code you suggested.

greens reviewed Mar 28, 2024

View reviewed changes

seungkwan-choi added 2 commits March 29, 2024 15:25

Update test_kichler_fan_light.lua

0bff912

updated test code

Update init.lua

6d25974

Update init code

greens reviewed Apr 1, 2024

View reviewed changes

seungkwan-choi and others added 3 commits April 2, 2024 10:14

Update init.lua

3e08669

240402_Update init

Update init.lua

367114a

Update init

Merge branch 'main' into main

bdbf638

wkhenon approved these changes Apr 2, 2024

View reviewed changes

Update init.lua

584c4a9

240403 update init file

greens approved these changes Apr 3, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Requset to add a fingerprints for Kichler Zigbee fan light #1278

Requset to add a fingerprints for Kichler Zigbee fan light #1278

seungkwan-choi commented Mar 19, 2024 •

edited

Loading

github-actions bot commented Mar 19, 2024

github-actions bot commented Mar 19, 2024

github-actions bot commented Mar 19, 2024 •

edited

Loading

github-actions bot commented Mar 19, 2024 •

edited

Loading

greens Mar 22, 2024

seungkwan-choi Mar 26, 2024

greens Mar 22, 2024

seungkwan-choi Mar 26, 2024

greens Mar 26, 2024

seungkwan-choi Mar 26, 2024

greens Mar 22, 2024

seungkwan-choi Mar 26, 2024

greens Mar 26, 2024

seungkwan-choi Mar 27, 2024

greens Mar 27, 2024

seungkwan-choi Mar 28, 2024

greens commented Mar 22, 2024

greens Mar 26, 2024

seungkwan-choi Mar 27, 2024

greens Mar 27, 2024

seungkwan-choi Mar 28, 2024 •

edited

Loading

greens Mar 27, 2024

seungkwan-choi Mar 28, 2024

greens left a comment

seungkwan-choi commented Mar 29, 2024

greens Apr 1, 2024

seungkwan-choi Apr 2, 2024

greens Apr 2, 2024

greens Apr 1, 2024

seungkwan-choi Apr 2, 2024

		local last_level = device:get_field('LAST_DIM_LEVEL') or 100
		local level = math.floor((last_level/100.0) * 254 )

Requset to add a fingerprints for Kichler Zigbee fan light #1278

Are you sure you want to change the base?

Requset to add a fingerprints for Kichler Zigbee fan light #1278

Conversation

seungkwan-choi commented Mar 19, 2024 • edited Loading

github-actions bot commented Mar 19, 2024

github-actions bot commented Mar 19, 2024

github-actions bot commented Mar 19, 2024 • edited Loading

Test Results

github-actions bot commented Mar 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greens commented Mar 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seungkwan-choi Mar 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greens left a comment

Choose a reason for hiding this comment

seungkwan-choi commented Mar 29, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

seungkwan-choi commented Mar 19, 2024 •

edited

Loading

github-actions bot commented Mar 19, 2024 •

edited

Loading

github-actions bot commented Mar 19, 2024 •

edited

Loading

seungkwan-choi Mar 28, 2024 •

edited

Loading