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

Redesign AppInfo: Update sensor #96

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
Draft

Conversation

richibrics
Copy link
Owner

@richibrics richibrics commented Feb 12, 2024

Related to #92

AppInfo now has

  • Name
  • Current version
  • Latest version

In HomeAssistant is shown as Update entity which notifies the user when an update is available

TODO

  • Understand if the install button can be disabled if the InstallUpdate callback is not implemented
  • Find a way to remove the command_topic if no Install method is implemented
  • Restore latest version check from the mocked value

@richibrics richibrics linked an issue Feb 12, 2024 that may be closed by this pull request
@richibrics
Copy link
Owner Author

richibrics commented Feb 12, 2024

@infeeeee Current idea is to find a good way to implement an Update entity of Home assistant out of a Sensor of IoTuring, which could be used at start in IoTuring, but later also to check possible updates of other programs if anyone will be interested in it

@richibrics
Copy link
Owner Author

richibrics commented Mar 8, 2024

Current idea:
Entity that has update data can have

  • current version sensor
  • latest version sensor
  • is new version available sensor
  • update command

The problem is that each entity data has its own key and if I have the HomeAssistant warehouse that has to map these 4 data in one Update Entity in Hass, it doesn't know which data is what (e.g. doesn't know current version sensor needs to go in the configuration as the current version).

Two ideas:

  1. Have fixed keys for the entity data to use so that the Hass WH will look for those to place in the Update Entity configurations in the correct spots.
  2. (Favorite) Have the Entity data (those 4) in my Entity, then in order for the WH to know which data is the one for example of current version, we place in the entities.yaml the key of that data for the current version.

Example of entities.yaml for choice 2:

IoTuringUpdate:
  icon: mdi:update
  custom_type: update
  sensor_current_version: "THE KEY OF THE SENSOR IN MY ENTITY"
  command_update: "THE KEY OF THE COMMAND IN MY ENTITY"
  sensor_latest_version: "KEY..."
  sensor_is_version_available: "KEY..."

What do you think @infeeeee ? I don't know if I wrote it down in an understable way

@infeeeee

This comment was marked as off-topic.

@richibrics
Copy link
Owner Author

Yeah sorry let me rewrite it.
Goal: have a HomeAssistant "Update" entity with IoTuring current version and available version with the future possibility of updating it from Hass.

In order to have the HassUpdate entity, we need to specify in the configuration we send via MQTT to Hass:

  • the topic with the current version
  • the topic with the latest version
  • the topic to which send a message to start the installation (for future uses)

In order to get this behavior, I need my IoTuring entity to have two sensors that send the current and latest version and a command to start the installation.
Then in the Hass WH I want to translate the 2 sensors and the command to 1 entity in HASS (an update entity).
In order to do it, I need to provide in the MQTT configurations the topics of the sensors and of the commands in the corrects spots:

  • topic of the sensor with current version in "state_topic" key in config
  • topic of the sensor with latest version in "latest_version_topic" in config
  • and the same for the command.

The problem is that the Hass WH doesn't know which of the sensors of my entity is the one with the current version and the one with the latest version.
To do it I thought we could provide to the WH the keys of the sensors of each information in the entities.yaml like I wrote above.

In this way when we type in the YAML that we want to configure an Update entity in Hass, then we place in the MQTT configurations the topics of the sensors I can correctly identify by using the keys I have in entities.yaml

e.g.

IoTuringUpdate:
  icon: mdi:update
  custom_type: update
  sensor_current_version: "THE KEY OF THE SENSOR IN MY ENTITY" -> this sensor's topic will be taken from the WH and will be placed in the MQTT configurations as "state_topic"
  .....

I hope it's better now; with Update I was not talking of the entities update interval

@infeeeee
Copy link
Collaborator

infeeeee commented Mar 8, 2024

Ah, ok , I understand now, I didn't know about the MQTT Update entity.

Basically it's an EntityCommand with 2 connected EntitySensors, and one of the sensors uses latest_version_topic instead of the state_topic.

I would do it this way:

  • Modify EntityCommand to allow 2 connected EntitySensors
  • Create a subclass of HomeAssistantEntityBase, something similar to LwtSensor, which will send the latest_version_topic. Lets call it SecondarySensor.
  • In CollectEntityData selects this new subclass for every additional connected sensors, first connected sensor remains the same.
  • For discovery, SecondarySensor only needs this custom topic everything else can be read from the first sensor. The topic could be read from the yaml:
IoTuringUpdate:
  icon: mdi:update
  custom_type: update
  secondary_topic: latest_version_topic
  • As it's still an EntitySensor, it updates the same way as any other sensors. Also this sensor is still available to other warehouses

@richibrics
Copy link
Owner Author

But this would be a problem if we would like to use the MQTT update entity without the command to trigger the update, since in IoTuring the command would become necessary isn't it ?

@infeeeee
Copy link
Collaborator

infeeeee commented Mar 8, 2024

Good point, then the other way: create a sensor with 2 topics. Actually this is the same way as ExtraAttributes work, maybe ExtraAttribute workflow could be tweaked, so they can publish to a custom topic, and in plaintext not in json.

Or create some dummy HomeAssistantCommand which doesn't have a command topic, and connect the 2 sensors to that.

@richibrics
Copy link
Owner Author

Well I prefer the Command way but I have to figure it out the best approach, thank you

@richibrics
Copy link
Owner Author

@infeeeee I can use a simple sensor connected to a command, then I define the topic of latest version to be the extra attributes of current version and set the value template to extract the value from the extra attributes json

