Add service ota_update to shelly integration by mib1185 · Pull Request #48448 · home-assistant/core

mib1185 · 2021-03-28T21:43:16Z

Proposed change

This PR adds a new service ota_update to the shelly integration, which is intended to trigger OTA updates to a single or multiple devices at the same time.

Type of change

Dependency upgrade
Bugfix (non-breaking change which fixes an issue)
New integration (thank you!)
New feature (which adds functionality to an existing integration)
Breaking change (fix/feature causing existing functionality to break)
Code quality improvements to existing code or addition of tests

Additional information

This PR fixes or closes issue: fixes #
This PR is related to issue:
Link to documentation pull request: Add service ota_update to shelly integration home-assistant.io#17163

Checklist

The code change is tested and works locally.
Local tests pass. Your PR cannot be merged unless tests pass
There is no commented out code in this PR.
I have followed the development checklist
The code has been formatted using Black (black --fast homeassistant tests)
Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

Documentation added/updated for www.home-assistant.io

If the code communicates with devices, web services, or third-party tools:

The manifest file has all fields filled out correctly.
Updated and included derived files by running: python3 -m script.hassfest.
New or updated dependencies have been added to requirements_all.txt.
Updated by running python3 -m script.gen_requirements_all.
Untested files have been added to .coveragerc.

The integration reached or maintains the following Integration Quality Scale:

No score or internal
🥈 Silver
🥇 Gold
🏆 Platinum

To help with the load of incoming pull requests:

I have reviewed two other open pull requests in this repository.

frenck · 2021-03-29T07:52:13Z

homeassistant/components/shelly/__init__.py

+            resp = await rest.device.http_request("get", "ota", {"update": "true"})
+            _LOGGER.debug("OTA update service - response: %s", resp)
+
+    hass.services.async_register(DOMAIN, SERVICE_OTA_UPDATE, async_service_ota_update)


This should be implemented as an entity service:

https://developers.home-assistant.io/docs/dev_101_services#entity-services

Hi @frenck
sorry but I'm not sure if I get it correct 😕
The service I want to implement is not related to a specific entity/sensor or platform, but to a device - therefore I added this service as integration service.
To be honest I was also wondering why I have to extract the device_ids from area_id (and filter them to get only shelly devices) to get afterwards the (config_)entry_id from the devices and in addition have to care about that no duplicated device_ids were processed 🤔
Nevertheless it works as expected - yesterday I have already updated 5 Shelly devices at the same time with one service call 😎
Surely it is more realistic that I'm total wrong and have missed something essential 🙈
I would be very grateful for hints on this, also about extended examples.

When implemented as an entity service, it will support a target, in that case Home Assistant will resolve/handle devices, areas and entity ID references for you, and call the matching entity services (no matter what the user has put in).

The example is in the link above. If you search the codebase for async_register_entity_service, you'll find dozens of examples.

bdraco

Please use entity services as they can target an area, device, or entity_id when using target: in services.yaml

https://github.com/home-assistant/core/blob/dev/homeassistant/helpers/service.py#L506

balloob · 2021-03-31T16:45:42Z

