[veSync] 131 and Vital Purifiers base support

[dag81]

Adjustments to add base support for Purifiers 131 and Vital model's
Signed-off-by: David Goodyear david.goodyear@gmail.com

This correct's support for two 131 based models. It has been tested and updated via simulations of a proven working interface PyVeSync against the payloads generated from this updated revision.

This add's support for 2 new Vital named devices - the 100S and 200S.

Corrected humidifier mode support for 2 device models, added additional validation to humidifier support to ensure the new extra pet mode is not used, and apply restrictions based on the pyvesync lib setups.

Title

• [veSync] Improve support for PUR131 devices
• [veSync] Add base support for Vital 100S
• [veSync] Add base support for Vital 200S
• [veSync] Bug fix to stop console errors when manual scanning
• [veSync] Add warm mist level for Oasis and 600S humidifiers models
• [veSync] Updated Oasis and 600S to use humifier mode at protocol level for Auto in OpenHab
• [veSync] Add's support for Oasis 1000 model

Description

This is a improvement to potentially add the support of PUR131 devices. The motivation is attempting to add missing devices to complete the support of the binding across the entire range.

This also add's base support for 2 new devices the Vital 100S and Vital 200S.

Includes bug fix, after cross checking the hue binding - added check for a null bridge reference, to prevent null pointer errors appearing.

Add's updates to support writing to the warm mist channel, tested via simulations again, against the pyvesync system.

Testing

Testing for this has been done by writing a simulator that interfaces to a know good implementation PyVeSync. The simulator then allowed cross comparison of its behaviors via this one to ensure they both interact with the external system correctly. The changes in this align the support for the PUR131 devices, so that they should work. The Vital support is based on simulations as well, and a few channel's have intentionally been removed for now, until issues in PyVeSync are closed and any adjustments made, and therefore mapped to this binding. The Vital support implemented has been confirmed as working in PyVeSync looking through the testing threads.

[dag81]

I suspect the checks failed due to issues outside of this PR. The initial commit passed all checks. This one only has a README.md update, and spotless:apply locally does not change the file that was committed. How can I retry the checks for the PR, to get the necessary ticks?

[dag81]

Converted back to draft, as no ones assigned yet, to save adding another PR later on, for 2 more set's of devices. Please advise on the first question however - for future reference - looks like it fixed between checkin's.

[openhab-bot]

This pull request has been mentioned on openHAB Community. There might be relevant details there:

https://community.openhab.org/t/vesync-levoit-binding-help-testing-requested/144175/18

[lsiepel]

@dag81 i might review this, but before that review, this PR has to build succesfull wihtout new SAT errors or warnigs. Can you sync your branches with openhab main, fix the conflicts and try to build it? Also the license headers should change to 2024.
If you need any help with those steps, please let me know.
Edit: Thing-upgrade instructison have to be added as some existings Things have channels added. See here: https://www.openhab.org/docs/developer/bindings/thing-xml.html#updating-thing-types

[dag81]

@lsiepel this one is ready again when you are ready.

[lsiepel]

Thanks, i left some comments, nothing big.

[dag81]

@lsiepel - Regarding the bits about the units. That a interesting one. Everyone I've added the unitHint of "one" to, is to ensure it doesn't present the value as a % which the system now appears to do since the UoM model came into play.

Really everything that has had that unitHint added as "one" should generally be a DecimalType rather than a Dimensionless number, to sort this out. However, rather than be inconsistent in the binding I kept it as it was, as this would def. be a breaking change.

Correcting the new one I suspect would actually confuse people more? I could do the whole lot but that would break most peoples configurations at that point, and that's the primarily reason I held off updating them all adding the missing area unit. I'd rather go through them all and update if this is the correct approach and acceptable given the amount it would break config's. In this case I kept the incorrect unit for consistency with rather than implementing a sweeping breaking change.

What do you reckon - align it all with several breaking changes, or keep it consistently wrong?

[lsiepel]

While reading and thinking about your post, I came to this:
We prefer to have scoped and specific PR’s as that makes it easier to review, reduces riscs and improves the visibility in the release notes.

This PR is not only adding support for some devices but also corrects other things. While greatly appreciated, users won’t expect breaking changes to their 600 device based on the release notes title that states that it add support for 100 device.

That being said I think the breaking changes need to happen and they are relative simple to correct. If possible it is best to extract those changes to a separate PR, if that is ok with you.

[dag81]

@lsiepel I couldn't see how to change the state to request another pass - I think it's good for another pass to see if the changes done and not done seem sensible, whenever you are ready please have a quick look. Thank you again!!! ;)

[dag81]

While reading and thinking about your post, I came to this: We prefer to have scoped and specific PR’s as that makes it easier to review, reduces riscs and improves the visibility in the release notes.

This PR is not only adding support for some devices but also corrects other things. While greatly appreciated, users won’t expect breaking changes to their 600 device based on the release notes title that states that it add support for 100 device.

That being said I think the breaking changes need to happen and they are relative simple to correct. If possible it is best to extract those changes to a separate PR, if that is ok with you.

Yeah I was thinking before of doing a second pass on this one, as I can see further room to clean it up including those channel definitions. I did correct the Numer:Area one to the correct units, so I believe that's the only breaking change. All of the other's, I'll modify to basic Numbers and re-test, for a new PR. I presume this is what you're thinking as well from the comments?

P.S For some reason I only just saw this an hour after you posted it so apologies on the delay replying.

[dag81]

Just adding a comment since the last update. Apologies I had to force-push to over-write a mistaken push from a different local copy, used during testing. On comparing the protocol implementation ported from well over a year ago, it looks like they found some variances for the EU regional model. So this last change is to, add the bespoke behaviour for that variant, and ensure the units are identified correctly. (Another one to look at on my next refactor - cleaning up this identification behaviour further to a prioritised 2-pass approach). Behaviours verified against the spec using the simulator - so this should be fine for the Mist Humidifiers, based on the latest data available.

[dag81]

@lsiepel okay I think I found it - it was the local git config setup that didn't have David Goodyear but dag81, so the DCO check failed. I've updated the latest commit. If its all squashed when merged, I presume it will take the sign-off from that one?

[lsiepel]

@lsiepel okay I think I found it - it was the local git config setup that didn't have David Goodyear but dag81, so the DCO check failed. I've updated the latest commit. If its all squashed when merged, I presume it will take the sign-off from that one?

Yes, me or another maintainer will pick that commit.

[dag81]

Hi @lsiepel, is there anything I've missed on this PR. I think its good for another look by someone. Just wanted to check it wasn't blocked by myself - am aware the cut-off is very soon :)

[lsiepel]

Thanks, LGTM. As this is an extensive PR, and we are close to a stable release, do you need some more testing? Is this changed tested by other users? Just want to prevent regressions here.

[dag81]

Thanks, LGTM. As this is an extensive PR, and we are close to a stable release, do you need some more testing? Is this changed tested by other users? Just want to prevent regressions here.

@lsiepel Yeah this should be fine. Most of it has been tested by different users over the years, because this approach solves a lot of device identification issues, with regional model's the original ID system didn't have which was ported over, so it should be good to merge. there's no bespoke functionality like LinkTap with the new MDNS usage so I don't believe regression should be impacted at all by this merge, its standard injection of services that have been around for years.

[lsiepel]

Thanks