WWSTCERT-11482 & WWSTCERT-11485: Add support to frient keypad#2927
WWSTCERT-11482 & WWSTCERT-11485: Add support to frient keypad#2927marcintyminski wants to merge 25 commits intoSmartThingsCommunity:mainfrom
Conversation
…cintyminski/SmartThingsEdgeDrivers into add-support-to-frient-keypad
…cintyminski/SmartThingsEdgeDrivers into add-support-to-frient-keypad
Co-authored-by: Copilot <copilot@github.com>
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Invitation URL: |
Test Results 73 files 509 suites 0s ⏱️ Results for commit f12466e. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against f12466e |
| version: 1 | ||
| categories: | ||
| - name: SecurityPanel | ||
| preferences: |
There was a problem hiding this comment.
@marcintyminski Have you considered adding a preference to enable and disable panicAlarm events? (scenario when the keypad is outside)
There was a problem hiding this comment.
It is added now
Co-authored-by: Copilot <copilot@github.com>
cjswedes
left a comment
There was a problem hiding this comment.
Even though this isnt explicitly a door lock, it belongs in the zigbee-lock driver. We do not want to create a new driver that will only handle a small number of devices, because it negatively impacts hub memory more than adding devices to an existing driver. Please make this a subdriver of the zigbee-lock driver.
| end | ||
|
|
||
| local function handle_arm_command(driver, device, zb_rx) | ||
| armCommandFromKeypad = true |
There was a problem hiding this comment.
Using a module level variable to track device state breaks down if there are more than 1 device using the driver. This should probably be a transient (persisted = false) on the device object.
There are also several early returns, where the value would not be unset, leaving the state tracking inconsistent for the device. This can result in commands being blocked indefinitely after one fails.
|
|
||
| local function send_panel_status(device, status) | ||
| local duration = get_exit_delay_duration(device) | ||
| local panel_status = STATUS_TO_PANEL[status] or PanelStatus.PANEL_DISARMED_READY_TO_ARM |
There was a problem hiding this comment.
This is a little confusing. STATUS_TO_PANEL is defined as
local STATUS_TO_PANEL = {
armedAway = PanelStatus.ARMED_AWAY,
armedStay = PanelStatus.ARMED_STAY,
disarmed = PanelStatus.PANEL_DISARMED_READY_TO_ARM,
exitDelay = PanelStatus.EXIT_DELAY,
}
But you call send_panel_status with both Unlocked as the status, which default to PANEL_DISARMED_READY_TO_ARM; is that intentional? There are several other possible values that could be fed into send_panel_status that do not have mappings in that constant.
| local SECURITY_STATUS_EVENTS = { | ||
| armedAway = SecuritySystem.securitySystemStatus.armedAway, | ||
| armedStay = SecuritySystem.securitySystemStatus.armedStay, | ||
| disarmed = SecuritySystem.securitySystemStatus.disarmed, | ||
| } | ||
|
|
||
| local MODE_STATUS_VALUES = { | ||
| Locked = "Locked", | ||
| Unlocked = "Unlocked", | ||
| } | ||
|
|
||
| local ARM_MODE_TO_STATUS = { | ||
| [ArmMode.DISARM] = "disarmed", | ||
| [ArmMode.ARM_DAY_HOME_ZONES_ONLY] = "armedStay", | ||
| [ArmMode.ARM_NIGHT_SLEEP_ZONES_ONLY] = "armedStay", | ||
| [ArmMode.ARM_ALL_ZONES] = "armedAway", | ||
| } | ||
|
|
||
| local ARM_MODE_TO_NOTIFICATION = { | ||
| [ArmMode.DISARM] = ArmNotification.ALL_ZONES_DISARMED, | ||
| [ArmMode.ARM_DAY_HOME_ZONES_ONLY] = ArmNotification.ONLY_DAY_HOME_ZONES_ARMED, | ||
| [ArmMode.ARM_NIGHT_SLEEP_ZONES_ONLY] = ArmNotification.ONLY_NIGHT_SLEEP_ZONES_ARMED, | ||
| [ArmMode.ARM_ALL_ZONES] = ArmNotification.ALL_ZONES_ARMED, | ||
| } | ||
|
|
||
| local STATUS_TO_PANEL = { | ||
| armedAway = PanelStatus.ARMED_AWAY, | ||
| armedStay = PanelStatus.ARMED_STAY, | ||
| disarmed = PanelStatus.PANEL_DISARMED_READY_TO_ARM, | ||
| exitDelay = PanelStatus.EXIT_DELAY, | ||
| } | ||
|
|
||
| local STATUS_TO_ACTIVITY = { | ||
| armedAway = "armed away", | ||
| armedStay = "armed stay", | ||
| disarmed = "disarmed", | ||
| exitDelay = "exit delay", | ||
| } | ||
|
|
||
| local LOCK_STATUS_TO_ACTIVITY = { | ||
| Locked = "Locked", | ||
| Unlocked = "Unlocked", | ||
| } |
There was a problem hiding this comment.
These constants need some explanation and if possible, we should combine them. At the very least, please add a comment explaining what the mappings are used for to help anyone trying to understand all the different status, modes, activities, and notifications and what they mean for the user.
| device:send(device_management.build_bind_request(device, PowerConfiguration.ID, self.environment_info.hub_zigbee_eui)) | ||
| device:send(PowerConfiguration.attributes.BatteryVoltage:configure_reporting(device, 30, 21600, 1)) |
There was a problem hiding this comment.
| device:send(device_management.build_bind_request(device, PowerConfiguration.ID, self.environment_info.hub_zigbee_eui)) | |
| device:send(PowerConfiguration.attributes.BatteryVoltage:configure_reporting(device, 30, 21600, 1)) | |
| device:configure() |
Use the default configuration path to ensure that the default attributes are configured properly and that IASZone is setup properly for this device. I do not see anything in here that will setup the IASZone status attribute reporting or the zone status notifications.
There was a problem hiding this comment.
Please clean up commented code in the tests.
| end | ||
|
|
||
| local function refresh(driver, device) | ||
| device:send(PowerConfiguration.attributes.BatteryVoltage:read(device)) |
There was a problem hiding this comment.
| device:send(PowerConfiguration.attributes.BatteryVoltage:read(device)) | |
| device:refresh() |
Use default refresh behavior for the default attributes, and then do extra panel specific stuff.
Check all that apply
Type of Change
Checklist
Description of Change
Summary of Completed Tests