[avmfritz] Added initial refresh of Call Monitor channels and improved thread handling #9734

cweitkamp · 2021-01-07T13:28:43Z

Added initial refresh of Call Monitor channels
Improved CallMonitorThread handling

Signed-off-by: Christoph Weitkamp github@christophweitkamp.de

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

maniac103 · 2021-01-08T09:29:24Z

...binding.avmfritz/src/main/java/org/openhab/binding/avmfritz/internal/handler/BoxHandler.java

-        } else if (this.callMonitor != null && !callChannelsLinked()) {
-            CallMonitor cm = this.callMonitor;
+            // initialize states of call monitor channels
+            scheduler.schedule(this::refreshCallChannels, refreshInterval, TimeUnit.SECONDS);


Wouldn't it be more elegant to do this in the constructor of CallMonitorThread?

Nice idea but I do not think that it makes a difference because we just set default values without querying the real data (which btw is not possible as the call Monitor is an event based communication).

It probably doesn't, it just would avoid potential race conditions since this refresh calls happens asynchronously, and so does the thread creation. If a call came in right after thread creation, we might wrongly reset the state during the call.
Hypothetical, I know, but moving the initialization avoids that scenario :-)

That is a valid point. On the opposite side we have the 2 hour recreation of the thread which will then lead to repeated initialization ... Might happen during an active call too.

Good point. Maybe do the initialization in the CallMonitor constructor then?

It is not a real thread, but periodically scheduled jobs. What I mean is that the variable refreshInterval is a protected property within the AVMFritzBaseThingHandler and I have no access to it in CallMonitor constructor unless I add a method to expose it.

Why do you need refreshInterval though? I thought the point of that initial refresh is to have the channels have undef/idle instead of NULL on startup. Is that correct? If so, how is the timing of that initialization dependent on the box poll interval?

It can be 5sec either. We just need a short time span to wait for the thread to be started as ist is done asynchronously too.

It can be 5sec either

I figured so, but in that case you don't need to access refreshInterval and could initialize the channels in the CallMonitor constructor (in the main thread, before starting the call monitor thread) just fine.
The method for resetting the channels could be moved from CallMonitorThread to CallMonitor (basically remove refreshChannels and move resetChannels to its place), since the former can also call into the latter.

I changed it.

...binding.avmfritz/src/main/java/org/openhab/binding/avmfritz/internal/handler/BoxHandler.java

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

...z/src/main/java/org/openhab/binding/avmfritz/internal/handler/AVMFritzBaseBridgeHandler.java

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

maniac103

LGTM now 👍

cpmeister · 2021-01-13T23:11:21Z

...ng.avmfritz/src/main/java/org/openhab/binding/avmfritz/internal/callmonitor/CallMonitor.java

