Add depth calculation for zhimi.humidifier.cb1#1079
Add depth calculation for zhimi.humidifier.cb1#1079bieniu wants to merge 2 commits intorytilahti:masterfrom bieniu:humidifier-cb1-depth
depth calculation for zhimi.humidifier.cb1#1079Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1079 +/- ##
=======================================
Coverage 75.40% 75.40%
=======================================
Files 73 73
Lines 8367 8367
Branches 745 745
=======================================
Hits 6309 6309
Misses 1874 1874
Partials 184 184 ☔ View full report in Codecov by Sentry. |
| MODEL_HUMIDIFIER_CB1, | ||
| MODEL_HUMIDIFIER_CB2, | ||
| ]: | ||
| return round(int(self.data["depth"]) / 1.25) |
There was a problem hiding this comment.
| return round(int(self.data["depth"]) / 1.25) | |
| return int(int(self.data["depth"]) / 1.25) |
would do the same.
Regarding to the comment, do you think that there should be a separate property for the water tank availability?
There was a problem hiding this comment.
int() uses floor rounding, which I personally don't like, but you're right the result will be almost the same. I'll revert this.
After opening this PR I started to wonder if we should leave the depth property as the RAW value (for compatibility), add a water_level property where we will calculate the value and return None when the depth == 127. water_tank_availability also can be usefull to use it as binary sensor in HA. What do you think?
There is one more thing. We had discussion today with @jbouwh about water_level for Miot humidifiers. When water tank is full the device (zhimi.humidifier.ca4) send value 125 ({'did': 'water_level', 'siid': 2, 'piid': 7, 'code': 0, 'value': 125}) but the library uses as divider 1.2 https://github.com/rytilahti/python-miio/blob/master/miio/airhumidifier_miot.py#L132 and we get value of 104% then.
Should we unify this in the library? What is your opinion?
There was a problem hiding this comment.
After opening this PR I started to wonder if we should leave the
depthproperty as the RAW value (for compatibility), add awater_levelproperty where we will calculate the value and returnNonewhen thedepth == 127.water_tank_availabilityalso can be usefull to use it as binary sensor in HA. What do you think?
Sounds good to me. And I'm definitely in favor of unifying behavior and API inside this library -- this library has grown way too big from its inception targeted to support only the rockrobo v1 that I have, and is in a dire need for (API) restructuring to keep it maintainable and usable for downstreams, too. I wrote some thoughts on that topic here: #1059 (comment) . Shortly put, I think all devices on a specific category should implement a common API that will make it simpler for downstreams (like homeassistant) to avoid hardcoding any device specific details.
What I would like to see at some point is a mechanism to define the available sensors (maybe based on the property-like decorators on status classes) inside this code base, exposing all the necessary meta information to let homeassistant create sensors without hardcoding details (or device specific information).
So the flow on homeassistant's side should look something like:
- Request initialization of a device with host and token
- The library performs model detection (related to Add model autodetection #1038), and initializes the implementation that provides support for that device
- Homeassistant can detect the type of the device so that it knows which platform it wants to initialize for it, the common API ensures that homeassistant does not need to care about which implementation is required for the device
- The device object has an interface that can be used to request all sensors, supported, and maybe even actions(?) available to that implementation, making it unnecessary to hardcode device-specific details on homeassistant's end.
What do you think? Does that make sense? The downside is that this will obviously be an API breaking change on many levels, and will require quite a bit of work to pull it off properly..
There was a problem hiding this comment.
I could make an additional PR for HA to add 2 binairy_sensors that at cover_removed and max_water_level reached. Now water_level is 104% when max_water_level is true and is 105% when the cover is removed.
Better is not to use water_level as a source for the binary_sensors, but add additional attributes.
water_level should return None when cover is removed and 100% when max_water_level reached is true.
The code in the current PR's not displays not available at 104 or 105%. What the correct division should be, I cannot tell.
There was a problem hiding this comment.
Sounds good to me.
OK, so I close this PR, and will open new one.
What do you think? Does that make sense?
Yes, seems good to me. Progress requires extra work, as always ;)
zhimi.humidifier.cb1 reports
127whithout water tank and125when water tank is full.This PR also adds rounding value instead of simply converting to
int.