From 86d3991e7cdb55faad693bfcf3adc0a8779a1e9a Mon Sep 17 00:00:00 2001 From: Gwendal Roulleau Date: Mon, 26 Sep 2022 23:06:32 +0200 Subject: [PATCH] [smsmodem] Apply review Signed-off-by: Gwendal Roulleau --- .../internal/SMSModemBridgeConfiguration.java | 2 +- .../internal/actions/SMSModemActions.java | 2 +- .../handler/SMSConversationHandler.java | 34 ++++++------- .../handler/SMSModemBridgeHandler.java | 50 +++++++------------ .../main/resources/OH-INF/binding/binding.xml | 2 +- .../main/resources/OH-INF/thing/smsmodem.xml | 3 +- 6 files changed, 35 insertions(+), 58 deletions(-) diff --git a/bundles/org.openhab.binding.smsmodem/src/main/java/org/openhab/binding/smsmodem/internal/SMSModemBridgeConfiguration.java b/bundles/org.openhab.binding.smsmodem/src/main/java/org/openhab/binding/smsmodem/internal/SMSModemBridgeConfiguration.java index 7305e5d56841f..799d2f2d7e65d 100644 --- a/bundles/org.openhab.binding.smsmodem/src/main/java/org/openhab/binding/smsmodem/internal/SMSModemBridgeConfiguration.java +++ b/bundles/org.openhab.binding.smsmodem/src/main/java/org/openhab/binding/smsmodem/internal/SMSModemBridgeConfiguration.java @@ -23,7 +23,7 @@ public class SMSModemBridgeConfiguration { public String serialPort = ""; - public Integer baud = 9800; + public Integer baud = 9600; public String simPin = ""; public Integer pollingInterval = 15; public Integer delayBetweenSend = 0; diff --git a/bundles/org.openhab.binding.smsmodem/src/main/java/org/openhab/binding/smsmodem/internal/actions/SMSModemActions.java b/bundles/org.openhab.binding.smsmodem/src/main/java/org/openhab/binding/smsmodem/internal/actions/SMSModemActions.java index 9f83d4d74df5f..c0879534e6f91 100644 --- a/bundles/org.openhab.binding.smsmodem/src/main/java/org/openhab/binding/smsmodem/internal/actions/SMSModemActions.java +++ b/bundles/org.openhab.binding.smsmodem/src/main/java/org/openhab/binding/smsmodem/internal/actions/SMSModemActions.java @@ -55,7 +55,7 @@ public void sendSMS( if (recipient != null && !recipient.isEmpty() && message != null) { handler.send(recipient, message, false, encoding); } else { - logger.error("SMSModem cannot send a message with no recipient or text"); + logger.warn("SMSModem cannot send a message with no recipient or text"); } } diff --git a/bundles/org.openhab.binding.smsmodem/src/main/java/org/openhab/binding/smsmodem/internal/handler/SMSConversationHandler.java b/bundles/org.openhab.binding.smsmodem/src/main/java/org/openhab/binding/smsmodem/internal/handler/SMSConversationHandler.java index cd8b5adbfed1d..decde941000f2 100644 --- a/bundles/org.openhab.binding.smsmodem/src/main/java/org/openhab/binding/smsmodem/internal/handler/SMSConversationHandler.java +++ b/bundles/org.openhab.binding.smsmodem/src/main/java/org/openhab/binding/smsmodem/internal/handler/SMSConversationHandler.java @@ -16,6 +16,7 @@ import org.eclipse.jdt.annotation.Nullable; import org.openhab.binding.smsmodem.internal.SMSConversationConfiguration; import org.openhab.binding.smsmodem.internal.SMSModemBindingConstants; +import org.openhab.core.i18n.ConfigurationException; import org.openhab.core.library.types.StringType; import org.openhab.core.thing.Bridge; import org.openhab.core.thing.ChannelUID; @@ -56,24 +57,21 @@ public String getRecipient() { return config.recipient.trim(); } - private synchronized @Nullable SMSModemBridgeHandler getBridgeHandler() { + private synchronized void checkBridgeHandler() { if (this.bridgeHandler == null) { Bridge bridge = getBridge(); if (bridge == null) { - logger.error("Required bridge not defined for SMSconversation {} with {}.", thing.getUID(), - getRecipient()); - return null; + throw new ConfigurationException("Required bridge not defined for SMSconversation {} with {}.", + thing.getUID(), getRecipient()); } ThingHandler handler = bridge.getHandler(); if (handler instanceof SMSModemBridgeHandler) { this.bridgeHandler = (SMSModemBridgeHandler) handler; } else { - logger.error("No available bridge handler found for SMSConversation {} bridge {} .", thing.getUID(), - bridge.getUID()); - return null; + throw new ConfigurationException("No available bridge handler found for SMSConversation {} bridge {} .", + thing.getUID(), bridge.getUID()); } } - return this.bridgeHandler; } protected void checkAndReceive(String sender, String text) { @@ -115,29 +113,25 @@ public void send(String text) { @Override public void initialize() { config = getConfigAs(SMSConversationConfiguration.class); - bridgeHandler = getBridgeHandler(); - setStatusByBridgeStatus(); + try { + checkBridgeHandler(); + setStatusByBridgeStatus(); + } catch (ConfigurationException confe) { + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.CONFIGURATION_ERROR, confe.getMessage()); + } } private void setStatusByBridgeStatus() { SMSModemBridgeHandler bridgeHandlerFinal = bridgeHandler; if (bridgeHandlerFinal != null) { switch (bridgeHandlerFinal.getThing().getStatus()) { - case INITIALIZING: - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE); - break; - case OFFLINE: - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE); - break; case ONLINE: updateStatus(ThingStatus.ONLINE, ThingStatusDetail.NONE); break; + case INITIALIZING: + case OFFLINE: case REMOVED: - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE); - break; case REMOVING: - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE); - break; case UNINITIALIZED: updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.BRIDGE_OFFLINE); break; diff --git a/bundles/org.openhab.binding.smsmodem/src/main/java/org/openhab/binding/smsmodem/internal/handler/SMSModemBridgeHandler.java b/bundles/org.openhab.binding.smsmodem/src/main/java/org/openhab/binding/smsmodem/internal/handler/SMSModemBridgeHandler.java index b00d24c1cc9da..b8b5f3a188f08 100644 --- a/bundles/org.openhab.binding.smsmodem/src/main/java/org/openhab/binding/smsmodem/internal/handler/SMSModemBridgeHandler.java +++ b/bundles/org.openhab.binding.smsmodem/src/main/java/org/openhab/binding/smsmodem/internal/handler/SMSModemBridgeHandler.java @@ -21,9 +21,11 @@ import java.nio.file.Paths; import java.util.Collection; import java.util.HashSet; +import java.util.Objects; import java.util.Set; import java.util.concurrent.ScheduledFuture; import java.util.concurrent.TimeUnit; +import java.util.stream.Collectors; import org.eclipse.jdt.annotation.NonNullByDefault; import org.eclipse.jdt.annotation.Nullable; @@ -42,7 +44,6 @@ import org.openhab.core.thing.ThingStatusDetail; import org.openhab.core.thing.ThingTypeUID; import org.openhab.core.thing.binding.BaseBridgeHandler; -import org.openhab.core.thing.binding.ThingHandler; import org.openhab.core.thing.binding.ThingHandlerService; import org.openhab.core.types.Command; import org.slf4j.Logger; @@ -77,8 +78,6 @@ public class SMSModemBridgeHandler extends BaseBridgeHandler private final Logger logger = LoggerFactory.getLogger(SMSModemBridgeHandler.class); - private Set childHandlers = new HashSet<>(); - private SerialPortManager serialPortManager; /** @@ -167,7 +166,6 @@ private void checkAndStartModemIfNeeded() { } } catch (ModemConfigurationException e) { String message = e.getMessage(); - logger.error(message, e); updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, message); } } @@ -190,7 +188,6 @@ private void checkRemoteParam(SMSModemRemoteBridgeConfiguration config) throws M // test reachable address : try (Socket s = new Socket(ip, config.networkPort)) { } - } catch (IOException | NumberFormatException ex) { // no ip throw new ModemConfigurationException( @@ -216,20 +213,6 @@ public boolean isRunning() { return modem != null && (modem.getStatus() == Status.Started || modem.getStatus() == Status.Starting); } - @Override - public void childHandlerInitialized(ThingHandler childHandler, Thing childThing) { - if (childHandler instanceof SMSConversationHandler) { - childHandlers.add((SMSConversationHandler) childHandler); - } else { - logger.error("The SMSModemBridgeHandler can only handle SMSConversation as child"); - } - } - - @Override - public void childHandlerDisposed(ThingHandler childHandler, Thing childThing) { - childHandlers.remove(childHandler); - } - @Override public void handleCommand(ChannelUID channelUID, Command command) { } @@ -244,7 +227,7 @@ public void messageReceived(InboundMessage message) { if (text != null) { messageText = text; } else { - logger.error("Message has no payload !"); + logger.warn("Message has no payload !"); return; } } else { @@ -253,14 +236,14 @@ public void messageReceived(InboundMessage message) { logger.warn("Message payload in binary format. Don't know how to handle it. Please report it."); messageText = bytes.toString(); } else { - logger.error("Message has no payload !"); + logger.warn("Message has no payload !"); return; } } logger.debug("Receiving new message from {} : {}", sender, messageText); // dispatch to conversation : - for (SMSConversationHandler child : childHandlers) { + for (SMSConversationHandler child : getChildHandlers()) { child.checkAndReceive(sender, messageText); } @@ -277,7 +260,7 @@ public void messageReceived(InboundMessage message) { try { // delete message on the sim modem.delete(message); } catch (CommunicationException e) { - logger.error("Cannot delete message after receiving it !", e); + logger.warn("Cannot delete message after receiving it !", e); } } @@ -296,7 +279,7 @@ public void send(String recipient, String text, boolean deliveryReport, @Nullabl out.setEncoding(encoding2); } } catch (IllegalArgumentException e) { - logger.error("Encoding {} is not supported. Use Enc7, Enc8, EncUcs2, or EncCustom", encoding); + logger.warn("Encoding {} is not supported. Use Enc7, Enc8, EncUcs2, or EncCustom", encoding); } out.setRequestDeliveryReport(deliveryReport); logger.debug("Sending message to {}", recipient); @@ -321,25 +304,21 @@ public Collection> getServices() { public boolean processStatusCallback(Modem.Status oldStatus, Modem.Status newStatus) { switch (newStatus) { case Error: - logger.error("SMSLib reported an error on the underlying modem {}", modem.getDescription()); - updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR); + updateStatus(ThingStatus.OFFLINE, ThingStatusDetail.COMMUNICATION_ERROR, + "SMSLib reported an error on the underlying modem " + modem.getDescription()); break; case Started: - logger.debug("SMSLib reported the modem {} is started", modem.getDescription()); updateStatus(ThingStatus.ONLINE); break; case Starting: - logger.debug("SMSLib reported the modem {} is starting", modem.getDescription()); updateStatus(ThingStatus.UNKNOWN); break; case Stopped: - logger.debug("SMSLib reported the modem {} is stopped", modem.getDescription()); if (thing.getStatus() != ThingStatus.OFFLINE) { updateStatus(ThingStatus.OFFLINE); } break; case Stopping: - logger.debug("SMSLib reported the modem {} is stopping", modem.getDescription()); if (thing.getStatus() != ThingStatus.OFFLINE) { updateStatus(ThingStatus.OFFLINE); } @@ -374,7 +353,7 @@ public void messageSent(OutboundMessage message) { MsIsdn recipientAddress = message.getRecipientAddress(); if (recipientAddress != null) { String recipient = recipientAddress.getAddress(); - for (SMSConversationHandler child : childHandlers) { + for (SMSConversationHandler child : getChildHandlers()) { child.checkAndUpdateDeliveryStatus(recipient, sentStatus); } } @@ -405,17 +384,22 @@ public void messageDelivered(DeliveryReportMessage message) { MsIsdn recipientAddress = message.getRecipientAddress(); if (recipientAddress != null) { String recipient = recipientAddress.getAddress(); - for (SMSConversationHandler child : childHandlers) { + for (SMSConversationHandler child : getChildHandlers()) { child.checkAndUpdateDeliveryStatus(recipient, sentStatus); } } try { modem.delete(message); } catch (CommunicationException e) { - logger.error("Cannot delete delivery report after receiving it !", e); + logger.warn("Cannot delete delivery report after receiving it !", e); } } + private Set getChildHandlers() { + return getThing().getThings().stream().map(Thing::getHandler).filter(Objects::nonNull) + .map(handler -> (SMSConversationHandler) handler).collect(Collectors.toSet()); + } + @Override public void setManufacturer(String manufacturer) { thing.setProperty(SMSModemBindingConstants.PROPERTY_MANUFACTURER, manufacturer); diff --git a/bundles/org.openhab.binding.smsmodem/src/main/resources/OH-INF/binding/binding.xml b/bundles/org.openhab.binding.smsmodem/src/main/resources/OH-INF/binding/binding.xml index 7a94f67a700d3..619e6fd783636 100644 --- a/bundles/org.openhab.binding.smsmodem/src/main/resources/OH-INF/binding/binding.xml +++ b/bundles/org.openhab.binding.smsmodem/src/main/resources/OH-INF/binding/binding.xml @@ -4,7 +4,7 @@ xsi:schemaLocation="https://openhab.org/schemas/binding/v1.0.0 https://openhab.org/schemas/binding-1.0.0.xsd"> SMSModem Binding - This binding handle a GSM modem connected to the openHAB server (Serial), or exposed on the network. It + This binding handles a GSM modem connected to the openHAB server (Serial), or exposed on the network. It can send and receive SMS. diff --git a/bundles/org.openhab.binding.smsmodem/src/main/resources/OH-INF/thing/smsmodem.xml b/bundles/org.openhab.binding.smsmodem/src/main/resources/OH-INF/thing/smsmodem.xml index 5835bcd3602fe..da35c44eb719e 100644 --- a/bundles/org.openhab.binding.smsmodem/src/main/resources/OH-INF/thing/smsmodem.xml +++ b/bundles/org.openhab.binding.smsmodem/src/main/resources/OH-INF/thing/smsmodem.xml @@ -84,8 +84,7 @@ trigger Triggered when a message is received, in the form "<msisdn_sender>|<text>" - - +