add support for generic zigbee sensor #1921

pInksenberg · 2025-02-11T09:00:04Z

Check all that apply

Type of Change

WWST Certification Request
- If this is your first time contributing code:
  - I have reviewed the README.md file
  - I have reviewed the CODE_OF_CONDUCT.md file
  - I have signed the CLA
- I plan on entering a WWST Certification Request or have entered a request through the WWST Certification console at developer.smartthings.com
Bug fix
New feature
Refactor

Checklist

I have performed a self-review of my code
I have commented my code in hard-to-understand areas
I have verified my changes by testing with a device or have communicated a plan for testing
I am adding new behavior, such as adding a sub-driver, and have added and run new unit tests to cover the new behavior

Description of Change

Add support for generic Zigbee sensor base on IASZone type

Summary of Completed Tests

github-actions · 2025-02-11T09:00:48Z

Duplicate profile check: Passed - no duplicate profiles detected.

github-actions · 2025-02-11T09:01:33Z

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

github-actions · 2025-02-11T09:03:53Z

Test Results

65 files 410 suites 0s ⏱️
2 048 tests 2 045 ✅ 0 💤 3 ❌
3 533 runs 3 530 ✅ 0 💤 3 ❌

For more details on these failures, see this check.

Results for commit 7a4d856.

♻️ This comment has been updated with latest results.

github-actions · 2025-02-11T09:03:55Z

File	Coverage
All files	`76%`	❌
/home/runner/work/SmartThingsEdgeDrivers/SmartThingsEdgeDrivers/drivers/SmartThings/zigbee-sensor/src/init.lua	`76%`	❌

Minimum allowed coverage is 90%

Generated by 🐒 cobertura-action against 7a4d856

inasail · 2025-02-11T11:16:12Z

drivers/SmartThings/zigbee-sensor/fingerprints.yml

@@ -0,0 +1,8 @@
+zigbeeGeneric:
+  - id: "ias-generic-sensor"
+    deviceLabel: "IAS Zigbee Generic Sensor"


The label is a little bit difficult to understand. How about the 'Zigbee Safety Sensor"
@greens @varzac Do you have any good idea?

inasail · 2025-02-11T11:16:39Z

drivers/SmartThings/zigbee-sensor/profiles/ias-generic-button-profile.yml

@@ -0,0 +1,12 @@
+name: ias-generic-button-profile


If this profile is unnecessary, please remove.

inasail · 2025-02-11T11:18:14Z

drivers/SmartThings/zigbee-sensor/src/init.lua

+
+local ZIGBEE_GENERIC_SENSOR_PROFILE = "ias-generic-sensor"
+local ZIGBEE_GENERIC_CONTACT_SENSOR_PROFILE = "ias-generic-contact-sensor"
+local ZIGBEE_GENERIC_EMERGENCY_BUTTON_PROFILE = "ias-generic-emergency-button"


You can remove this because there is no any implementation about button functionality in this code.

inasail · 2025-02-11T11:18:44Z

drivers/SmartThings/zigbee-sensor/src/init.lua

+  local profile = ZIGBEE_GENERIC_SENSOR_PROFILE
+  if zone_type == Contact_Switch then
+    profile = ZIGBEE_GENERIC_CONTACT_SENSOR_PROFILE
+  elseif zone_type == Remote_Control or zone_type == Key_Fob or zone_type == Keypad then


You can remove this because there is no any implementation about button functionality in this code.

inasail · 2025-02-11T11:23:25Z

drivers/SmartThings/zigbee-sensor/src/init.lua

+    state_change = true
+  }
+
+  log.info("---zone_status: "..zone_status.value)


You can remove log.info() after you have finished the test.

inasail · 2025-02-11T11:40:10Z

drivers/SmartThings/zigbee-sensor/src/init.lua

+local ZONETYPE = "ZoneType"
+local log = require "log"
+
+local Contact_Switch = 21 -- 0x0015


You can use 0x0015 directly and use a capital letter when you define.
local CONTACT_SWITCH = 0x0015
local REMOTE_CONTROL = 0x010F

