-
-
Notifications
You must be signed in to change notification settings - Fork 29k
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 Broadlink time platform #121470
Add Broadlink time platform #121470
Conversation
Hey there @Danielhiversen, @felipediel, @L-I-Am, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
@@ -6,6 +6,7 @@ | |||
|
|||
DOMAINS_AND_TYPES = { | |||
Platform.CLIMATE: {"HYS"}, | |||
Platform.TIME: {"HYS"}, |
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.
Let's keep this alphabetical
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.
What do we have Ruff for? 😅
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.
Done
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.
For a lot of the checks. We also have a pylint rule that checks if PLATFORMS
variables are alphabetical. But this is a dict, not a list, and it's called DOMAINS_AND_TYPED
hence there is no check for this. But I personally do really like when these lists are neatly ordered to be consistent with the rest
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 added it as the datetime platform and later just renamed it. Shame on me^^
if self._coordinator.data is None or "dayofweek" not in self._coordinator.data: | ||
raise ServiceValidationError( | ||
translation_domain=DOMAIN, | ||
translation_key="request_failed_device_not_connected", | ||
) |
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.
Shouldn't the entity be unavailable in that moment? Or are there other moments when this can happen
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 had the direct service call in mind. And at least during testing the device when Homeassistant first connected to the device (after bootup) it was available but no time was displayed, meaning the coordinator had to data yet.
Something I want to investigate and possible fix in a separate PR
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.
But why is that the case? you're now raising an issue that the device is not connected, but from your response I also guess that it already was connected, so what should the user do to get the data?
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.
Removed
"""Initialize the sensor.""" | ||
super().__init__(device) | ||
|
||
self._attr_unique_id = f"{device.unique_id}-device-time" |
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.
self._attr_unique_id = f"{device.unique_id}-device-time" | |
self._attr_unique_id = f"{device.unique_id}-device_time" |
if you ever need to migrate you can just do .split("-")
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.
Done
Please take a look at the requested changes, and use the Ready for review button when you are done, thanks 👍 |
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.
Looks very good! Here are a few suggestions...
async_add_entities([BroadlinkTime(device)]) | ||
|
||
|
||
class BroadlinkTime(BroadlinkEntity, TimeEntity): |
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.
Let's name this entity BroadlinkHysenTime, as it is specific to the Hysen class, and we may want to add other types in the future.
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.
Do you think other device types would have a fundamentally different time platform? The data used is retrieved via the broadlink api get_full_status
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, although we currently only have this device with day of the week and time in the API today, this structure could vary depending on the type of device in the future. Some devices might include day, month, and year, while others might include timezone information.
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.
Do we know of a device which is available today or will be available in the future? Or is this an assumption?
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.
It's always an assumption when it comes to Broadlink. When we don't know what is going to happen, we better keep it flexible. But I am totally okay if you want to leave it as is and change it later if we need to.
device = hass.data[DOMAIN].devices[config_entry.entry_id] | ||
async_add_entities([BroadlinkTime(device)]) |
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.
Let's add some flow control to ensure that if the entity is of type 'HYS', then we add BroadlinkHysenTime entities. Otherwise, we do nothing.
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.
While I get the idea I think this would be different then how filtering is done for other platforms. E.g.
core/homeassistant/components/broadlink/sensor.py
Lines 95 to 104 in 27a2114
BroadlinkSensor(device, description) | |
for description in SENSOR_TYPES | |
if description.key in sensor_data | |
and ( | |
# These devices have optional sensors. | |
# We don't create entities if the value is 0. | |
sensor_data[description.key] != 0 | |
or device.api.type not in {"RM4PRO", "RM4MINI"} | |
) | |
] |
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.
Even if we do a good job creating a unified interface in the library, we could still have at least two types of time devices: weekday/time and date/time.
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.
Moreover we do already filter the platforms by device type: https://github.com/home-assistant/core/pull/121470/files#diff-de5887f91e2b6b4559fe8fe3c68b104fbbcc5048feb84a0db325be16527d87f0R39
Why should we do it again?
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.
It's about making the code more robust. This function accepts a ConfigEntry
in its signature. It should be able to handle any ConfigEntry
, without depending on filtering or validation by a higher level module.
7aa9db3
to
672ae9d
Compare
@felipediel Please let us know if you think think your concerns are addressed so we can start getting this feature in :) |
They have not been addressed, but that's okay. Those were just suggestions. The code looks good to me; let's ship this thing. |
🚀 Okay let's go! |
Proposed change
Add the time platform for supported Broadlink devices.
Split from #115408
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: