WWSTCERT-13068 - add aeotec 8 series new - #2897
iot-holding wants to merge 20 commits into
Conversation
|
Duplicate profile check: Passed - no duplicate profiles detected. |
|
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 20da095 |
|
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. |
| 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.
There was a problem hiding this comment.
When I use the standard handler, which converts the values from m/s² to mG, the converted values fall outside the allowed range of min: -10000 and max: 10000, which causes an error. That’s why I tried the custom handler.
There was a problem hiding this comment.
Please check that the values with this handler make sense. I am concerned by the lack of conversion; however, it could be that our default is incorrect in the conversion. If this is what is needed, I would ask that you leave a comment in this function explaining how this is the only part that differs from the defaults because of the units/conversion
| 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
There was a problem hiding this comment.
This has not been resolved.
|
@iot-holding Please check latest remarks. Thank You. |
| profile = t_utils.get_profile_definition("aeotec-door-window-sensor-8.yml"), | ||
| zwave_endpoints = sensor_endpoints, | ||
| zwave_manufacturer_id = 0x0371, | ||
| zwave_product_id = 0x0037, |
| 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.
This has not been resolved.
| supported_capabilities = { | ||
| capabilities.powerSource, | ||
| capabilities.threeAxis, | ||
| }, |
There was a problem hiding this comment.
| supported_capabilities = { | |
| capabilities.powerSource, | |
| capabilities.threeAxis, | |
| }, |
This doesnt actually do anything when put in a subdriver; it is used by the top level driver when registering for default functionality. IMO it is misleading to put it here.
| device:set_field("active_profile", profile.profile, {persist = true}) | ||
|
|
||
| -- Set supported modes and default value based on profile | ||
| if profile.profile == "aeotec-water-sensor-8" then |
There was a problem hiding this comment.
I should have caught this in the first pass, but emitting these events immediately after calling try_update_metadata means any capability that wasnt already on the old profile will not have events emitted. The profile is not immediately updated; the indication that it has been updated is an infoChanged event with the new profile capabilities on the device. Move the initial event emission to the infoChanged handler.
| local function added_handler(driver, device) | ||
| -- Get parameter 10 to switch device profile bsaed on the parameter value | ||
| -- Get parameter 10 to switch device profile based on the parameter value | ||
| device:send(Configuration:Get({ parameter_number = 10 })) |
There was a problem hiding this comment.
Can the user change this configuration value? It is only being read once after the device is onboarded, so there is no way for it to affect the device profiles afterwards.
| supported_capabilities = { | ||
| capabilities.powerSource, | ||
| capabilities.threeAxis, | ||
| }, |
|
|
||
| -- Z-Wave: m/s²; SmartThings threeAxis: mG | ||
| local mg = utils.round(value / 9.81 * 1000) | ||
| mg = math.max(-10000, math.min(10000, mg)) |
There was a problem hiding this comment.
The conversion is now the same as the default; the only difference is the clamp. If the device is sending values > 98.1 m/s^2 then perhaps the device is actually reporting in different units, and the clamping is misleading. I do not believe the device is actually being accelerated that fast. The max/min are in place on the capability to prevent unrealistic numbers, and as such we should understand why we are receiving unrealistic values instead of covering up the value by clamping it to the max value.
6 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