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, | ||
| }, |
| 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.
| if param_number == 10 then | ||
| local mapping = DEVICE_PROFILES[value] | ||
| if mapping then | ||
| set_profile(device, mapping) | ||
|
|
||
| if mapping.profile == "aeotec-water-sensor-8-glass-break" then | ||
| device:emit_event( | ||
| SoundDetection.supportedSoundTypes({ | ||
| "noSound", | ||
| "glassBreaking" | ||
| }) | ||
| ) | ||
| elseif mapping.profile == "aeotec-water-sensor-8-co2" then | ||
| device:emit_event( | ||
| CarbonDioxideHealthConcern.supportedCarbonDioxideValues({ | ||
| "good", | ||
| "unhealthy" | ||
| }) | ||
| ) | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
This is still the issue from the earlier review. Emitting events right after try_update_metadata means any capability not already on the old profile never receives them. The new profile isn’t applied until an infoChanged arrives with the new capabilities on the device.
The emission moved out of added, but configuration_report_handler still calls set_profile() and then emits supportedSoundTypes / supportedCarbonDioxideValues straight away. On the glass-break and CO2 profiles, soundDetection and carbonDioxideHealthConcern don’t exist on the device yet at that point, so those events are dropped.
Please move the supported-value emission into info_changed_handler, guarded by capability support. Note that infoChanged fires for many reasons (preference changes and so on), so emit_event_if_latest_state_missing should probably be used to keep this from overwriting existing state. Please also add a test that sends the parameter 10 configuration report, delivers the infoChanged with the new profile, and checks that the supported-values events are emitted. The current tests pass, so they don’t exercise this flow.
It would also help to confirm on a real hub that the CO2 and glass-break profiles end up with the supported values populated.
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