[airgradient] Support measures added for firmwares up to v3.1.21 - #18430
[airgradient] Support measures added for firmwares up to v3.1.21#18430austvik wants to merge 13 commits into
Conversation
439c301 to
47981f7
Compare
lsiepel
left a comment
There was a problem hiding this comment.
Seems like a nice addition 15 new channels. Besides the comment, could you:
- add the new channels to the documentation/ readme.md
- add the upgrade instructions if the channels are not created dynamically (i guess they are not)
- regenerate the properteis file foir i18n
|
Gentel ping @austvik, would be nice to have these channels included. oh5 code freeze is sunday, If you want this added please update the PR. |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for new measures and fields introduced in AirGradient firmware versions up to v3.1.21, expanding the available sensor data that can be collected from devices.
Key changes:
- Added support for new particulate matter measurements (standard PM values, compensated values, and particle counts)
- Added support for TVOC and NOx measurements with both index and raw values
- Added compensated temperature and humidity readings
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| RemoteApiControllerTest.java | Added test data and test case for firmware v3.1.21 format |
| AirGradientLocationHandlerTest.java | Added test measure object and test case for v3.1.21 features |
| thing-types.xml | Added channel definitions and NOx channel type for new measurements |
| Measure.java | Added new fields for firmware v3.1.21 measurements and firmware field handling |
| MeasureHelper.java | Added state mapping for all new measurement channels |
| AirGradientBindingConstants.java | Added channel constants for new measurement types |
Comments suppressed due to low confidence (3)
bundles/org.openhab.binding.airgradient/src/main/java/org/openhab/binding/airgradient/internal/model/Measure.java:232
- The field name 'pm50Count' appears to be inconsistent with the naming pattern. Based on the context of other PM fields, this should likely be 'pm05Count' to represent PM0.5 particles, not PM50.
public Double pm50Count;
bundles/org.openhab.binding.airgradient/src/main/java/org/openhab/binding/airgradient/internal/AirGradientBindingConstants.java:68
- The channel name 'CHANNEL_PM50_COUNT' is inconsistent with the naming pattern. Based on the context and the pm005Count field, this should likely be 'CHANNEL_PM05_COUNT' to represent PM0.5 particles.
public static final String CHANNEL_PM50_COUNT = "pm50-count";
bundles/org.openhab.binding.airgradient/src/main/java/org/openhab/binding/airgradient/internal/handler/MeasureHelper.java:100
- The mapping uses 'CHANNEL_PM50_COUNT' and 'pm50Count' which appears inconsistent with the naming pattern. Based on other PM measurements, this should likely reference PM0.5 particles, not PM50.
states.put(CHANNEL_PM50_COUNT, toQuantityType(measure.pm50Count, Units.ONE));
|
Ping @austvik are you able to proceed? |
Catching up with changes to the format introduced by firmware updates. Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
Signed-off-by: Jørgen Austvik <jaustvik@acm.org>
47981f7 to
273eff3
Compare
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
|
The review comments i had where addressed by myself as this PR became stale. |
kaikreuzer
left a comment
There was a problem hiding this comment.
I left a couple of inline notes from a local review.
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
wborn
left a comment
There was a problem hiding this comment.
Thanks for picking this up again. I reviewed the current head against the AirGradient API and openHAB's Thing update mechanism.
Most of the new measurement parsing and state mapping looks good, but I think there are still two functional issues before this can be merged. Both were already raised in an earlier review and the corresponding threads were marked resolved, but the fixes don't appear to be present in the current head.
- The new measurement channels are still not available on newly created Things. The migration in
update.xmladds them to existing v1 Things, but the channels remain commented out forairgradient-local, are absent fromlocation, andDynamicChannelHelperonly adds configuration channels. Measure.getFirmwareVersion()supports the newfirmwarefield, but normal property updates still readmeasure.firmwareVersiondirectly.
The parsing/state tests look useful, but they don't currently cover these integration paths.
This review was AI-assisted; I checked the findings against the current PR code, the existing review discussion, the openHAB Thing update mechanism, and the AirGradient API documentation.
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
wborn
left a comment
There was a problem hiding this comment.
Review summary
Thanks for reworking the dynamic channel handling. The firmware-property issue from the previous review is now addressed, including test coverage, and the new measurement-channel handling fixes the original problem of newly created Things not receiving these channels.
There are still a few issues with the new implementation:
- Newly created v2 Things get dynamically created measurement channels with incorrect channel-type UIDs. Existing v1 Things migrated through
update.xmlget the correct types, but the dynamic path currently assumes the channel ID is also the channel-type ID. - Migrated and newly created Things can end up with different channel sets. The v1 → v2 migration adds all measurement channels, while the dynamic path only adds channels supported by the actual measurement response.
- Local polling now retrieves the configuration before publishing measurements, so a failure of the separate configuration request prevents an otherwise valid measurement response from being processed.
The previous firmware-property thread is addressed and can be resolved. The dynamic-channel thread should remain open until the remaining dynamic-channel issues are addressed.
This is an AI re-review of the current PR head. A human maintainer review is still required after the remaining findings are addressed.
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
wborn
left a comment
There was a problem hiding this comment.
Re-reviewed the current head after the latest dynamic-channel changes. The previous issues around firmware-property handling, measurement channel-type mappings, migration behavior, and processing measurements when the configuration request fails are addressed.
Two functional issues remain in the dynamic-channel implementation:
- capability detection is based on fields present in an individual measurement, but the result is cached using only firmware/model. AirGradient can omit temporarily invalid measurements while keeping firmware/model unchanged, so a channel missed during the first scan may never be created later;
rhum-compensatedis created withNumber:Humidity, while itssystem:atmospheric-humiditychannel type usesNumber:Dimensionless.
There is also a smaller documentation mismatch around the meaning of the pm*Standard fields.
CI is green, and the earlier review findings look resolved, but the two dynamic-channel issues should be addressed before merging.
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
wborn
left a comment
There was a problem hiding this comment.
Re-reviewed the current head after the latest fixes. The channel-type/Item-type mappings, migration behavior, configuration failure handling, and documentation issues from the previous review are now addressed.
One functional issue remains in the dynamic measurement-channel detection. Channel support is inferred from the fields present in an individual measurement response, but the result is cached using only firmware and model.
AirGradient can omit measurements such as TVOC/NOx while their current values are invalid. If such a field is missing during the initial scan and becomes available on a later poll with the same firmware/model, the dynamic-channel scan is skipped and the channel is not created.
I think the cache should represent the capabilities actually observed, or the inexpensive dynamic-channel scan could simply run for every measurement and rely on the existing no-op behavior when no channels need to be added.
CI is green and the other previous findings look resolved, but I think this remaining capability-detection issue should be addressed before merging.
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
wborn
left a comment
There was a problem hiding this comment.
Re-reviewed the current head after the latest dynamic-channel change. The remaining capability-detection issue is now addressed: measurement channels are checked for every measurement in both handlers, while Thing updates are still avoided when no new channel needs to be added. The new local-handler regression test also covers a measurement field appearing on a later poll with unchanged firmware/model.
The previous findings around channel-type/Item-type mappings, migration behavior, configuration failure handling, firmware-property handling, and measurement descriptions remain resolved. CI is green.
There are two minor non-blocking findings: the README currently overstates the minimum firmware version for all of the additional measurement channels, and the latest static-analysis report contains two newly introduced low-priority MethodNameCheck findings for the new v3.1.21 test methods.
AI found no further blocking issues in the current changes. A human maintainer should still review the PR before merge.
This review was AI-assisted.
AI found no further blocking issues in the current changes. A human maintainer review is still needed before merge.
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
wborn
left a comment
There was a problem hiding this comment.
Previous findings are resolved; no blocking issues remain.
This review was AI-assisted.
|
This seems to have been a review coverage blind spot. I recently tightened the AI review policy to inspect changed code more systematically in its surrounding context, which is why the interaction with the pre-existing compensated-value behavior surfaced only now. |
Signed-off-by: Leo Siepel <leosiepel@gmail.com>
|
Anything left here @wborn? |
Catching up with changes to the format introduced by firmware updates.