-
Notifications
You must be signed in to change notification settings - Fork 3.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for JY-GZ-01AQ
and improve JT-BZ-01AQ/A
#4058
Conversation
case '163': | ||
payload.mute = value === 1; // JT-BZ-01AQ/A | ||
if (['JT-BZ-01AQ/A', 'JY-GZ-01AQ'].includes(model.model)) { | ||
payload.mute = value === 1; | ||
} | ||
break; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea with this utility library is to try to not limit by model the codes, if it's possible.
I think that until we know of other devices that use the 163, for example, for other things we must not make it conditional.
In this way some devices will mostly work simply adding the converter and we only need to add/remove the values that can be wrong. If we add a conditional for each device, then we will need to add all the models for each attribute.
That's my opinion. @Koenkk what's your idea? We must filter by model always or only when needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, Aqara allows itself not to adhere to standards, and even between its own devices, and this library is proof of that.
Examples:
-
in current PR, I had to add an exception for the JT-BZ-01AQ/A gas sensor to case 11, because it certainly does not have the ability to measure illuminance:
case '11': if (!['JT-BZ-01AQ/A'].includes(model.model)) { payload.illuminance = calibrateAndPrecisionRoundOptions(value, options, 'illuminance'); // DEPRECATED: remove illuminance_lux here. payload.illuminance_lux = calibrateAndPrecisionRoundOptions(value, options, 'illuminance_lux'); } break;
-
in current PR, I had to specify model in condition for cases 165 and 166 (both mean
linkage_alarm
), because gas sensor JT-BZ-01AQ/A has both of these cases, butlinkage_alarm
only corresponds to case 166, and 165 means something else (not clear yet):case '165': if (['JY-GZ-01AQ'].includes(model.model)) { payload.linkage_alarm = value === 1; } break; case '166': if (['JT-BZ-01AQ/A'].includes(model.model)) { payload.linkage_alarm = value === 1; } break;
-
in case 102 for the RTCZCGQ11LM presence sensor, the value changes depending on the firmware version:
zigbee-herdsman-converters/lib/xiaomi.js
Lines 254 to 267 in 2ae0857
case '102': if (['QBKG25LM', 'QBKG34LM'].includes(model.model)) { payload.state_right = value === 1 ? 'ON' : 'OFF'; } else if (['WSDCGQ01LM', 'WSDCGQ11LM'].includes(model.model)) { payload.pressure = calibrateAndPrecisionRoundOptions(value/100.0, options, 'pressure'); } else if (['RTCZCGQ11LM'].includes(model.model)) { if (meta.device.applicationVersion < 50) { payload.presence_event = {0: 'enter', 1: 'leave', 2: 'left_enter', 3: 'right_leave', 4: 'right_enter', 5: 'left_leave', 6: 'approach', 7: 'away', 255: null}[value]; } else { payload.motion_sensitivity = {1: 'low', 2: 'medium', 3: 'high'}[value]; } } break;
Therefore, I believe that you need to know exactly what each case means for a particular device and therefore explicitly indicate the model in the condition, otherwise there may be problems (for example, duplication of values).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't must fix it before is broken. It will complicate and make the code a lot bigger. I understand that we need to add exceptions when we encounter something wrong, but in my opinion we must left the others without conditional.
The worst thing that can happen is that some device will have something bad in the state, but if it does not expose it, it will not be published.
If we decide too filter by model for all the attributes, maybe we must try to move the big switch to methods, one for each attribute, to maintain it under control.
But again, all of this is only my opinion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Koenkk, I tried to add support for Aqara smoke detector (JY-GZ-01AQ) so that it would be included in today's zigbee2mqtt release, so if there are no fundamental objections to current PR, then please merge it so that it can be included in release.
If, according to remarks @McGiverGim, the conditions in some cases need to be removed, then if necessary, I will do this in a new PR after release of zigbee2mqtt.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, couldn't work on z2m yesterday and already did the 1.25.0 release this morning. Thanks for the changes!
Fixes Koenkk/zigbee2mqtt#11986
This converter is designed to work on coordinator firmware with support for native Aqara logic.
OTA file for
JY-GZ-01AQ
added with Koenkk/zigbee-OTA#101