Is this a service that we want? Or is this something that should be part of device actions (which we don't currently have)?

mib1185 · 2021-03-31T16:47:19Z

@bdraco to be honest until now I do not get it in my mind how to implement/use an entity service for a device (not for a specific entity/sensor/platform of this device). I would be very grateful for hints on this. I have also already searched the codebase for async_register_entity_service but to be honest this confused me more 🙈

mib1185 · 2021-03-31T16:50:31Z

Is this a service that we want? Or is this something that should be part of device actions (which we don't currently have)?

@balloob mhhh ... device actions sounds like it is what I'm looking for 🤔
I will read throw the documentation and give it a try ... thanks for the hint 👍

mib1185 · 2021-03-31T17:14:42Z

Is this a service that we want? Or is this something that should be part of device actions (which we don't currently have)?

@balloob mhhh ... device actions sounds like it is what I'm looking for thinking
I will read throw the documentation and give it a try ... thanks for the hint +1

mhhh ... as I understand from the docs the base for an device action is at least always a service ... so the main question remains - how to implement an entity service for a device 😕

bdraco · 2021-03-31T17:22:28Z

@bdraco to be honest until now I do not get it in my mind how to implement/use an entity service for a device (not for a specific entity/sensor/platform of this device). I would be very grateful for hints on this. I have also already searched the codebase for async_register_entity_service but to be honest this confused me more 🙈

In the service call we get the entities that are attached to the device when the device is passed in.

Here are some examples:

https://github.com/home-assistant/core/blob/dev/homeassistant/components/homeassistant/__init__.py#L55

https://github.com/home-assistant/core/blob/dev/homeassistant/components/automation/__init__.py#L362

mib1185 · 2021-03-31T21:09:55Z

Sorry @bdraco but your reply did not help me, to get it 🙈
My issue is, that I do not get the relation between an entity service and a device in the code (simple said - where (in which file/place) I have to place the code for the update function and where and how I have to register this code as a service)
Each integration, which implements entity services is somehow setup like:

async def async_setup(hass: HomeAssistant, config: dict):
    component = EntityComponent(_LOGGER, DOMAIN, hass)
    await component.async_setup(config)
    component.async_register_entity_service(...)

But the Shelly integration does not go this way - so for me the entry-point to implement an entity service seems missing.

Maybe it is an language related issue on my end (i do net get it, because i misunderstood something/the docs) or simply i miss something essential from the mechanic (i do net get it, because lag of knowledge about some related thing) 🤔 😕
But I really want to understand it so that I'm able to solve the issue by my own, also to understand and apply suggestions and even to learn more and more about the mechanic of home assistant and the relation and interaction with integrations.

MartinHjelmare · 2021-04-01T06:22:19Z

I don't believe an entity service is correct in this case since we're not targeting a specific platform. It's a config entry specific service.

Paulus has a point: Do users want to automate OTA updates? If yes, a service is relevant. If no, maybe this should just be a websocket command and a frontend card?

mib1185 · 2021-04-01T13:13:25Z

Hi @MartinHjelmare
thanks for your reply. I also think it is a config entry specific service.
IMHO it should be a service to get advantage of the flexibility of a services (can be used in scripts, automations, action in lovelace cards, ...)
For me there is a use case for that service, so that I'm able to update all shelly devices at once.
As I noticed from some issues comments, there are also users out there which has quiet more shelly devices in use (some >30) for them it could also be an improvement.

thecode · 2021-04-03T21:28:55Z

homeassistant/components/shelly/__init__.py

+        for device in devices:
+            entry_id = next(iter(device.config_entries))
+            entry_data = hass.data[DOMAIN][DATA_CONFIG_ENTRY][entry_id]
+            rest: ShellyDeviceRestWrapper = entry_data[REST]


Note that ShellyDeviceRestWrapper is not created if the device is a sleeping device, the service should not rely on this wrapper, but on the main wrapper - ShellyDeviceWrapper

Suggested change

rest: ShellyDeviceRestWrapper = entry_data[REST]

device_wrapper: ShellyDeviceWrapper = entry_data[COAP]

see:

core/homeassistant/components/shelly/__init__.py

Line 133 in 86176f1

if not entry.data.get("sleep_period"):

In addition we should not try to trigger update for a sleeping devices, as I see it we have two options:

(preferred option) For a sleeping device, store a flag that we need to trigger an update and trigger an update upon CoAP event from the device, can be done here:

core/homeassistant/components/shelly/__init__.py

Line 184 in 86176f1

# Check for input events

For a sleeping device log a message that device is skipped since it is a sleeping device

the method is now moved into ShellyDeviceRestWrapper as async_trigger_ota_update()

async_trigger_ota_update() uses the flag self._ota_update_pending for scheduling an ota update for sleeping devices

thecode · 2021-04-03T21:40:19Z

homeassistant/components/shelly/__init__.py

+            entry_id = next(iter(device.config_entries))
+            entry_data = hass.data[DOMAIN][DATA_CONFIG_ENTRY][entry_id]
+            rest: ShellyDeviceRestWrapper = entry_data[REST]
+            update_data = rest.device.status["update"]


If REST sensors are not enabled, the status will be the status of the first init of the device which may not reflect it's current state, even a REST sensor is enabled, the update period for it is 60 seconds, so the status may not reflect the correct device state, since it is used to decide if device needs an update, it would be better to first pull the device latest status:
Example can be found here (need to wrap with timeout and catch the error):

core/homeassistant/components/shelly/__init__.py

Line 299 in 86176f1

async with async_timeout.timeout(AIOSHELLY_DEVICE_TIMEOUT_SEC):

await self.device.update_status()

async_trigger_ota_update() use now async_refresh() from DataUpdateCoordinator to update status data

thecode · 2021-04-03T21:42:42Z

homeassistant/components/shelly/__init__.py

+                update_data["old_version"],
+                update_data["new_version"],
+            )
+            resp = await rest.device.http_request("get", "ota", {"update": "true"})


Also here, need to wrap with timeout and catch the error.

Note: We intentionally don't do any http request directly from HA, but add a method in aioshelly library and call it from HA, might be a better option to do the same here

yeah for sure ... I've created an PR home-assistant-libs/aioshelly#90 to add a new device method ... this is already used in async_trigger_ota_update() wraped with timeout in try-except-block

thecode · 2021-04-03T21:46:59Z

homeassistant/components/shelly/services.yaml

@@ -0,0 +1,10 @@
+# shelly service descriptions.
+
+ota_update:


I think we should add a flag named beta which will allow updating to beta firmware, since the OTA command support triggering update to beta FW.
https://shelly-api-docs.shelly.cloud/#ota
Might also need to consider if we allow specifying a URL for a direct update file in the service call

url and beta parameter were added
In addition there is now a force parameter which allows to downgrade from beta-version to release-version and also to reinstall current version

mib1185 · 2021-04-05T13:00:26Z

wait for upstream PR home-assistant-libs/aioshelly#90 to be merged and released
after release, requirements and dependencies will be updated here ...

thecode · 2021-04-06T20:16:28Z

Hi @mib1185,
Do you use discord? can you message me - thecode#6829?
Thanks

homeassistant/components/shelly/__init__.py

github-actions · 2021-05-07T16:05:47Z

There hasn't been any activity on this pull request recently. This pull request has been automatically marked as stale because of that and will be closed if no further activity occurs within 7 days.
Thank you for your contributions.

mib1185 · 2021-05-07T16:19:33Z

still waiting for upstream ...

mib1185 · 2021-05-11T17:47:23Z

accidental deleted the branch

mib1185 · 2021-05-19T10:36:21Z

Close this PR in respect to ongoing architecture discussion home-assistant/architecture#566

homeassistant added the cla-signed label Mar 28, 2021

probot-home-assistant bot added has-tests integration: shelly new-feature labels Mar 28, 2021

probot-home-assistant bot assigned balloob Mar 28, 2021

mib1185 mentioned this pull request Mar 28, 2021

Add service ota_update to shelly integration home-assistant/home-assistant.io#17163

Closed

8 tasks

frenck reviewed Mar 29, 2021

View reviewed changes

bdraco requested changes Mar 31, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

mib1185 requested review from bdraco and frenck April 3, 2021 16:15

thecode requested changes Apr 3, 2021

View reviewed changes

mib1185 marked this pull request as draft April 5, 2021 11:34

mib1185 force-pushed the shelly_add-otaupdate-service branch from d622ac6 to 3d8a159 Compare April 5, 2021 17:08

mib1185 added 4 commits April 5, 2021 19:10

add ota_update service

63b4db9

fix pylint W1505

68e9361

add name to service description

35b37b1

add url and beta parameter, respect sleeping devices

901d494

mib1185 force-pushed the shelly_add-otaupdate-service branch from 3d8a159 to 901d494 Compare April 5, 2021 17:10

pylint disable=broad-except

f5417b0

bdraco added the waiting-for-upstream We're waiting for a change upstream label Apr 5, 2021

use async_extract_referenced_entity_ids()

8f8cc86

MartinHjelmare reviewed Apr 7, 2021

View reviewed changes

homeassistant/components/shelly/__init__.py Outdated Show resolved Hide resolved

mib1185 added 2 commits April 7, 2021 13:35

apply suggestion

91b37b1

replace depricated async_get_registry()

2a3bdc5

mib1185 force-pushed the shelly_add-otaupdate-service branch from 495f460 to 2a3bdc5 Compare April 7, 2021 15:44

github-actions bot added the stale label May 7, 2021

github-actions bot removed the stale label May 7, 2021

mib1185 closed this May 11, 2021

mib1185 deleted the shelly_add-otaupdate-service branch May 11, 2021 17:44

mib1185 restored the shelly_add-otaupdate-service branch May 11, 2021 17:46

mib1185 reopened this May 11, 2021

mib1185 closed this May 19, 2021

github-actions bot locked and limited conversation to collaborators May 20, 2021

mib1185 deleted the shelly_add-otaupdate-service branch November 1, 2021 17:53

	rest: ShellyDeviceRestWrapper = entry_data[REST]
	device_wrapper: ShellyDeviceWrapper = entry_data[COAP]

Uh oh!

Conversation

mib1185 commented Mar 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Proposed change

Type of change

Additional information

Checklist

Uh oh!

frenck Mar 29, 2021

Choose a reason for hiding this comment

Uh oh!

mib1185 Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frenck Mar 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bdraco left a comment

Choose a reason for hiding this comment

Uh oh!

balloob commented Mar 31, 2021

Uh oh!

mib1185 commented Mar 31, 2021

Uh oh!

mib1185 commented Mar 31, 2021

Uh oh!

mib1185 commented Mar 31, 2021

Uh oh!

bdraco commented Mar 31, 2021

Uh oh!

mib1185 commented Mar 31, 2021

Uh oh!

MartinHjelmare commented Apr 1, 2021

Uh oh!

This comment has been minimized.

mib1185 commented Apr 1, 2021

Uh oh!

thecode Apr 3, 2021

Choose a reason for hiding this comment

Uh oh!

mib1185 Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

thecode Apr 3, 2021

Choose a reason for hiding this comment

Uh oh!

mib1185 Apr 5, 2021

Choose a reason for hiding this comment

Uh oh!

thecode Apr 3, 2021

Choose a reason for hiding this comment

Uh oh!

mib1185 Apr 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thecode Apr 3, 2021

Choose a reason for hiding this comment

Uh oh!

mib1185 Apr 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mib1185 commented Apr 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thecode commented Apr 6, 2021

Uh oh!

Uh oh!

github-actions bot commented May 7, 2021

Uh oh!

mib1185 commented May 7, 2021

Uh oh!

mib1185 commented May 11, 2021

Uh oh!

mib1185 commented May 19, 2021

Uh oh!

mib1185 commented Mar 28, 2021 •

edited

Loading

mib1185 Mar 29, 2021 •

edited

Loading

frenck Mar 29, 2021 •

edited

Loading

mib1185 Apr 5, 2021 •

edited

Loading

mib1185 Apr 5, 2021 •

edited

Loading

mib1185 commented Apr 5, 2021 •

edited

Loading