Implement embedding DeviceStatus containers #1526
Conversation
Adds DeviceStatus.embed(self, other: DeviceStatus) method that allows embedding separate DeviceStatus containers into a single one. This will copy over the switch, setting, and sensor descriptors to the container and prefixing their names with the name of the other class. Internally, __getattribute__ is overridden to check if the property name contains ':' and if so, the attribute lookup is done in an embedded class named after the first part of the key.
|
@starkillerOG feel free to give this PR a try on your vacuum to see if the embedding works as expected! I haven't tested this as of yet as I don't have access to my vacuum at the moment. |
Codecov Report
@@ Coverage Diff @@
## master #1526 +/- ##
==========================================
+ Coverage 82.13% 82.29% +0.15%
==========================================
Files 145 145
Lines 14165 14195 +30
Branches 3416 3422 +6
==========================================
+ Hits 11635 11682 +47
+ Misses 2305 2286 -19
- Partials 225 227 +2
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -403,7 +403,10 @@ def manual_control( | |||
| @command() | |||
| def status(self) -> VacuumStatus: | |||
| """Return status of the vacuum.""" | |||
| return VacuumStatus(self.send("get_status")[0]) | |||
There was a problem hiding this comment.
@rytilahti very nice, this simplifies it quite a bit.
However now there is no way to only get the VucuumStatus withouth calling the consumable_status and clean_history.
For HomeAssistant it is optimal to have every endpoint a diffrent UpdateCoordinator such that if some entities are disabled, no needless calls are made to the roborock.
However this would complicate the code quite a bit, so I am okay with just having a single coordinator that always calls all endpoints and just accept the possible unneeded calls.
There was a problem hiding this comment.
I will try to test this tonight
There was a problem hiding this comment.
I'm slowly working the backlog after returning back home. You have a good point about having separate functions to make it possible to avoid unnecessary I/O that I was not considering while working on the initial implementation. This is not only relevant to save some I/O requests, but the current way has another issue, namely that handling of error cases (e.g., a device not supporting some feature) needs to be done case-by-case...
Instead of embedding, another approach could be to extend the API to add a way to expose references to methods that can be used to obtain the status information, and leave it to the downstream like homeassistant to choose if they want to use it. The problem I'm seeing though is that when the sensors & co are dynamic, homeassistant would need to hardcode what it wants as long as there is no way to check which entities are disabled, so I don't think that's a good idea for now. Your solution to add a separate "raw update" for cli and other users to allow obtaining just the get_status results sounds good to me.
Back to the issue of error handling / avoiding unsupported calls: maybe the embed should rather acts as a builder and take a callable instead of a status container as an input. This would make it easier to implement generic handling to avoid crashing on unsupported features, as well as to avoid doing unnecessary I/O on subsequent update cycles. I think this needs some thinking and experimenting to see what works the best.
|
@rytilahti I have tested this today. |
Adds
DeviceStatus.embed(self, other: DeviceStatus)method that allows embedding separate DeviceStatus containers into a single one. This will copy over the switch, setting, and sensor descriptors to the container and prefixing their names with the name of the other class.Internally,
__getattribute__is overridden to check if the property name contains ':' and if so, the attribute lookup is done in an embedded class named after the first part of the key.This PR also converts roborock vacuum integration to expose cleaning summary and consumable statuses using this functionality.