Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Update KNX component to xknx 0.11 #24738
Update KNX component to xknx 0.11 #24738
Changes from 3 commits
334885b
6811589
c3d0727
57235db
255a9ec
f11742d
15a2c49
8e81b78
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is xknx aware of home assistant sensor device classes?
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.
Yes, xknx sensor classes are. The device class is returned as a string - or
None
if there is no fitting HA device class. From xknx/devices/remote_value_sensor.py:looks for
ha_device_class
from eg. here:https://github.com/XKNX/xknx/blob/cc10a82d11b4a60bd9765a7b410ab05edd5da41f/xknx/knx/dpt_2byte_float.py#L87
homeassistant/const.py is not imported in xknx, if you meant that.
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.
Ok. Coupling the two packages this tightly is fragile. If the home assistant API would change, something could break until the xknx library is updated. I would recommend to have a dict in this module that maps between the xknx sensor class and the home assistant device class.
But since the xknx library seems to be built for home assistant I don't think it's strictly necessary.
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.
Xknx works on its own but is built with HA as a main target.
With almost 50 different sensor classes its nice to have everything in one place (range, unit of measurement, ha_device_class, ...) instead of spreading these attributes out to different modules.
We could import the DEVICE_CLASS_* constants in this module and map against our ha_device_class attribute. To me this would feel kind of redundant.