@@ -106,7 +113,7 @@ public void run() {
                    reader = new BufferedReader(new InputStreamReader(socket.getInputStream()));
                    // reset the retry interval
                    reconnectTime = 60000L;
-                } catch (Exception e) {
+                } catch (IOException e) {


Since this is a custom thread uncaught runtime exceptions will not be logged if they occur. To make troubleshooting easier I think that you should add an UncaughtExceptionHandler to the thread so that unexpected thread deaths can be logged.

Something like 1533478?

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

maniac103 · 2021-01-14T16:57:56Z

...ng.avmfritz/src/main/java/org/openhab/binding/avmfritz/internal/callmonitor/CallMonitor.java

+            public void uncaughtException(@Nullable Thread thread, @Nullable Throwable throwable) {
+                logger.debug("Lost connection to FRITZ!Box because of an uncaught exception: ", throwable);
+                handler.setStatusInfo(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, String.format(
+                        "Lost connection to FRITZ!Box: %s", throwable == null ? "null" : throwable.getMessage()));


Isn't this a bit harsh? My suggestion would be making CallMonitor implement the uncaught exception handler interface and from there logging the error and respawning the thread.

Isn't the 2h reconnect job sufficient?

It probably is, but I don't really see why we need to wait with reconnection when we

know we need reconnection anyway and

there is a trivial way to do that reconnection?

Things would be different if we needed to write a lot of code to respawn the thread, but I don't see how that would be the case here.

I usually implement the UncaughtExceptionHandler as a lambda.

@cpmeister Done.

@maniac103 Yes, valid points. But I think the 2h reconnect is enough. I now changed the LOG level to warn and do not set the handler to OFFLINE anymore.

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

cpmeister

LGTM

…d thread handling (openhab#9734) * Added initial refresh of Call Monitor channels

…d thread handling (openhab#9734) * Added initial refresh of Call Monitor channels Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de> Signed-off-by: John Marshall <john.marshall.au@gmail.com>

…d thread handling (openhab#9734) * Added initial refresh of Call Monitor channels Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

Added initial refresh of Call Monitor channels

239b0f1

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

cweitkamp added the enhancement An enhancement or new feature for an existing add-on label Jan 7, 2021

cweitkamp mentioned this pull request Jan 7, 2021

[avmfritz] Call monitoring does not work #9712

Closed

Improved CallMonitorThread handling

d03174b

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

cweitkamp added bug An unexpected problem or unintended behavior of an add-on and removed enhancement An enhancement or new feature for an existing add-on labels Jan 8, 2021

cweitkamp changed the title ~~[avmfritz] Added initial refresh of Call Monitor channels~~ [avmfritz] Added initial refresh of Call Monitor channels and improved thread handling Jan 8, 2021

maniac103 reviewed Jan 8, 2021

View reviewed changes

wborn added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 8, 2021

maniac103 reviewed Jan 11, 2021

View reviewed changes

...binding.avmfritz/src/main/java/org/openhab/binding/avmfritz/internal/handler/BoxHandler.java Outdated Show resolved Hide resolved

Incorporated commentd from review

f3a311a

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

cweitkamp force-pushed the feature-avmfritz-refresh-call-monitor-channels branch from 2df4067 to f3a311a Compare January 12, 2021 07:42

maniac103 reviewed Jan 12, 2021

View reviewed changes

...z/src/main/java/org/openhab/binding/avmfritz/internal/handler/AVMFritzBaseBridgeHandler.java Outdated Show resolved Hide resolved

Incorporated commentd from review

cedb203

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

maniac103 approved these changes Jan 12, 2021

View reviewed changes

cpmeister requested changes Jan 13, 2021

View reviewed changes

cweitkamp added 2 commits January 14, 2021 16:48

Added UncaughtExceptionHandler

1533478

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

Incorporated comments from review

915f8d4

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

maniac103 reviewed Jan 14, 2021

View reviewed changes

Incorporated comments from review

b845dc6

Signed-off-by: Christoph Weitkamp <github@christophweitkamp.de>

cpmeister approved these changes Jan 15, 2021

View reviewed changes

cpmeister added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 15, 2021

cweitkamp added rebuild Triggers Jenkins PR build and removed rebuild Triggers Jenkins PR build labels Jan 16, 2021

cpmeister merged commit 3c27aeb into openhab:main Jan 17, 2021

cpmeister added this to the 3.1 milestone Jan 17, 2021

cweitkamp deleted the feature-avmfritz-refresh-call-monitor-channels branch January 18, 2021 05:49

nowaterman pushed a commit to nowaterman/openhab-addons that referenced this pull request Jan 19, 2021

[avmfritz] Added initial refresh of Call Monitor channels and improve…

e061c24

…d thread handling (openhab#9734) * Added initial refresh of Call Monitor channels

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[avmfritz] Added initial refresh of Call Monitor channels and improved thread handling #9734

[avmfritz] Added initial refresh of Call Monitor channels and improved thread handling #9734

cweitkamp commented Jan 7, 2021 •

edited

maniac103 Jan 8, 2021

cweitkamp Jan 8, 2021

maniac103 Jan 8, 2021

cweitkamp Jan 8, 2021

maniac103 Jan 8, 2021

cweitkamp Jan 8, 2021

maniac103 Jan 8, 2021

cweitkamp Jan 11, 2021

maniac103 Jan 11, 2021 •

edited

cweitkamp Jan 12, 2021

maniac103 left a comment

cpmeister Jan 13, 2021

cweitkamp Jan 14, 2021

maniac103 Jan 14, 2021

cweitkamp Jan 14, 2021

maniac103 Jan 14, 2021 •

edited

cpmeister Jan 15, 2021

cweitkamp Jan 15, 2021

cpmeister left a comment

[avmfritz] Added initial refresh of Call Monitor channels and improved thread handling #9734

[avmfritz] Added initial refresh of Call Monitor channels and improved thread handling #9734

Conversation

cweitkamp commented Jan 7, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maniac103 Jan 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maniac103 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maniac103 Jan 14, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpmeister left a comment

Choose a reason for hiding this comment

cweitkamp commented Jan 7, 2021 •

edited

maniac103 Jan 11, 2021 •

edited

maniac103 Jan 14, 2021 •

edited