inasail · 2025-02-11T11:51:38Z

drivers/SmartThings/zigbee-sensor/profiles/ias-generic-contact-sensor.yml

+name: ias-generic-contact-sensor
+components:
+- id: main
+  capabilities:


Would you test the battery capability for Group2 devices?
and check the configureReport set for the powerConfiguration cluster. (don't forget to add capabilities.battery in supported_capabilities of init.lua.)
Sensor should report its battery state periodically.

varzac · 2025-02-11T13:38:35Z

drivers/SmartThings/zigbee-sensor/src/init.lua

+local ZIGBEE_GENERIC_MOTION_SENSOR_PROFILE = "ias-generic-motion-sensor"
+local ZIGBEE_GENERIC_WATERLEAK_SENSOR_PROFILE = "ias-generic-waterleak-sensor"
+
+-----add by dzd 20250124 start-----


these comments can be removed

varzac · 2025-02-11T13:52:45Z

drivers/SmartThings/zigbee-sensor/src/init.lua

+end
+
+-- since we don't have button devices using IASZone, the driver here is remaining to be updated
+local generate_event_from_zone_status = function(driver, device, zone_status, zb_rx)


Instead of having a single function here that that branches, the standard way to handle this in drivers is with subdrivers. So if you had a subdriver that looked like:

local capabilities = require "st.capabilities" local clusters = require "st.zigbee.zcl.clusters" local IASZone = clusters.IASZone local subdriver_template local generate_event_from_zone_status = function(driver, device, zone_status, zb_rx) if zone_status:is_alarm1_set() then device:emit_event(capabilities.contactSensor.contact.open()) else device:emit_event(capabilities.contactSensor.contact.closed()) end end local function subdriver_canhandle(opts, driver, device) -- Create a "fields.lua" to contain this string definition and contact switch constant return device:get_field("ZoneType") == 0x0015, subdriver_template end -- These functions could be moved to a common file as well. local ias_zone_status_attr_handler = function(driver, device, zone_status, zb_rx) generate_event_from_zone_status(driver, device, zone_status, zb_rx) end local ias_zone_status_change_handler = function(driver, device, zb_rx) generate_event_from_zone_status(driver, device, zb_rx.body.zcl_body.zone_status, zb_rx) end subdriver_template = { NAME = "Zigbee Contact", zigbee_handlers = { attr = { [IASZone.ID] = { [IASZone.attributes.ZoneStatus.ID] = ias_zone_status_attr_handler } }, cluster = { [IASZone.ID] = { [IASZone.client.commands.ZoneStatusChangeNotification.ID] = ias_zone_status_change_handler } } }, can_handle = subdriver_canhandle } return subdriver_template

There will be a bit more duplicate code, but the it allows us to separate all the different types into their own files, so if we need more complicated logic we don't end up with a big mess of a function.

@pInksenberg
For this, we need to check the ZoneType can be determined before the 'can_handle' triggered.

In this driver, we can seperate sub driver following zonetype, but this edge driver read zone type in add_handler. so this device wouldn't likely to handle some message in initial time potentially.

varzac · 2025-02-11T13:53:37Z

drivers/SmartThings/zigbee-sensor/src/init.lua

+  local additional_fields = {
+    state_change = true
+  }


You should not be setting state_change = true. That is only necessary, and should only be done for devices where they will emit the same value, but still need to be processed, like a button press.

inasail · 2025-02-12T03:08:48Z

drivers/SmartThings/zigbee-sensor/config.yml

@@ -0,0 +1,6 @@
+name: 'Zigbee Sensor'


@varzac @greens @tpmanley Is the name of this driver(zigbee-Sensor) okay? Should we add the IAS explicitly?
like 'zigbee-ias-sensor' and the device label also.(now, it is 'IAS Zigbee Generic Sensor')
I wonder your opinion.

I think it's OK as is

varzac · 2025-02-13T15:36:18Z

drivers/SmartThings/zigbee-sensor/profiles/ias-generic-contact-sensor.yml

+  capabilities:
+  - id: contactSensor
+    version: 1
+  - id: batteryLevel


battery level is different than battery. All other Zigbee devices use the battery capability and that's what the defaults support not batteryLevel

@varzac @tpmanley
SRCN tried to use the battery capability but the result was untrustworthy.
So they removed the battery capabilty at the beginning, but I think If we cannot accurately display battery percentage, showing battery level (normal/warning/critical) instead would greatly benefit users.
SRCN suggested to use the zoneStatus(zone_status:is_battery_low_set()) because these devices are working based on IAS zone cluster. It is nice suggestion. Now, batteryLevel has 3 conditions.(normal/warning/critical) and we can know two battery condition with zoneStatus(1 – Low battery / 0 – Battery OK), so I think we can match the 'low battery' state to 'warning' state in batteryLevel. What do you think of this?

I left a somewhat similar comment here: https://github.com/SmartThingsCommunity/SmartThingsEdgeDrivers/pull/1921/files#r1956324647.

Was the BatteryPercentageRemaining attribute found to not be trustworthy? We've found that usually works well, and the problems arise when trying to make BatteryVoltage to a percentage as that's very hard to do, especially for coin cell batteries that have a pretty flat voltage for most of their life and then quickly decrease at the end. I would suggest using BatteryPercentageRemaining and the battery capability and/or ZoneStatus low battery to indicate warning like you suggested (I don't have a strong preference whether it should be warning or critical).

FrankSpringfield · 2025-02-14T11:23:36Z

drivers/SmartThings/zigbee-sensor/src/test/test_zigbee_sensor.lua

+      -- test.mock_time.advance_time(50000)
+      -- test.socket.zigbee:__set_channel_ordering("relaxed")
+      -- test.socket.zigbee:__expect_send({ mock_device_waterleak_sensor.id, IASZone.attributes.ZoneStatus:read(mock_device_waterleak_sensor) })
+      -- test.wait_for_events()


Hi, @greens @varzac
I tried to add these three pieces of code in this file.
I found that this has relationship to the capabilities in supported_capabilities in init.lua.
However, when I add capabilities contactSensor, motionSensor and waterSensor in file init.lua (the commented-out code), I found that only the front one can work and the others failed. Why did this happen?

Can you clarify what the failure is. Looking at the driver, there is no code that will send a ZoneStatus read after a delay. If you are expecting the infoChanged to happen, in the test environment that will only happen if you write test code to generate it. In addition, I don't think sending a read on the infoChanged is correct behavior.

FrankSpringfield · 2025-02-14T11:24:42Z

drivers/SmartThings/zigbee-sensor/src/init.lua

+    capabilities.refresh
+    -- capabilities.motionSensor,
+    -- capabilities.contactSensor,
+    -- capabilities.waterSensor


Only the motionSensor here can work.

varzac · 2025-02-14T15:14:08Z

drivers/SmartThings/zigbee-sensor/src/init.lua

+end
+
+-- ask device to upload its zone status, then the status of capabilities can be synchronized
+local ias_info_changed = function(driver, device)


This function will be run every time the device is updated on the cloud side. You should not be reading an attribute here. If the intent is for this to happen at some point after the device is added, you should use the call_with_delay to have the driver wait some period and then send the read.

tpmanley · 2025-02-14T15:20:53Z

drivers/SmartThings/zigbee-sensor/src/init.lua

+      event = capabilities.contactSensor.contact.closed()
+    end
+  elseif type == MOTION_SENSOR then
+    if zone_status:is_alarm1_set() then


Motion sensors can use either alarm1 or alarm2 so I would suggest reporting active if either are set

tpmanley · 2025-02-14T15:22:49Z

drivers/SmartThings/zigbee-sensor/src/init.lua

+  local type = device:get_field(ZONETYPE)
+  local event
+  if type == CONTACT_SWITCH then
+    if zone_status:is_alarm1_set() then


The ZCL spec says that contact switches can use both alarm1 and alarm2 so I think both should be checked

tpmanley · 2025-02-14T15:26:52Z

drivers/SmartThings/zigbee-sensor/src/init.lua

+
+local battery_level_handler = function(driver, device, value, zb_rx)
+  local voltage = value.value
+  if voltage <= 25 then


It's pretty difficult to accurately translate voltage to battery level without knowing the details of the device and battery technology. I'd either suggest using the low battery indicator in the IAS Zone Status and/or switching from batteryLevel capability to battery and looking at the BatteryPercentageRemaining attribute instead of BatteryVoltage

Apologies, I just saw the similar comment here: #1921 (comment)

inasail reviewed Feb 11, 2025

View reviewed changes

varzac reviewed Feb 11, 2025

View reviewed changes

lelandblue requested review from cbaumler and wkhenon February 11, 2025 14:04

inasail reviewed Feb 12, 2025

View reviewed changes

pInksenberg force-pushed the add_support_for_generic_zigbee_sensor branch 3 times, most recently from 52d5eca to 7320dbe Compare February 13, 2025 11:08

varzac reviewed Feb 13, 2025

View reviewed changes

pInksenberg force-pushed the add_support_for_generic_zigbee_sensor branch from 7320dbe to 6771cba Compare February 14, 2025 10:48

add support for generic zigbee sensor

7a4d856

pInksenberg force-pushed the add_support_for_generic_zigbee_sensor branch from 6771cba to 7a4d856 Compare February 14, 2025 11:01

FrankSpringfield reviewed Feb 14, 2025

View reviewed changes

FrankSpringfield mentioned this pull request Feb 14, 2025

A test for automatically identify and provide suitable profile FrankSpringfield/SmartThingsEdgeDrivers#1

Closed

varzac reviewed Feb 14, 2025

View reviewed changes

tpmanley reviewed Feb 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for generic zigbee sensor #1921

add support for generic zigbee sensor #1921

pInksenberg commented Feb 11, 2025

github-actions bot commented Feb 11, 2025

github-actions bot commented Feb 11, 2025

github-actions bot commented Feb 11, 2025 •

edited

Loading

github-actions bot commented Feb 11, 2025 •

edited

Loading

inasail Feb 11, 2025 •

edited

Loading

inasail Feb 11, 2025

pInksenberg Feb 12, 2025

inasail Feb 11, 2025

pInksenberg Feb 12, 2025

inasail Feb 11, 2025

pInksenberg Feb 12, 2025

inasail Feb 11, 2025

pInksenberg Feb 12, 2025

inasail Feb 11, 2025

inasail Feb 11, 2025 •

edited

Loading

varzac Feb 11, 2025

varzac Feb 11, 2025 •

edited

Loading

inasail Feb 12, 2025

kdbang Feb 14, 2025

varzac Feb 11, 2025

inasail Feb 12, 2025

tpmanley Feb 14, 2025

varzac Feb 13, 2025

inasail Feb 14, 2025

tpmanley Feb 14, 2025

FrankSpringfield Feb 14, 2025

varzac Feb 14, 2025

FrankSpringfield Feb 14, 2025

varzac Feb 14, 2025

tpmanley Feb 14, 2025

tpmanley Feb 14, 2025

tpmanley Feb 14, 2025

tpmanley Feb 14, 2025

add support for generic zigbee sensor #1921

Are you sure you want to change the base?

add support for generic zigbee sensor #1921

Conversation

pInksenberg commented Feb 11, 2025

Type of Change

Checklist

Description of Change

Summary of Completed Tests

github-actions bot commented Feb 11, 2025

github-actions bot commented Feb 11, 2025

github-actions bot commented Feb 11, 2025 • edited Loading

Test Results

github-actions bot commented Feb 11, 2025 • edited Loading

inasail Feb 11, 2025 • 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

inasail Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

varzac Feb 11, 2025 • 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

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

github-actions bot commented Feb 11, 2025 •

edited

Loading

github-actions bot commented Feb 11, 2025 •

edited

Loading

inasail Feb 11, 2025 •

edited

Loading

inasail Feb 11, 2025 •

edited

Loading

varzac Feb 11, 2025 •

edited

Loading