Allow to configure KNX time, date & datetime entities via UI#157603
Conversation
|
Hey there @Julius2342, @farmio, @marvin-w, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
0eacf86 to
a636bb0
Compare
a636bb0 to
c8785bd
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds UI configuration support for KNX time, date, and datetime entities, allowing users to configure these entities through the Home Assistant UI instead of requiring YAML configuration. This complements the existing YAML configuration method and brings these platforms in line with other KNX platforms that already support UI configuration.
- Refactored entity classes to support both YAML and UI configuration patterns
- Added schema definitions for time, date, and datetime platforms in the entity store
- Created comprehensive tests for both entity creation and loading from storage
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| homeassistant/components/knx/time.py | Refactored to support both YAML (KnxYamlTimeEntity) and UI (KnxUiTimeEntity) configuration with shared base class |
| homeassistant/components/knx/date.py | Refactored to support both YAML (KnxYamlDateEntity) and UI (KnxUiDateEntity) configuration with shared base class |
| homeassistant/components/knx/datetime.py | Refactored to support both YAML (KnxYamlDateTimeEntity) and UI (KnxUiDateTimeEntity) configuration with shared base class |
| homeassistant/components/knx/const.py | Added time, date, and datetime platforms to SUPPORTED_PLATFORMS_UI |
| homeassistant/components/knx/storage/const.py | Added constants for group address configuration (CONF_GA_TIME, CONF_GA_DATE, CONF_GA_DATETIME) |
| homeassistant/components/knx/storage/entity_store_schema.py | Added schema definitions for time, date, and datetime entities with validation for DPT types |
| homeassistant/components/knx/strings.json | Added UI strings for entity configuration dialogs |
| tests/components/knx/test_time.py | Added tests for UI entity creation and loading from storage |
| tests/components/knx/test_date.py | Added tests for UI entity creation and loading from storage |
| tests/components/knx/test_datetime.py | Added tests for UI entity creation and loading from storage |
| tests/components/knx/snapshots/test_websocket.ambr | Updated snapshots with schema validation for new platforms |
| tests/components/knx/fixtures/config_store_*.json | Added test fixtures for loading entities from storage |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
farmio
left a comment
There was a problem hiding this comment.
Thank you very much for your contribution 😃
Here are some - mostly stylistic / nit-picky - suggestions I have found
|
|
||
|
|
||
| class KNXDateTimeEntity(KnxYamlEntity, DateTimeEntity, RestoreEntity): | ||
| class _KNXDateTimeEntity(DateTimeEntity, RestoreEntity): |
There was a problem hiding this comment.
Nitpick: Other platforms don't add "Entity" for their platform classes. For consistency, consider to use _KnxDateTime as class name. (same for KnxYamlDateTime and KnxUiDateTime).
| # date_without_state_address | ||
| await knx.assert_no_telegram() |
There was a problem hiding this comment.
| # date_without_state_address | |
| await knx.assert_no_telegram() |
Remaining unasserted telegrams are checked at test-teardown so we don't need to do this explicitly.
| # date_with_state_address | ||
| await knx.assert_read( | ||
| "0/0/2", response=(0x18, 0x02, 0x18), ignore_order=True | ||
| ) # current |
| DATE_KNX_SCHEMA = vol.Schema( | ||
| { | ||
| vol.Required(CONF_GA_DATE): GASelector( | ||
| passive=False, write_required=True, valid_dpt="11.001" |
There was a problem hiding this comment.
Why wouldn't we allow passive GAs here? And for time and datetime?
There was a problem hiding this comment.
I was not sure what the passive addresses would be used for. Right now we are using get_write to get the group_address and get_state for the group_address_state, so the passive addresses are discarded, hence it did not make sense to ask for any.
I guess having address aliasses for group_address could make sense, but it feels less usefull for the group_address_state. What do you think?
There was a problem hiding this comment.
It doesn't make a difference. xknx treats all of those the same - it doesn't send anything but updates from telegrams.
For other entities I've used get_state_and_passive for the state address. If no state address is set, this would result in a list with first item None and append the passive addresses. xknx stores list-tails as passive addresses, regardless of group_address or group_address_state.
There was a problem hiding this comment.
There was a problem hiding this comment.
I see, all of the passive addresses end up in RemoteValue.passive_group_addresses anyways. Thanks for the explanation, I'll stick to get_state_and_passive then. 👍
|
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
d631394 to
65e9235
Compare
65e9235 to
8049f75
Compare
|
Do you, by any chance, have a clue why the tests failed when |
The problem was not the This was my bad, I copied another fixture and at that point didn't realize that all passives end up in the same place, so I assumed that |
|
Ah great, thanks for explaining this. I was just worried that, whenever we do a |
Proposed change
Support KNX time, date and datetime entity configuration from UI.
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: