-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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 concentration level sensor to Aranet #137291
base: dev
Are you sure you want to change the base?
Conversation
Hey there @aschmitz, @thecode, @Anrijs, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
status_sensor_attrs = status_sensor.attributes | ||
assert status_sensor.state == "GREEN" | ||
assert status_sensor_attrs[ATTR_FRIENDLY_NAME] == "Aranet4 12345 Threshold Level" | ||
assert set(status_sensor_attrs[ATTR_OPTIONS]) == {"ERROR", "GREEN", "YELLOW", "RED"} |
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'd love to map these to more useful values. For example, the CO₂ could have the following mappings:
- GREEN → Good
- YELLOW → Average
- RED → Unhealthy
For the Radon sensor:
- GREEN → Normal
- YELLOW → Elevated
- RED → Unhealthy
I'm just not sure if that would be considered in scope for this integration.
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.
From end-user viewpoint, it would make more sense to map them to more understandable names
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.
In that case, I'll work on implementing this. Thanks for the quick feedback!
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.
OTOH, isn't the user expecting to see "Red/Yellow/Green", based on the official app and device screen?
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.
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.
Personally, I would view the descriptive (Good/Average/Unhealthy) terms as more useful on a dashboard, so that would be my preference. I understand that they are not necessarily as obvious when looking at the device, though, so if there's a consensus in another direction that's fine too.
My only concern would be that they would possibly require some care when handling the translation strings (especially because this meaning of "average" isn't necessarily "the mean of some numbers"), but I'd find it more useful to see "Unhealthy" than "Red" on a HA card.
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.
Personally, I would view the descriptive (Good/Average/Unhealthy) terms as more useful on a dashboard, so that would be my preference.
Certainly, a dashboard is where I would want to use those values, too. That was my motivation. However, I'm also open to sticking to the colors.
My only concern would be that they would possibly require some care when handling the translation strings (especially because this meaning of "average" isn't necessarily "the mean of some numbers
This is a good call-out. Perhaps if we do use these more human-friendly values, we should stick with one set (i.e., normal, elevated, and unhealthy) rather than having different values per device type.
@@ -161,7 +166,11 @@ def sensor_update_to_bluetooth_data_update( | |||
val = getattr(adv.readings, key) | |||
if val == -1: | |||
continue | |||
val *= desc.scale | |||
val = ( |
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.
Instead of this, can we set the translation_key
add add translations for the states?
Also, isn't the user expecting to see "Red/Yellow/Green", based on the official app and device screen?
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 agree, I think we should represent it same as the app does, also for the translations (was actually waiting to have time to search for example for the translations later, thanks @abmantis)
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.
Sure, I'll add translations for the states instead.
Also, isn't the user expecting to see "Red/Yellow/Green", based on the official app and device screen?
I'm not sure if this would necessarily be the expectation. See #137291 (comment).
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'm not sure if I did the translations correctly, and I haven't figured out how to test the translations programmatically. Could someone take a look at the latest changes and let me know if you have any further suggestions?
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
Proposed change
Adds a threshold level sensor so you can see the threshold levels from the Aranet4 and AranetRn+ in Home Assistant.
Type of change
Additional information
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
.To help with the load of incoming pull requests: