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

[velux] Implement new API, and log critical device errors #13212

Merged
merged 11 commits into from Aug 12, 2022

Conversation

andrewfg
Copy link
Contributor

@andrewfg andrewfg commented Aug 3, 2022

Background

During the development and testing of #12618 the following things were discovered concerning the older API calls on which the original binding was based, versus newer API calls which were introduced later..

  1. The older calls do not return proper position info for devices with vanes, whereas the newer calls do.
  2. The older calls are much slower to update to the real position information of devices than the new calls.

Consequently in #12618 it was decided to use the newer API calls for devices with vanes. However it was decided to keep using the older API calls for devices without vanes; although issue #13123 was opened to report the slow status updating on such devices.

In the meantime further testing confirmed that the newer API calls function correctly for both devices with and without vanes. And it also confirmed that the newer API does indeed speed up the real status updating of both types of devices.

Furthermore during the testing of #12618 it was noticed that the newer API call (in addition to the benefits mentioned above) also provides additional status information StatusReply concerning the physical state of the devices. Some of this status information concerns temporary faults, and some concerns critical faults in the devices that might require physical maintenance. For this reason issue #13205 was opened.

PS it should also be noted that the Home Assistant integration is also based on the newer API calls; thus providing further test evidence that the newer calls function properly.

Solution

This PR does three things..

  1. The newer API calls are now used for polling the status of all devices (both those with, and now without vanes).
  2. And therefore the status updating of all types of devices is faster than before.
  3. The StatusReply information from the newer API calls is used to log critical errors to warn (and non critical errors to debug).

Fixes #13123
Fixes #13205

Signed-off-by: Andrew Fiddian-Green software@whitebear.ch

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg andrewfg added the enhancement An enhancement or new feature for an existing add-on label Aug 3, 2022
@andrewfg andrewfg requested a review from gs4711 as a code owner August 3, 2022 14:39
@andrewfg andrewfg requested a review from jlaur August 3, 2022 14:59
Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

Just one small comment

@andrewfg
Copy link
Contributor Author

andrewfg commented Aug 9, 2022

@lolodomo do you have any thoughts on this comment #13205 (comment) currently I am differentiating between log warn for critical errors and log debug for all others. But the decision "critical" is my personal judgement based on how I interpret the names of the errors; whereas by contrast @gs4711 is suggesting to log them all to warn. WDYT?

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
@andrewfg
Copy link
Contributor Author

do you have any thoughts on this comment #13205 (comment)

In today's commits I changed logger.debug() => logger.info()

Copy link
Contributor

@gs4711 gs4711 left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks @andrewfg !

@lolodomo
Copy link
Contributor

In today's commits I changed logger.debug() => logger.info()

INFO level should be rarely used.
You have to be sure that this log cannot be repeated and finally flood the logs.
You have to be sure that this log will have an interest for all users of this binding.

By the way, I will not delay the merge. If necessary, you can revert later.

Copy link
Contributor

@lolodomo lolodomo left a comment

Choose a reason for hiding this comment

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

LGTM with a remark about the new INFO log introduced.

@lolodomo lolodomo merged commit b075d37 into openhab:main Aug 12, 2022
@lolodomo lolodomo added this to the 3.4 milestone Aug 12, 2022
@andrewfg
Copy link
Contributor Author

@lolodomo many thanks for merging.

You have to be sure that this log cannot be repeated and finally flood the logs.

These events are extremely rare: so this requirement is indeed assured :)

You have to be sure that this log will have an interest for all users of this binding.

They indicate something physically broken in the window/shutter: so this requirement is also assured :)

@andrewfg andrewfg deleted the 13123-velux-fast-update branch September 26, 2022 16:21
leifbladt pushed a commit to leifbladt/openhab-addons that referenced this pull request Oct 15, 2022
)

* [velux] use new API, and log StatusReply codes
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
andan67 pushed a commit to andan67/openhab-addons that referenced this pull request Nov 6, 2022
)

* [velux] use new API, and log StatusReply codes
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
andrasU pushed a commit to andrasU/openhab-addons that referenced this pull request Nov 12, 2022
)

* [velux] use new API, and log StatusReply codes
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Signed-off-by: Andras Uhrin <andras.uhrin@gmail.com>
psmedley pushed a commit to psmedley/openhab-addons that referenced this pull request Feb 23, 2023
)

* [velux] use new API, and log StatusReply codes
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
nemerdaud pushed a commit to nemerdaud/openhab-addons that referenced this pull request Feb 28, 2023
)

* [velux] use new API, and log StatusReply codes
Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>

Signed-off-by: Andrew Fiddian-Green <software@whitebear.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement An enhancement or new feature for an existing add-on
Projects
None yet
3 participants