[velux] Stability checks and improvements in slip io

[andrewfg]

Background

The Velux KLF200 hub is notoriously unpredictable in its communications.

On the OH community forum, various users have recently reported communication issues. But unfortunately the exact cause of the issues if often hard to nail down; and often not repeatable on my own test system; so hard to debug.

Solution

This PR is a step to try to improve the situation by the following means..

• Some 'hardening' of the Slip IO communications to handle possible (but unexpected) communication edge cases.
• Extra logging of potential Slip IO errors that should not happen, so we can detect if they do ever actually occur.

Signed-off-by: Andrew Fiddian-Green [email protected]

[andrewfg]

@gs4711 feedback appreciated

[openhab-bot]

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

https://community.openhab.org/t/velux-new-openhab2-binding-feedback-welcome/32926/1223

[gs4711]

Look fine.... unfortuntelly this addition of debugging code seems to be reasonable.

[andrewfg]

unfortuntelly this addition of debugging code seems to be reasonable

:)

Indeed Guenther. Just so you know it, during my tests I discovered the following..

The Slip Encoding is described in the Velux API specification as below. And my idea was to check the correct full syntax of all elements in the Slip telegrams (including LL and KK). But I found that even so called 'good' telegrams are non compliant because, for example, the LL value is wrong (e.g. 0x10 instead of 0x12). So finally I decided to forego the full syntax check (because it definitely does not comply), and instead just check for valid MM and PP markers..

MM PP LL CC CC DD .. DD KK MM

where..
MM = Slip marker (at both start and end of telegram)
PP = Protocol ID
LL = Length of telegram
CC = Command
DD = Data (if any)
KK = Check sum

[andrewfg]

Still not happy with the stability?

Unfortunately not. Aargh!

I installed OH3.1.x Milestone 2 just now, and one of my two KLF bridges was left in a zombie state after the restart; which required a manual power cycle to resolve. The two KLF bridges are identical (except that they control different windows and shutters) so there is no clear reason why one bridge would restart cleanly and the other not.

The commit just now is to ensure that the input buffer has been fully emptied before issuing the socket close command; although I can't really see why it would make much difference.

[andrewfg]

Ping!

[openhab-bot]

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

https://community.openhab.org/t/velux-new-openhab2-binding-feedback-welcome/32926/1246

[andrewfg]

@openhab/add-ons-maintainers could I kindly request someone to review this PR please?

[martinvw]

I'm a bit in doubt about these log statements in the light of https://www.openhab.org/docs/developer/guidelines.html#f-logging

If there are no other complaints, I would say we should merge it

[andrewfg]

Hi @martinvw I do understand that the developer guideline says one should not use the logger to replace the debugger. But our problem is that the Velux bridge has an undocumented weakness whereby it sometimes (very infrequently) falls into a "zombie state" that requires the bridge to be physically power cycled. Users of the binding have been trying to track down the exact circumstances when such zombie states occur, but it is very infrequent, so I can't expect normal users to be trying to run the binding in a debugger. That is the reason why I added these logger.trace() commands instead. So I do hope you that can allow this please.

[martinvw]

@andrewfg could you fix the merge conflict? If no-one else steps it with comments I could merge if after that. Feel free to pine me once you fixed the merge conflict.

[martinvw]

When people are willing to run the binding in trace mode you could of course also provide a separate version for that.

Given no one else stood up against it, I propose to merge it.

[andrewfg]

Given no one else stood up against it, I propose to merge it.

Thank you :)

I have also resolved the merge conflict.