Fix upnp creating derived sensors #57436
Conversation
|
Hey there @ehendrix23, mind taking a look at this pull request as it has been labeled with an integration ( |
|
Care for a review @ehendrix23 ? |
| @@ -74,28 +75,32 @@ | |||
|
|
|||
| DERIVED_SENSORS: tuple[UpnpSensorEntityDescription, ...] = ( | |||
| UpnpSensorEntityDescription( | |||
| key="KiB/sec_received", | |||
| key=BYTES_RECEIVED, | |||
There was a problem hiding this comment.
Side note: Having these sensors that are calculated states is not in line with our standards. Integrations that integrate devices or services should not calculate state. We have other integrations for that like template and integration sensors.
There was a problem hiding this comment.
Do you want this to be fixed with this PR @MartinHjelmare? It'll be a breaking change as certain sensors are no longer (directly) supported.
There was a problem hiding this comment.
No. I suggest we deprecate these sensors separately. Start by making them disabled by default. If they are still added, log a warning saying they are deprecated and will be removed in a future release. Then after some time, remove them.
There was a problem hiding this comment.
@MartinHjelmare @StevenLooman This is bad. Us users are mostly interested in the kB/sec sensors.
Me personally I don't want to create template sensors when it can be avoided.
Is it a solution to perform the calculations in the backend library and provide the resulting data to Core? And maybe leave out / replace some of the unwanted RAW_SENSORs?
There was a problem hiding this comment.
Yes, you can offload it to the library.
There was a problem hiding this comment.
Issue requesting the proposed implementation: StevenLooman/async_upnp_client#102
|
Should this be part of a hot fix ? |
Sure @balloob ! |
|
Great, it is working again! |
|
When the calculated sensors were removed, was that breaking change documented in the update release notes for the release the change came in? I've gone back through several past releases and cannot find any release notes indicating this breaking change was included. It was only by luck I managed to find a reference to this github issue to understand what happened in the first place. After reading the comments here, it sounds like the breaking change is imminent again in the future? |
Proposed change
Fix adding derived sensors (KiB/s, packets/s) on device creation.
Type of change
Additional information
Checklist
black --fast 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
.coveragerc.The integration reached or maintains the following Integration Quality Scale:
To help with the load of incoming pull requests: