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

[keba] Robustness improvements on communication error #10399

Merged
merged 4 commits into from
Apr 17, 2021

Conversation

MikeTheTux
Copy link
Contributor

@MikeTheTux MikeTheTux commented Mar 27, 2021

[keba] Robustness improvements on communication error

Fix of issue #10385
Keba Binding is set to offline on communication error (timeout) and changes back to online once the communication is re-established.

Tested:
org.openhab.binding.keba-3.1.0-SNAPSHOT-20210410.zip
Result:

2021-04-10 19:17:52.881 [INFO ] [ab.event.ThingStatusInfoChangedEvent] - Thing 'keba:kecontact:c14a649964' changed from ONLINE to OFFLINE (COMMUNICATION_ERROR): An error occurred while polling the charging station
2021-04-10 19:21:08.041 [INFO ] [ab.event.ThingStatusInfoChangedEvent] - Thing 'keba:kecontact:c14a649964' changed from OFFLINE (COMMUNICATION_ERROR): An error occurred while polling the charging station to ONLINE

MikeTheTux and others added 2 commits March 27, 2021 19:53
@MikeTheTux MikeTheTux requested a review from kgoderis as a code owner March 27, 2021 19:13
@MikeTheTux MikeTheTux changed the title [keba] Robustness improvements on communication error [keba,WIP] Robustness improvements on communication error Mar 28, 2021
long stamp = System.currentTimeMillis();
if (!isKebaReachable()) {
logger.debug("isKebaReachable() timed out after '{}' milliseconds", System.currentTimeMillis() - stamp);
transceiver.unRegisterHandler(getHandler());
Copy link
Member

Choose a reason for hiding this comment

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

I actually believe that this is the issue. The handler is unregistered here, but never registered again afterwards.
Have you tried adding

transceiver.registerHandler(getHandler());

at the top of the else branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I did. In this case the runnable seems to be executed to early. It throws the following exception during startup:

2021-03-30 20:25:55.371 [WARN ] [mmon.WrappedScheduledExecutorService] - Scheduled runnable ended with an exception: 
java.lang.NoSuchMethodError: 'com.google.gson.JsonElement com.google.gson.JsonParser.parseString(java.lang.String)'
	at org.openhab.binding.keba.internal.handler.KeContactHandler.onData(KeContactHandler.java:239) ~[?:?]
	at org.openhab.binding.keba.internal.handler.KeContactHandler.pollingRunnable(KeContactHandler.java:190) ~[?:?]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515) ~[?:?]
	at java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305) ~[?:?]
	at java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128) [?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628) [?:?]
	at java.lang.Thread.run(Thread.java:834) [?:?]

In the solution provided by this PR, the Handler is never unregistered. Thereby it is also not required to register it again, once the connection is re-established.
Only drawback of the current solution: Thing status is not set to offline on communication errors.

Copy link
Member

Choose a reason for hiding this comment

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

Only drawback of the current solution: Thing status is not set to offline on communication errors.

As already mentioned: This is not really acceptable and we need to find a solution for it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the PR with a proper solution.
It changes the state to OFFLINE and back to ONLINE, when the communication is re-established.
My comment #10399 (comment) was crap. The error was caused by the JsonParser update openhab/openhab-core#2244 done on TRUNK while I tested the solution using OH3.0.1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated the solution again :-|
The used UDP communication is not 100% reliable. Even in a switched network, some of the report polling requests do not receive a response. In order to avoid the sporadic OFFLINE/ONLINE events, I introduced isKebaReachable() in the polling runnable again. This function checks the presence of the TCP port (reliable). Only if the TCP port cannot be reached, the Binding goes OFFLINE. It tries to go ONLINE again in the next polling round. In order to change back to ONLINE isKebaReachable() must be successful AND data via UDP must be received.
On UDP timeouts, only a debug message is logged.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds sensible to me.

Signed-off-by: Michael Weger <weger.michael@gmx.net>
@MikeTheTux MikeTheTux changed the title [keba,WIP] Robustness improvements on communication error [keba] Robustness improvements on communication error Apr 7, 2021
@MikeTheTux MikeTheTux requested a review from kaikreuzer April 9, 2021 09:26
Signed-off-by: Michael Weger <weger.michael@gmx.net>
Copy link
Member

@kaikreuzer kaikreuzer left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@kaikreuzer kaikreuzer merged commit fb1d6a2 into openhab:main Apr 17, 2021
@kaikreuzer kaikreuzer added the bug An unexpected problem or unintended behavior of an add-on label Apr 17, 2021
@kaikreuzer kaikreuzer added this to the 3.1 milestone Apr 17, 2021
themillhousegroup pushed a commit to themillhousegroup/openhab2-addons that referenced this pull request May 10, 2021
Signed-off-by: Michael Weger <weger.michael@gmx.net>
Signed-off-by: John Marshall <john.marshall.au@gmail.com>
computergeek1507 pushed a commit to computergeek1507/openhab-addons that referenced this pull request Jul 13, 2021
Signed-off-by: Michael Weger <weger.michael@gmx.net>
frederictobiasc pushed a commit to frederictobiasc/openhab-addons that referenced this pull request Oct 26, 2021
Signed-off-by: Michael Weger <weger.michael@gmx.net>
thinkingstone pushed a commit to thinkingstone/openhab-addons that referenced this pull request Nov 7, 2021
Signed-off-by: Michael Weger <weger.michael@gmx.net>
marcfischerboschio pushed a commit to bosch-io/openhab-addons that referenced this pull request May 5, 2022
Signed-off-by: Michael Weger <weger.michael@gmx.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug An unexpected problem or unintended behavior of an add-on
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants