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.

Sign up for GitHub

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

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

Merged
merged 7 commits into from
Jan 17, 2021
Merged

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
239b0f1
Added initial refresh of Call Monitor channels
cweitkamp Jan 7, 2021
d03174b
Improved CallMonitorThread handling
cweitkamp Jan 8, 2021
f3a311a
Incorporated commentd from review
cweitkamp Jan 12, 2021
cedb203
Incorporated commentd from review
cweitkamp Jan 12, 2021
1533478
Added UncaughtExceptionHandler
cweitkamp Jan 14, 2021
915f8d4
Incorporated comments from review
cweitkamp Jan 14, 2021
b845dc6
Incorporated comments from review
cweitkamp Jan 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 30 additions & 20 deletions ...g.avmfritz/src/main/java/org/openhab/binding/avmfritz/internal/callmonitor/CallEvent.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,11 @@
@NonNullByDefault
public class CallEvent {

public static final String CALL_TYPE_CALL = "CALL";
public static final String CALL_TYPE_CONNECT = "CONNECT";
public static final String CALL_TYPE_RING = "RING";
public static final String CALL_TYPE_DISCONNECT = "DISCONNECT";

private final String rawEvent;
private final String timestamp;
private final String callType;
Expand All @@ -47,26 +52,31 @@ public CallEvent(String rawEvent) {
callType = fields[1];
id = fields[2];

if (callType.equals("RING")) {
externalNo = fields[3];
internalNo = fields[4];
connectionType = fields[5];
} else if (callType.equals("CONNECT")) {
line = fields[3];
if (fields.length > 4) {
externalNo = fields[4];
} else {
externalNo = "Unknown";
}
} else if (callType.equals("CALL")) {
line = fields[3];
internalNo = fields[4];
externalNo = fields[5];
connectionType = fields[6];
} else if (callType.equals("DISCONNECT")) {
// no fields to set
} else {
throw new IllegalArgumentException("Invalid call type: " + callType);
switch (callType) {
case CALL_TYPE_RING:
externalNo = fields[3];
internalNo = fields[4];
connectionType = fields[5];
break;
case CALL_TYPE_CONNECT:
line = fields[3];
if (fields.length > 4) {
externalNo = fields[4];
} else {
externalNo = "Unknown";
}
break;
case CALL_TYPE_CALL:
line = fields[3];
internalNo = fields[4];
externalNo = fields[5];
connectionType = fields[6];
break;
case CALL_TYPE_DISCONNECT:
// no fields to set
break;
default:
throw new IllegalArgumentException("Invalid call type: " + callType);
}
}

Expand Down
103 changes: 54 additions & 49 deletions ...avmfritz/src/main/java/org/openhab/binding/avmfritz/internal/callmonitor/CallMonitor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,8 @@
*/
package org.openhab.binding.avmfritz.internal.callmonitor;

import static org.openhab.binding.avmfritz.internal.AVMFritzBindingConstants.*;

import java.io.BufferedReader;
import java.io.IOException;
import java.io.InputStreamReader;
Expand All @@ -22,7 +24,6 @@

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
import org.openhab.binding.avmfritz.internal.AVMFritzBindingConstants;
import org.openhab.binding.avmfritz.internal.handler.BoxHandler;
import org.openhab.core.library.types.StringListType;
import org.openhab.core.thing.ThingStatus;
Expand All @@ -32,7 +33,7 @@
import org.slf4j.LoggerFactory;

/**
* This class handles all communication with the call monitor port of the fritzbox.
* This class handles all communication with the Call Monitor port of the FRITZ!Box.
*
* @author Kai Kreuzer - Initial contribution
*/
Expand All @@ -41,8 +42,8 @@ public class CallMonitor {

protected final Logger logger = LoggerFactory.getLogger(CallMonitor.class);

// port number to connect to fritzbox
private final int MONITOR_PORT = 1012;
// port number to connect to FRITZ!Box
private static final int MONITOR_PORT = 1012;

private @Nullable CallMonitorThread monitorThread;
private final ScheduledFuture<?> reconnectJob;
Expand Down Expand Up @@ -70,15 +71,22 @@ public CallMonitor(String ip, BoxHandler handler, ScheduledExecutorService sched
}, 0, 2, TimeUnit.HOURS);
}

/**
* Refresh channels.
*/
public void refreshChannels() {
CallMonitorThread thread = this.monitorThread;
if (thread != null) {
thread.resetChannels();
}
}

/**
* Cancel the reconnect job.
*/
public void dispose() {
reconnectJob.cancel(true);
CallMonitorThread monitorThread = this.monitorThread;
if (monitorThread != null) {
monitorThread.interrupt();
}
stopThread();
}

public class CallMonitorThread extends Thread {
Expand Down Expand Up @@ -152,6 +160,16 @@ public void run() {
}
}

/**
* Resets states of call monitor channels
*/
public void resetChannels() {
handler.updateState(CHANNEL_CALL_INCOMING, UnDefType.UNDEF);
handler.updateState(CHANNEL_CALL_OUTGOING, UnDefType.UNDEF);
handler.updateState(CHANNEL_CALL_ACTIVE, UnDefType.UNDEF);
handler.updateState(CHANNEL_CALL_STATE, CALL_STATE_IDLE);
}

/**
* Close socket and stop running thread.
*/
Expand All @@ -176,54 +194,41 @@ public void interrupt() {
* @param ce call event to process
*/
private void handleCallEvent(CallEvent ce) {
if (ce.getCallType().equals("DISCONNECT")) {
// reset states of call monitor channels
handler.updateState(AVMFritzBindingConstants.CHANNEL_CALL_INCOMING, UnDefType.UNDEF);
handler.updateState(AVMFritzBindingConstants.CHANNEL_CALL_OUTGOING, UnDefType.UNDEF);
handler.updateState(AVMFritzBindingConstants.CHANNEL_CALL_ACTIVE, UnDefType.UNDEF);
handler.updateState(AVMFritzBindingConstants.CHANNEL_CALL_STATE,
AVMFritzBindingConstants.CALL_STATE_IDLE);
} else if (ce.getCallType().equals("RING")) { // first event when call is incoming
StringListType state = new StringListType(ce.getInternalNo(), ce.getExternalNo());
handler.updateState(AVMFritzBindingConstants.CHANNEL_CALL_INCOMING, state);
handler.updateState(AVMFritzBindingConstants.CHANNEL_CALL_OUTGOING, UnDefType.UNDEF);
handler.updateState(AVMFritzBindingConstants.CHANNEL_CALL_ACTIVE, UnDefType.UNDEF);
handler.updateState(AVMFritzBindingConstants.CHANNEL_CALL_STATE,
AVMFritzBindingConstants.CALL_STATE_RINGING);
} else if (ce.getCallType().equals("CONNECT")) { // when call is answered/running
StringListType state = new StringListType(ce.getExternalNo(), "");
handler.updateState(AVMFritzBindingConstants.CHANNEL_CALL_ACTIVE, state);
handler.updateState(AVMFritzBindingConstants.CHANNEL_CALL_INCOMING, UnDefType.UNDEF);
handler.updateState(AVMFritzBindingConstants.CHANNEL_CALL_OUTGOING, UnDefType.UNDEF);
handler.updateState(AVMFritzBindingConstants.CHANNEL_CALL_STATE,
AVMFritzBindingConstants.CALL_STATE_ACTIVE);
} else if (ce.getCallType().equals("CALL")) { // outgoing call
StringListType state = new StringListType(ce.getExternalNo(), ce.getInternalNo());
handler.updateState(AVMFritzBindingConstants.CHANNEL_CALL_INCOMING, UnDefType.UNDEF);
handler.updateState(AVMFritzBindingConstants.CHANNEL_CALL_OUTGOING, state);
handler.updateState(AVMFritzBindingConstants.CHANNEL_CALL_ACTIVE, UnDefType.UNDEF);
handler.updateState(AVMFritzBindingConstants.CHANNEL_CALL_STATE,
AVMFritzBindingConstants.CALL_STATE_DIALING);
switch (ce.getCallType()) {
case CallEvent.CALL_TYPE_DISCONNECT:
// reset states of call monitor channels
resetChannels();
break;
case CallEvent.CALL_TYPE_RING: // first event when call is incoming
handler.updateState(CHANNEL_CALL_INCOMING,
new StringListType(ce.getInternalNo(), ce.getExternalNo()));
handler.updateState(CHANNEL_CALL_OUTGOING, UnDefType.UNDEF);
handler.updateState(CHANNEL_CALL_ACTIVE, UnDefType.UNDEF);
handler.updateState(CHANNEL_CALL_STATE, CALL_STATE_RINGING);
break;
case CallEvent.CALL_TYPE_CONNECT: // when call is answered/running
handler.updateState(CHANNEL_CALL_ACTIVE, new StringListType(ce.getExternalNo(), ""));
handler.updateState(CHANNEL_CALL_INCOMING, UnDefType.UNDEF);
handler.updateState(CHANNEL_CALL_OUTGOING, UnDefType.UNDEF);
handler.updateState(CHANNEL_CALL_STATE, CALL_STATE_ACTIVE);
break;
case CallEvent.CALL_TYPE_CALL: // outgoing call
handler.updateState(CHANNEL_CALL_INCOMING, UnDefType.UNDEF);
handler.updateState(CHANNEL_CALL_OUTGOING,
new StringListType(ce.getExternalNo(), ce.getInternalNo()));
handler.updateState(CHANNEL_CALL_ACTIVE, UnDefType.UNDEF);
handler.updateState(CHANNEL_CALL_STATE, CALL_STATE_DIALING);
break;
}
}
}

public void stopThread() {
logger.debug("Stopping call monitor thread...");
if (monitorThread != null) {
monitorThread.interrupt();
monitorThread = null;
}
}

public void startThread() {
logger.debug("Starting call monitor thread...");
if (monitorThread != null) {
monitorThread.interrupt();
CallMonitorThread thread = this.monitorThread;
if (thread != null) {
thread.interrupt();
monitorThread = null;
}
// create a new thread for listening to the FritzBox
monitorThread = new CallMonitorThread();
monitorThread.start();
}
}
2 changes: 1 addition & 1 deletion ...rc/main/java/org/openhab/binding/avmfritz/internal/handler/AVMFritzBaseBridgeHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ public abstract class AVMFritzBaseBridgeHandler extends BaseBridgeHandler {
/**
* Refresh interval which is used to poll values from the FRITZ!Box web interface (optional, defaults to 15 s)
*/
private long refreshInterval = 15;
protected long refreshInterval = 15;
cweitkamp marked this conversation as resolved.
Show resolved Hide resolved

/**
* Interface object for querying the FRITZ!Box web interface
Expand Down
23 changes: 20 additions & 3 deletions ...ding.avmfritz/src/main/java/org/openhab/binding/avmfritz/internal/handler/BoxHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
import static org.openhab.binding.avmfritz.internal.AVMFritzBindingConstants.*;

import java.util.Set;
import java.util.concurrent.TimeUnit;

import org.eclipse.jdt.annotation.NonNullByDefault;
import org.eclipse.jdt.annotation.Nullable;
Expand Down Expand Up @@ -56,10 +57,12 @@ public BoxHandler(Bridge bridge, HttpClient httpClient,
@Override
protected void manageConnections() {
AVMFritzBoxConfiguration config = getConfigAs(AVMFritzBoxConfiguration.class);
if (this.callMonitor == null && callChannelsLinked()) {
CallMonitor cm = this.callMonitor;
if (cm == null && callChannelsLinked()) {
this.callMonitor = new CallMonitor(config.ipAddress, this, scheduler);
} else if (this.callMonitor != null && !callChannelsLinked()) {
CallMonitor cm = this.callMonitor;
// initialize states of call monitor channels
scheduler.schedule(this::refreshCallChannels, refreshInterval, TimeUnit.SECONDS);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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).

Copy link
Contributor

Choose a reason for hiding this comment

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

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 :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@maniac103 maniac103 Jan 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

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 changed it.

} else if (cm != null && !callChannelsLinked()) {
cm.dispose();
this.callMonitor = null;
}
Expand Down Expand Up @@ -95,4 +98,18 @@ public void dispose() {
public void updateState(String channelID, State state) {
super.updateState(channelID, state);
}

@Override
public void handleRefreshCommand() {
refreshCallChannels();
super.handleRefreshCommand();
}

private void refreshCallChannels() {
CallMonitor cm = this.callMonitor;
if (cm != null && callChannelsLinked()) {
cweitkamp marked this conversation as resolved.
Show resolved Hide resolved
// initialize states of call monitor channels
cm.refreshChannels();
}
}
}