WWSTCERT-13068 - add aeotec 8 series new - #2897
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
Test Results 73 files 542 suites 0s ⏱️ Results for commit e1705ad. ♻️ This comment has been updated with latest results. |
|
Minimum allowed coverage is Generated by 🐒 cobertura-action against a01294e |
|
Invitation URL: |
There was a problem hiding this comment.
This subdriver is unnecessary. The aeotec-door-window-sensor-8 handlers cover everything here. Remove this driver, and have the device be handled by that one.
|
I will re-review once the tests are passing. Ill also request that you test your device on a real hub to ensure its functionality is what you expect. |
KKlimczukS
changed the title
There was a problem hiding this comment.
This file must also have a weird name
|
@iot-holding once you resolve the branch conflicts and ensure the tests are passing, we can re-review this PR. |
|
@cjswedes @aleclorimer could you re-review this? |
There was a problem hiding this comment.
This can handle never returns true and no longer loads the sub_driver when returning.
aleclorimer
left a comment
aleclorimer
left a comment
There was a problem hiding this comment.
Look like a couple of @cjswedes comments were still not addressed and a lot of driver tests are broken.
|
Hi @iot-holding, let us know when the comments/tests have been addressed and you're ready for us to re-review. |
|
zwave-sensor_coverage.xml
Minimum allowed coverage is Generated by 🐒 cobertura-action against e1705ad |
|
Profile category check: ✅ Passed - all profiles have a category defined. |
|
I apologize for the delay. All open issues should have been addressed in the latest commits. If there are still any issues, please let me know. |
KKlimczukS
changed the title
|
Hi @iot-holding, thanks for making the changes. Could you update this branch from main and we'll re-review. |
| if cmd.args.event == Notification.event.weather_alarm.STATE_IDLE then | ||
| event = MoldHealthConcern.moldHealthConcern.good() | ||
| elseif cmd.args.event == Notification.event.weather_alarm.MOISTURE_ALARM then | ||
| event = MoldHealthConcern.moldHealthConcern.moderate() |
There was a problem hiding this comment.
The defaults use moldHealthConcern.unhealthy() for MOISTURE_ALARM event. Is there a reason for diverging for never emitting unhealthy?
| local event | ||
| local event_parameter | ||
|
|
||
| if (0 ~= string.len(cmd.args.event_parameter)) then |
There was a problem hiding this comment.
I think a defensive nil check and early return for cmd.args.event_parameter is warranted since it is an optional field in the spec.
| if (sensor_type == SensorMultilevel.sensor_type.ACCELERATION_X_AXIS) then | ||
| x = value | ||
| device:set_field("three_axis_x", x) | ||
| event = ThreeAxis.threeAxis({value = {x, y, z}, unit = 'mG'}) | ||
| elseif (sensor_type == SensorMultilevel.sensor_type.ACCELERATION_Y_AXIS) then | ||
| y = value | ||
| device:set_field("three_axis_y", y) | ||
| event = ThreeAxis.threeAxis({value = {x, y, z}, unit = 'mG'}) | ||
| elseif (sensor_type == SensorMultilevel.sensor_type.ACCELERATION_Z_AXIS) then | ||
| z = value | ||
| device:set_field("three_axis_z", z) | ||
| event = ThreeAxis.threeAxis({value = {x, y, z}, unit = 'mG'}) | ||
| end |
There was a problem hiding this comment.
This is the only part of the sensor_multilevel_report_handler that is not default functionality. It differs in the device fields it uses, and most importantly in the unit conversion (there is none here, whereas the default handler converts from ms^2 to mG.
Is this device reporting acceleration in mG or in m/s^2? The spec dictates it is reported in m/s^2 so I would expect that is the unit being used by the device. In that case, this function is not needed, and the default should be used instead.
| profile = t_utils.get_profile_definition("aeotec-door-window-sensor-8.yml"), | ||
| zwave_endpoints = sensor_endpoints, | ||
| zwave_manufacturer_id = 0x0371, | ||
| zwave_product_id = 0x0037, |
There was a problem hiding this comment.
missing product type in the mock.
| test.mock_device.add_test_device(mock_co_sensor) | ||
| test.mock_device.add_test_device(mock_co2_sensor) | ||
| test.mock_device.add_test_device(mock_contact_sensor) | ||
| test.mock_device.add_test_device(mock_contact_sensor) |
There was a problem hiding this comment.
mock_contact_sensor is added twice unnecessarily
| options: | ||
| 0: "Celsius" | ||
| 1: "Fahrenheit" | ||
| default: 0 No newline at end of file |
There was a problem hiding this comment.
This and all other profile files should end with a newline.
5 participants
Check all that apply
Type of Change
Checklist
Description of Change
This is a new clean pull request for the new Aeotec Series 8 devices.
Summary of Completed Tests