@richibrics
Copy link
Owner Author

It's still a work in progress but looks great to show the update in hass:
image

@richibrics
Copy link
Owner Author

Need to come back to previous idea: having extra attributes

  • doesn't allow to have latest version in other WH
  • needs a way to link the extra attribute topic dynamically

@richibrics richibrics changed the title Remove version sensor Redesign AppInfo: Update sensor Mar 9, 2024
@richibrics
Copy link
Owner Author

HAss supports the Update entity discovery payload without command topic so to not show the "install" button. However without the Install button, the update doesn't show up in the available updates page.
Currently the command topic is provided but the script shows a warning message above the Install button that says the Install process must be run manually

@richibrics richibrics requested a review from infeeeee March 10, 2024 11:59
@richibrics
Copy link
Owner Author

⚠️ A fake latest version is currently returned to show how the update looks like

Copy link
Collaborator

@infeeeee infeeeee left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good!

The NO_REMOTE_INSTALL_AVAILABLE_MSG is cut for me on the HA frontend for some reason, maybe there is some hardcoded character limit?

kép

It really nice that HA translates everything correctly :)

@richibrics
Copy link
Owner Author

That's strange it doesn't happen to me

@infeeeee
Copy link
Collaborator

It cuts after exactly 255 characters, it's documented here: https://www.home-assistant.io/integrations/update.mqtt/#release_summary

release_summary string (Optional)

Summary of the release notes or changelog. This is suitable a brief update description of max 255 characters.

But this should be generated from the changelog in the future, and the App.getDescription() is not necessary there.

It's this line:

"release_summary": App.getDescription() + "<br><br>" + NO_REMOTE_INSTALL_AVAILABLE_MSG,

@richibrics
Copy link
Owner Author

Okay thanks I remove the description from there

@infeeeee
Copy link
Collaborator

Wait, this secondary sensor not even needed. I read the docs again, you can just publish a json to the state_topic, and latest_version_topic is not needed in that case, so only one sensor for one optional command:

JSON can also be used as state_topic payload.

{
  "installed_version": "1.21.0",
  "latest_version": "1.22.0",
  "title": "Device Firmware",
  "release_url": "https://example.com/release",
  "release_summary": "A new version of our amazing firmware",
  "entity_picture": "https://example.com/icon.png"

At the bottom of the page: https://www.home-assistant.io/integrations/update.mqtt/#examples

@richibrics
Copy link
Owner Author

richibrics commented Mar 15, 2024

Yes I know but in that way we would have lost the latest version information in other warehouses.

Also we would have needed to add to the discovery payload another attibute with the same topic as the state_topic: since the state_topic is dynamically computed and can't be set neither in entities.yaml or in AppInfo custom payload method, we would have had to do some other changes in the Hass WH.

WIth secondary sensor now we are able to support also other types of HomeAssistant entities that need additional attributes topic in the payload

edit: yes it's right we could have directly avoided the latest version topic.

@richibrics
Copy link
Owner Author

@infeeeee what do you think ? I'd leave it as is to make it available to other WH too

@infeeeee
Copy link
Collaborator

Both ways have pros and cons:

Pros of this workflow:

  • MQTT and console warehouse can easily get the current and new version numbers

Cons of this workflow:

  • Data in the custom payload is not available in other warehouses only in HA, e.g: release_url, release_summary
  • release_summary should be updated the same time as new version number, now it updates only after IoTuring restarts. So if there is a new update, the summary is not updated even in HA, users won't get info about new changes, it should be sent during Update, so sending discovery should be changed as well: https://www.home-assistant.io/integrations/update.mqtt/#release_summary
  • Technical debt: I don't know if we will ever need multiple sensors for any other entities, having this big workaround only for this single usecase seems a bit overkill

What should be changed if we send everything as a json/dict from a single sensor:

  • Console warehouse: some valueformatter to display the sensor value as it's a dict not a single string, but every value is just a string in the dict, so it shouldn't be that complex
  • MQTT Warehouse: I don't know how people use the MQTTWarehouse, but I guess it's for advanced users, so they can parse the json and get the data they want from there

Comparing pros and cons, I think sending everything in a json in a single sensor should be better solution.

@infeeeee
Copy link
Collaborator

I was thinking about this, and I found more pros for this workflow:

Maybe this multi sensor workflow would be useful elsewhere. MQTT Cover can send position and tilt values in a separate topics similarly: https://www.home-assistant.io/integrations/cover.mqtt/#position_topic But this needs 2 more sensors.

Maybe the ExtraAttributes could become a separate sensor as well, converted to ExtraAttribute json only in HA WH, their data could be visible in other WHs, as ExtraAtrributes now only visible in HA.

To solve the release_summary problem, the Entity should notify HA WH some way to update discovery_payload. EntityData.GetCustomPayload() already exists, with something like that.

So now I think this workflow would be better :)

@richibrics
Copy link
Owner Author

I like this change of idea :)

@richibrics
Copy link
Owner Author

@infeeeee How to place the release summary in just 255 chars ? I'd leave for sure the NO_REMOTE_INSTALL_AVAILABLE_MSG and then the space is really small

@richibrics
Copy link
Owner Author

Another thing, currently the custom payload is set at command register and not updated later

@richibrics
Copy link
Owner Author

Currently it's trimmed to place "..." when additional release summary entries don't fit in 255 chars

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

Successfully merging this pull request may close these issues.

AppInfo Enhancements
2 participants