Skip to content
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

Implement introspectable sensors #1488

Merged
merged 11 commits into from
Aug 13, 2022
Merged

Conversation

rytilahti
Copy link
Owner

@rytilahti rytilahti commented Aug 11, 2022

This is the first PR to add support for introspectable integration interfaces to allow downstream users to query the device in order to obtain information about available sensors (e.g., "Voltage"), settings ("Wifi LED"), and actions (e.g., "Reset brush filter").
This will reduce the need to hard-code any details (e.g., inside homeassistant) about what exactly is supported by a device object.

The way this works is to have descriptor classes that define the information necessary to present the information to end users.
This PR adds all currently existing descriptors, but only the implementation for sensors. The rest of the types will follow soon in follow-up pull requests.

Integration developers can modify their DeviceStatus implementations to add @sensor decorators to expose properties that can be interesting for end-users:

class VacuumStatus(DeviceStatus):
    @property
    @sensor(name="Error Code", icon="mdi:alert-circle")
    def error_code(self) -> int:
        return -1

    @property
    @sensor(name="Battery", unit="%", icon="mdi:battery")
    def battery_level(self) -> int:
        return 10

The sensor descriptor objects will be available using DeviceStatus.sensors() and will contain the supplied information.
The decorator inspects the return type of the property to deliver information whether a sensor is a binary sensor or just a regular one, here's an example how it looks like in homeassistant:
image

The API is currently provisional and will likely change in the near future, I'm currently looking for feedback from other developers using the library as well as those developing integrations. Ping @Kirmas @starkillerOG @syssi

TBD:

@starkillerOG
Copy link
Contributor

I think this is awesom and will reduce the maintenance work in HomeAssistant.

However inside python-miio, how is this going to work for diffrent models of lets say a vacuum.
Does every model need its own "VacuumStatus" class?
Is there then going to be a mapping stucture from models to Status classes?

@rytilahti
Copy link
Owner Author

rytilahti commented Aug 11, 2022

However inside python-miio, how is this going to work for diffrent models of lets say a vacuum.
Does every model need its own "VacuumStatus" class?

I'm not sure I understand what you mean, but I'll try to explain how I'm thinking about it.
The status container is integration specific (e.g., roborock has its own VacuumStatus as does Dreame or Viomi), so homeassistant (or other users) can check if the sensor is returning None and skip creating an entity for it. Therefore there should be no need to have a separate mapping structure, but it is the responsibility of the status class to respond accordingly.

Here's how the sensors are initialized in the homeassistant integration (from async_setup_entry of the sensor platform):

# Setup next-gen sensors
from .ng_sensor import XiaomiSensor

for sensor in device.sensors():
    if sensor.type == "sensor":
        if getattr(coordinator.data, sensor.property) is None:
            _LOGGER.debug("Skipping %s as it's value was None", sensor.property)
            continue

        entities.append(XiaomiSensor(device, sensor, config_entry, coordinator))

and it looks like this in the log for the power strip:

xiaomi_miio.ng_sensor] Adding sensor: XiaomiMiioSensorDescription(key='temperature', device_class=None, entity_category=None, entity_registry_enabled_default=True, entity_registry_visible_default=True, force_update=False, icon='mdi:thermometer', has_entity_name=False, name='Temperature', unit_of_measurement=None, last_reset=None, native_unit_of_measurement='C', state_class=None, attributes=(), parent_key=None)
xiaomi_miio.ng_sensor] Adding sensor: XiaomiMiioSensorDescription(key='current', device_class=None, entity_category=None, entity_registry_enabled_default=True, entity_registry_visible_default=True, force_update=False, icon='mdi:sensor', has_entity_name=False, name='Current', unit_of_measurement=None, last_reset=None, native_unit_of_measurement='A', state_class=None, attributes=(), parent_key=None)
xiaomi_miio.ng_sensor] Adding sensor: XiaomiMiioSensorDescription(key='load_power', device_class=None, entity_category=None, entity_registry_enabled_default=True, entity_registry_visible_default=True, force_update=False, icon='mdi:sensor', has_entity_name=False, name='Load power', unit_of_measurement=None, last_reset=None, native_unit_of_measurement='W', state_class=None, attributes=(), parent_key=None)
xiaomi_miio.sensor] Skipping leakage_current as it's value was None
xiaomi_miio.sensor] Skipping voltage as it's value was None
xiaomi_miio.sensor] Skipping power_factor as it's value was None

For vacuums on homeassistant's end, I'm planning to convert the existing custom implementation inside homeassistant to use the generic data update coordinator that is already used for many devices. This will require modification to the vacuumstatus container to expose some information (like consumables) somehow sanely so it's still a WIP. In the end, this will simplify adding support for non-roborock vacuums that may expose different types of sensors and settings.

