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

Consider a less rigid version checking policy #87

Closed
kquinsland opened this issue Jan 18, 2022 · 8 comments
Closed

Consider a less rigid version checking policy #87

kquinsland opened this issue Jan 18, 2022 · 8 comments

Comments

@kquinsland
Copy link

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: HASwitchPlate/openHASP#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:

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:

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:

  • Only check the major version, all versions within the same major version should be accepted, it's up to the users to check logs to see if anything did go wrong
  • If the reported firmware is more than 1 minor version, don't accept. E.G.: CC wants .6, device has .7 and since math.abs(.6-.7) <= .1, the device is accepted. If the CC wants .6 but the device has .4, that's a delta of 2 and 2 > 1 so device is rejected.
  • Semantic versioning in the yyyy-mm format to track the same version naming scheme that home assistant uses, use similar logic as above and only accept devices that have firmware +/- a few months from the desired...

Additional context

@fvanroie
Copy link
Collaborator

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
Copy link
Collaborator

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

@kquinsland
Copy link
Author

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
Copy link
Collaborator

nagyrobi commented Jan 24, 2022

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
Copy link
Collaborator

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
Copy link
Author

_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
Copy link
Collaborator

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
Copy link
Collaborator

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

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

No branches or pull requests

3 participants