HASwitchPlate / openHASP-custom-component

Home Assistant custom component for openHASP
https://www.openhasp.com
MIT License
49 stars 9 forks source link

Consider a less rigid version checking policy #87

Closed kquinsland closed 4 months ago

kquinsland commented 2 years ago

Is your feature request related to a problem? Please describe. Shortly after I found openHasp, I ran into a problem with the firmware: my mqtt hostname didn't fit in the allocated memory and the plates could not connect. See: https://github.com/HASwitchPlate/openHASP/issues/265

The fix was quickly rolled out and I was able to test a revised build soon after opening the issue. However, I was quickly hit with another issue: the test firmware was running version 0.7 and I was unable to discover the device and integrate it with Home Assistant.

This is because the device config/registration logic requires an exact match: https://github.com/HASwitchPlate/openHASP-custom-component/blob/762e55fbe1cf8815a1d56ed2a5f55e94d40b79be/custom_components/openhasp/config_flow.py#L97

I installed the custom component via HACS and chose the main release as the most recent release had a 0.6 in the tag. https://hacs.xyz/docs/publish/integration#github-releases-optional

I was hoping that the main branch would work with 0.7 devices since it appeared that there were releases meant specifically for 0.6 devices.

Nope. Even the main code wants 0.6 devices: https://github.com/HASwitchPlate/openHASP-custom-component/blob/762e55fbe1cf8815a1d56ed2a5f55e94d40b79be/custom_components/openhasp/const.py#L5

It was an easy fix: I just modified the custom component install:

root@ha-container [/custom_components/openhasp]# diff const.py const.py.orig 
5,8c5
< # 2022-01-09: trying to get things working with custom FW that I have on the test plate
< #MINOR = "6"
< MINOR = "7"
---
> MINOR = "6"

I was then able to add the latest 0.7 device and have been developing various UI and HA automations against a few different openHasp devices all running some 0.7 build. SO far, no issues with the custom component and the devices.

That got me thinking... how strict does the version check really need to be?

Describe the solution you'd like

This has been discussed on and off on discord over the past few days but I thought I'd create a dedicated thread / issue to track all discussion in a place where more people can see it / comment. 2 weeks from now, nobody is going to be searching through discord for the conversation(s)!

I was initially planning on a simple pull request that would change the version comparison to checking if the openHasp device that was just discovered had a firmware in a list of supported versions. This approach is simple / has the least invasive code changes but is far from ideal. Who is going to maintain the list? What level of "works" will be needed to consider a firmware 'supported'?

Describe alternatives you've considered

Other suggestions:

Additional context

fvanroie commented 2 years ago

I found that commenting out line #102 will let you discover the 0.7 devices. It will still generate the error in the log and warning pop-up, but at least you can discover the device as intended. I think it can be a good start to comment that line in main so we can have users more easily test 0.7. We can still discuss the best approach here.

nagyrobi commented 2 years ago

I think the warning should be "one shot" after HA restart. Once dismissed, shouldn't re-appear again and again.

kquinsland commented 2 years ago

I think the warning should be "one shot" after HA restart. Once dismissed, shouldn't re-appear again and again.

If you assume that old / past versions won't get any cummunity support then you basically have to force people to keep relatively current. Nagging with a simple persistent notification is - i think - the right balance between a nudge to take an action and being supremely disruptive like windows or firefox; windows will just wait until you're not present to do the update and firefox will flat out refuse to open new tabs until you've restarted it.

nagyrobi commented 2 years ago

It's enough if it appears only at each HA restart. Keep in mind that this software is not comparable to single-user scenarios, in best case only one family member handles HA truly at admin level, other users from the household find this very annoying as it appears again and again on their devices too and they can't do anything about it.

One may have a good reason to update firmware to the latest by buiding own binary, while CC is not yet released - but functionality is still good.

fvanroie commented 1 year ago

I think adding a flag (e.g. _firmware_notice) to the device could resolve this issue. The notification is triggered on discovery and the flag is set in the device object. Subsequent notices can then be suppressed if the flag is set and the notice was already sent.

kquinsland commented 1 year ago

_firmware_notice

Does this mean that the plate phones home to check $self.current_version() relative to some public endpoint and then sets some flag for the integration to pick up / display to the user? (this is how Shelly devices work now)

I know that HA did get some internal APIs for alerting to FW updates in preparation for HA being able to check for / push updates to various 802.15 devices so it could be that the integration does the check to figure out what latest/stable is and then compares that to the version of each discovered device?

fvanroie commented 1 year ago

Nope it is still checking what the plate reports versus what the CC version is... _firmware_notice would just prevent the popup message in HA at each discovery message received.

Alternatively, we could just do away with the popup and only add a warning in the LOGs...

fvanroie commented 4 months ago

This has now been made into a WARNING in the logs.