I have now homeassistant implementation for button, select, number, binary_sensor, and sensor platforms that use python-miio delivered information to expose the functionality, and the approach seems to work good enough. There are some details that needs figuring out, but it's a start. Here's an example of showing buttons, selects and number entities for my roborock:

image

As some of the things (like fanspeed preset) is already something the vacuum platform implements, we will just need to ignore creating entities for those cases.

Is there then going to be a mapping stucture from models to Status classes?

See my answer above, I hope there should be need for that but I only have two devices to test and develop on. I want to start this from simple to build upon and fix issues as they come, that's why the details will likely change when we get some more testing done :-) It is likely that there's might need for some sort of feature flags for some cases, but I'm trying to have a base implementation working before tackling that issue.

edit: to add a bit more, the @sensor decorator is just a shortcut to define SensorDescriptors for the class. An integration can simply override sensors() method to create those descriptors manually. For some settings it might be necessary to do exactly that if, e.g., the used enum for available values is chosen first after the initialization like it is for roborock.

@rytilahti rytilahti changed the title Initial implementation of introspectable device interfaces Initial implementation of introspectable device interfaces (sensors) Aug 11, 2022
@starkillerOG
Copy link
Contributor

@rytilahti sounds very good, nice implementation.
So if I understand correctly if you have 2 roborock vacuums of diffrent models one which has a water_bin the other not.
For one the "water_level" sensor in the "VacuumStatus" class will be set to lets say 50% and the other will be None.
Then HomeAssistant won't add the water_level sensor for the vacuum where the value is None.

So all roborock vacuum models will have the same "VacuumStatus" class containing all posible status properties/sensors. The once that are not supported by a specific model will just be None and won't be added to HomeAssistant.

@Kirmas
Copy link
Contributor

Kirmas commented Aug 12, 2022

Looks grate 👍

This fixes "Decorated property not supported" errors for the time being without
hopefully many downsides. The impact is hard to measure as the list of misc errors
is not to be found.

This should be revisited if/when python/mypy#1362 gets solved.
@rytilahti rytilahti force-pushed the feat/introspectable_sensors branch from bc784a9 to 5c684a9 Compare August 13, 2022 03:05
@codecov-commenter
Copy link

codecov-commenter commented Aug 13, 2022

Codecov Report

Merging #1488 (72c322e) into master (015fe47) will increase coverage by 0.09%.
The diff coverage is 95.32%.

@@            Coverage Diff             @@
##           master    #1488      +/-   ##
==========================================
+ Coverage   82.50%   82.59%   +0.09%     
==========================================
  Files         140      142       +2     
  Lines       13908    14009     +101     
  Branches     3305     3341      +36     
==========================================
+ Hits        11475    11571      +96     
- Misses       2214     2219       +5     
  Partials      219      219              
Impacted Files Coverage Δ
miio/device.py 55.21% <61.53%> (-0.63%) ⬇️
miio/__init__.py 100.00% <100.00%> (ø)
miio/descriptors.py 100.00% <100.00%> (ø)
miio/devicestatus.py 100.00% <100.00%> (ø)
miio/powerstrip.py 96.61% <100.00%> (+0.27%) ⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@rytilahti
Copy link
Owner Author

I simplified the interface, and added some tests and documentation. The @sensor allows passing any kwarg arguments, which is now used by the homeassistant integration. The details may still change as we move onwards but I consider this PR now done and continue adding switches. I'll link a homeassistant draft PR soon so anyone can start testing their devices and adding the necessary meta data. Here's how it currently looks, the next step is to convert the LED and power to toggles/switches:
image

@rytilahti
Copy link
Owner Author

rytilahti commented Aug 13, 2022

@starkillerOG oh, forgot push comment to answer you previously, but you got it right!

If you want to experiment how it works like with your devices, I pushed my local working copy of the homeassistant modifications in my repo: https://github.com/rytilahti/home-assistant/tree/xiaomi_miio/feat/entities_from_upstream - note that parts are rather hacked together and may require some changes to make it work on other devices. Let me know if you give it a try!

@rytilahti rytilahti changed the title Initial implementation of introspectable device interfaces (sensors) Implement introspectable sensors Aug 13, 2022
@rytilahti rytilahti merged commit d872d55 into master Aug 13, 2022
@rytilahti rytilahti deleted the feat/introspectable_sensors branch August 13, 2022 17:44
@starkillerOG
Copy link
Contributor

@rytilahti thanks a lot, this certainly looks like a greath improvement and a good way to go for python-miio!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants