Repository navigation
WWSTCERT-13068 - add aeotec 8 series new - #2897
iot-holding wants to merge 24 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. |
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.
|
@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, |
| 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.
There was a problem hiding this comment.
Please remove supported_capabilities from the subdrivers.
| supported_capabilities = { | ||
| capabilities.powerSource, | ||
| capabilities.threeAxis, | ||
| }, |
| capabilities.carbonDioxideHealthConcern, | ||
| capabilities.soundDetection, | ||
| capabilities.panicAlarm | ||
| }, |
There was a problem hiding this comment.
Please remove the supported capabilities from the subdriver. This does not do anything at the subdriver level; it does at the top level driver for defaults registration and default driver switch behavior.
| end | ||
| end | ||
|
|
||
| local function info_changed_handler(driver, device, event, args) |
There was a problem hiding this comment.
There are several problems with this function as it is. info_changed is called for a lot of different reasons; anything changing on the device object (i.e. capabilities, preferences).
- The default events being emitted based on profile, will be emitted anytime infoChanged fire, which could override a valid state with the default state. It would be better to refresh the device to read the actual state (this could be done every infoChanged). We should avoid emitting default events in general because they often override valid state (only sometimes at onboarding time is it warranted).
- This new handler should call
preferences.update_preferences(self, device, args)which is what the default handler called to handle preference updates, without it, preference updates are not handled anymore.
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