Repository navigation
[WWSTCERT-13550] Sonoff - Add the pressure display function to SNZB-02M - #3194
laity-w-sudo wants to merge 14 commits into
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
Profile category check: ✅ Passed - all profiles have a category defined. |
|
Invitation URL: |
Test Results 73 files 545 suites 0s ⏱️ Results for commit 0371b3b. ♻️ This comment has been updated with latest results. |
|
zigbee-humidity-sensor_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against 0371b3b |
The SNZB-02M sub-driver was added but never registered, so the pressure display feature was dead code on the device. Register it via sub_drivers.lua, drop the unrelated SNZB-02LD/WD handler and utils (not in this driver, and referencing a nonexistent sub-driver), and add unit tests for pressure conversion and refresh. Co-Authored-By: Claude <noreply@anthropic.com>
ae161d5 to
b5e3665
Compare
cjswedes
left a comment
There was a problem hiding this comment.
My biggest concern is the lack of configuration for reporting the PressureMeasurement MeasuredValue attribute; has this been tested manually?
There was a problem hiding this comment.
Is there a reason to have the nested directories? We should just add a sonoff subdriver rather than having sonoff/SNZB-02M. We want to avoid having single subdrivers for each model.
There was a problem hiding this comment.
Please change the directory structure so that it is not nested, and everything falls under the sonoff directory.
|
It looks like temperature, humidity, and battery reporting are not configured specifically for this model. So the default configs are used. The defaults are coarser than what's configured for the |
| minimum_interval = 5, | ||
| maximum_interval = 3600, | ||
| data_type = PressureMeasurement.attributes.MeasuredValue.base_type, | ||
| reportable_change = 50 |
There was a problem hiding this comment.
The PressureMeasurement.MeasuredValue attribute is in units of 0.1 kPa. So 50 means 5 kPa. Normal barometric changes are roughly 0.1–0.5 kPa per hour. So the device would almost never report on change. Can you confirm if this was tested on a real device and if this setting is desired?
| device:add_configured_attribute(PRESSURE_CONFIGURATION) | ||
| end | ||
|
|
||
| local function refresh_handler(driver, device, command) |
There was a problem hiding this comment.
This handler looks redundant. I removed it locally, and the default refresh (device:refresh()) sends the same four reads: temperature, humidity, pressure (configured via add_configured_attribute) and battery percentage. The existing refresh test still passes unchanged with relaxed ordering. Could you drop refresh_handler and its capability_handlers entry unless there’s a read the default misses?
| device:emit_event(capabilities.atmosphericPressureMeasurement.atmosphericPressure({value = pressure_value, unit = "kPa"})) | ||
| end | ||
|
|
||
| local function device_init(driver, device) |
There was a problem hiding this comment.
nit: A subdriver’s lifecycle_handlers.init replaces the parent’s device_init for this model instead of running alongside it. That’s harmless today, because configurations.lua has no SNZB-02M entry. If one is added later, its attributes will silently stop being configured. A one-line comment here would save the next person some debugging.
| local function pressure_report_handler(driver, device, value, zb_rx) | ||
| local pressure_value = convert_pressure_value(value.value) | ||
| if pressure_value == nil then | ||
| log.warn(string.format("SNZB-02M pressure report has no value: %s", tostring(value))) |
There was a problem hiding this comment.
The nil branch (log and return) has no test. Either add a small test for a report with no value, or drop the nil handling, since the framework always passes a value for this attribute.
| direction = "receive", | ||
| message = { | ||
| mock_device.id, | ||
| PressureMeasurement.attributes.MeasuredValue:build_test_attr_report(mock_device, 960) |
There was a problem hiding this comment.
The only pressure test uses 960. Could you add a second case with a non-round value, such as 1013, so the division by 10 is covered more than once?
| } | ||
| ) | ||
|
|
||
| test.run_registered_tests() |
There was a problem hiding this comment.
I don’t see a test that covers the pressure reporting configuration, which was @cjswedes's main concern earlier in the review. Please add one that checks device_init adds the pressure configuration and that doConfigure sends the bind and configure-reporting messages for cluster 0x0403.
There was a problem hiding this comment.
Lets remove sonoff from this profile name (and file name), so that other devices can use it if there are any others.
Check all that apply
Type of Change
Checklist
Description of Change
Adding the air pressure function to the SNZB-02M equipment
Summary of Completed Tests