Merge pull request #1 from ssalonen/optional-bits-and-registers

[modbus] read modbus data as Optionals & PollResult toString

bundles/org.openhab.binding.modbus.sunspec/src/main/java/org/openhab/binding/modbus/sunspec/internal/discovery/SunspecDiscoveryProcess.java

@@ -20,7 +20,6 @@
 import java.util.Queue;
 import java.util.concurrent.ConcurrentLinkedQueue;
 
-import org.eclipse.jdt.annotation.NonNull;
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
 import org.eclipse.smarthome.config.discovery.DiscoveryResult;
@@ -160,10 +159,8 @@ public void detectModel() {
  SUNSPEC_ID_SIZE, // number or words to return
  maxTries);
 
- comms.submitOneTimePoll(request, result -> {
- ModbusRegisterArray registers = (@NonNull ModbusRegisterArray) result.getRegisters();
- headerReceived(registers);
- }, this::handleError);
+ comms.submitOneTimePoll(request, result -> result.getRegisters().ifPresent(this::headerReceived),
+ this::handleError);
  }
 
  /**
@@ -197,10 +194,8 @@ private void lookForModelBlock() {
  MODEL_HEADER_SIZE, // number or words to return
  maxTries);
 
- comms.submitOneTimePoll(request, result -> {
- ModbusRegisterArray registers = (@NonNull ModbusRegisterArray) result.getRegisters();
- modelBlockReceived(registers);
- }, this::handleError);
+ comms.submitOneTimePoll(request, result -> result.getRegisters().ifPresent(this::modelBlockReceived),
+ this::handleError);
  }
 
  /**
@@ -252,10 +247,8 @@ private void readCommonBlock(ModelBlock block) {
  block.length, // number or words to return
  maxTries);
 
- comms.submitOneTimePoll(request, result -> {
- ModbusRegisterArray registers = (@NonNull ModbusRegisterArray) result.getRegisters();
- parseCommonBlock(registers);
- }, this::handleError);
+ comms.submitOneTimePoll(request, result -> result.getRegisters().ifPresent(this::parseCommonBlock),
+ this::handleError);
  }
 
  /**

bundles/org.openhab.binding.modbus.sunspec/src/main/java/org/openhab/binding/modbus/sunspec/internal/handler/AbstractSunSpecHandler.java

@@ -20,7 +20,6 @@
 
 import javax.measure.Unit;
 
-import org.eclipse.jdt.annotation.NonNull;
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
 import org.eclipse.smarthome.core.library.types.QuantityType;
@@ -335,8 +334,7 @@ private synchronized void registerPollTask(ModelBlock mainBlock) {
 
  long refreshMillis = myconfig.getRefreshMillis();
  pollTask = mycomms.registerRegularPoll(request, refreshMillis, 1000, result -> {
- ModbusRegisterArray registers = (@NonNull ModbusRegisterArray) result.getRegisters();
- handlePolledData(registers);
+ result.getRegisters().ifPresent(this::handlePolledData);
  if (getThing().getStatus() != ThingStatus.ONLINE) {
  updateStatus(ThingStatus.ONLINE);
  }

bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/discovery/internal/ModbusDiscoveryService.java

@@ -15,7 +15,6 @@
 import java.util.Set;
 import java.util.concurrent.CopyOnWriteArraySet;
 
-import org.eclipse.jdt.annotation.NonNull;
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.smarthome.config.discovery.AbstractDiscoveryService;
 import org.eclipse.smarthome.config.discovery.DiscoveryResult;
@@ -116,7 +115,7 @@ protected void scanFinished() {
  * instances. They call back this method when a thing has been discovered
  */
  @Override
- protected void thingDiscovered(@NonNull DiscoveryResult discoveryResult) {
+ protected void thingDiscovered(DiscoveryResult discoveryResult) {
  super.thingDiscovered(discoveryResult);
  }
 

bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/handler/ModbusPollerThingHandler.java

@@ -195,6 +195,12 @@ private class PollResult {
  this.result = null;
  this.failure = failure;
  }
+
+ @Override
+ public String toString() {
+ return result == null ? String.format("PollResult(result=%s)", result)
+ : String.format("PollResult(failure=%s)", failure);
+ }
  }
 
  private final Logger logger = LoggerFactory.getLogger(ModbusPollerThingHandler.class);

bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/handler/AbstractModbusEndpointThingHandler.java

@@ -102,7 +102,7 @@ public void dispose() {
  localComms.close();
  }
  } catch (Exception e) {
- logger.error("Error closing modbus communication interface", e);
+ logger.warn("Error closing modbus communication interface", e);
  } finally {
  comms = null;
  }

bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/handler/ModbusDataThingHandler.java

@@ -21,13 +21,11 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.Objects;
 import java.util.Optional;
 import java.util.concurrent.TimeUnit;
 
 import org.apache.commons.lang.NotImplementedException;
 import org.apache.commons.lang.StringUtils;
-import org.eclipse.jdt.annotation.NonNull;
 import org.eclipse.jdt.annotation.NonNullByDefault;
 import org.eclipse.jdt.annotation.Nullable;
 import org.eclipse.smarthome.core.library.items.ContactItem;
@@ -336,8 +334,8 @@ public synchronized void initialize() {
  // Long running initialization should be done asynchronously in background.
  try {
  logger.trace("initialize() of thing {} '{}' starting", thing.getUID(), thing.getLabel());
- config = getConfigAs(ModbusDataConfiguration.class);
- updateUnchangedValuesEveryMillis = config.getUpdateUnchangedValuesEveryMillis();
+ ModbusDataConfiguration localConfig = config = getConfigAs(ModbusDataConfiguration.class);
+ updateUnchangedValuesEveryMillis = localConfig.getUpdateUnchangedValuesEveryMillis();
  Bridge bridge = getBridge();
  if (bridge == null) {
  logger.debug("Thing {} '{}' has no bridge", getThing().getUID(), getThing().getLabel());
@@ -377,8 +375,8 @@ public synchronized void initialize() {
  pollStart = localReadRequest.getReference();
  childOfEndpoint = false;
  }
- validateAndParseReadParameters();
- validateAndParseWriteParameters();
+ validateAndParseReadParameters(localConfig);
+ validateAndParseWriteParameters(localConfig);
  validateMustReadOrWrite();
 
  updateStatusIfChanged(ThingStatus.ONLINE);
@@ -437,9 +435,7 @@ private void validateMustReadOrWrite() throws ModbusConfigurationException {
  }
  }
 
- private void validateAndParseReadParameters() throws ModbusConfigurationException {
- ModbusDataConfiguration config = this.config;
- Objects.requireNonNull(config);
+ private void validateAndParseReadParameters(ModbusDataConfiguration config) throws ModbusConfigurationException {
  ModbusReadFunctionCode functionCode = this.functionCode;
  boolean readingDiscreteOrCoil = functionCode == ModbusReadFunctionCode.READ_COILS
  || functionCode == ModbusReadFunctionCode.READ_INPUT_DISCRETES;
@@ -489,7 +485,12 @@ private void validateAndParseReadParameters() throws ModbusConfigurationExceptio
  }
 
  if (isReadEnabled) {
- String[] readParts = config.getReadStart().split("\\.", 2);
+ String readStart = config.getReadStart();
+ if (readStart == null) {
+ throw new ModbusConfigurationException(
+ String.format("Thing %s invalid readStart: %s", getThing().getUID(), config.getReadStart()));
+ }
+ String[] readParts = readStart.split("\\.", 2);
  try {
  readIndex = Optional.of(Integer.parseInt(readParts[0]));
  if (readParts.length == 2) {
@@ -507,7 +508,7 @@ private void validateAndParseReadParameters() throws ModbusConfigurationExceptio
  validateReadIndex();
  }
 
- private void validateAndParseWriteParameters() throws ModbusConfigurationException {
+ private void validateAndParseWriteParameters(ModbusDataConfiguration config) throws ModbusConfigurationException {
  boolean writeTypeMissing = StringUtils.isBlank(config.getWriteType());
  boolean writeStartMissing = StringUtils.isBlank(config.getWriteStart());
  boolean writeValueTypeMissing = StringUtils.isBlank(config.getWriteValueType());
@@ -537,26 +538,27 @@ private void validateAndParseWriteParameters() throws ModbusConfigurationExcepti
  WRITE_TYPE_HOLDING, WRITE_TYPE_COIL);
  throw new ModbusConfigurationException(errmsg);
  }
+ final ValueType localWriteValueType;
  if (writeParametersHavingTransformationOnly) {
  // Placeholder for further checks
- writeValueType = ModbusConstants.ValueType.INT16;
+ localWriteValueType = writeValueType = ModbusConstants.ValueType.INT16;
  } else if (writingCoil && writeValueTypeMissing) {
- writeValueType = ModbusConstants.ValueType.BIT;
+ localWriteValueType = writeValueType = ModbusConstants.ValueType.BIT;
  } else {
  try {
- writeValueType = ValueType.fromConfigValue(config.getWriteValueType());
+ localWriteValueType = writeValueType = ValueType.fromConfigValue(config.getWriteValueType());
  } catch (IllegalArgumentException e) {
  String errmsg = String.format("Invalid writeValueType=%s!", config.getWriteValueType());
  throw new ModbusConfigurationException(errmsg);
  }
  }
 
- if (writingCoil && !ModbusConstants.ValueType.BIT.equals(writeValueType)) {
+ if (writingCoil && !ModbusConstants.ValueType.BIT.equals(localWriteValueType)) {
  String errmsg = String.format(
  "Invalid writeValueType: Only writeValueType='%s' (or undefined) supported with coils. Value type was: %s",
  ModbusConstants.ValueType.BIT, config.getWriteValueType());
  throw new ModbusConfigurationException(errmsg);
- } else if (!writingCoil && writeValueType.getBits() < 16) {
+ } else if (!writingCoil && localWriteValueType.getBits() < 16) {
  // trying to write holding registers with < 16 bit value types. Not supported
  String errmsg = String.format(
  "Invalid writeValueType: Only writeValueType with larger or equal to 16 bits are supported holding registers. Value type was: %s",
@@ -566,7 +568,13 @@ private void validateAndParseWriteParameters() throws ModbusConfigurationExcepti
 
  try {
  if (!writeParametersHavingTransformationOnly) {
- writeStart = Integer.parseInt(config.getWriteStart().trim());
+ String localWriteStart = config.getWriteStart();
+ if (localWriteStart == null) {
+ String errmsg = String.format("Thing %s invalid writeStart: %s", getThing().getUID(),
+ config.getWriteStart());
+ throw new ModbusConfigurationException(errmsg);
+ }
+ writeStart = Integer.parseInt(localWriteStart.trim());
  }
  } catch (IllegalArgumentException e) {
  String errmsg = String.format("Thing %s invalid writeStart: %s", getThing().getUID(),
@@ -651,15 +659,8 @@ private boolean containsOpenClosed(List<Class<? extends State>> acceptedDataType
  }
 
  public synchronized void onReadResult(AsyncModbusReadResult result) {
- if (result.getRegisters() != null) {
- ModbusRegisterArray registers = result.getRegisters();
- assert registers != null;
- onRegisters(result.getRequest(), registers);
- } else {
- BitArray bits = result.getBits();
- assert bits != null;
- onBits(result.getRequest(), bits);
- }
+ result.getRegisters().ifPresent(registers -> onRegisters(result.getRequest(), registers));
+ result.getBits().ifPresent(bits -> onBits(result.getRequest(), bits));
  }
 
  public synchronized void handleReadError(AsyncModbusFailure<ModbusReadRequestBlueprint> failure) {
@@ -743,7 +744,7 @@ private synchronized void onError(ModbusReadRequestBlueprint request, Exception
  getThing().getUID(), getThing().getLabel(), error.getClass().getName(), error.toString(),
  error.getMessage(), error);
  }
- Map<@NonNull ChannelUID, @NonNull State> states = new HashMap<>();
+ Map<ChannelUID, State> states = new HashMap<>();
  ChannelUID lastReadErrorUID = getChannelUID(ModbusBindingConstantsInternal.CHANNEL_LAST_READ_ERROR);
  if (isLinked(lastReadErrorUID)) {
  states.put(lastReadErrorUID, new DateTimeType());
@@ -779,7 +780,7 @@ private synchronized void onError(ModbusWriteRequestBlueprint request, Exception
  getThing().getUID(), getThing().getLabel(), error.getClass().getName(), error.toString(),
  error.getMessage(), error);
  }
- Map<@NonNull ChannelUID, @NonNull State> states = new HashMap<>();
+ Map<ChannelUID, State> states = new HashMap<>();
  ChannelUID lastWriteErrorUID = getChannelUID(ModbusBindingConstantsInternal.CHANNEL_LAST_WRITE_ERROR);
  if (isLinked(lastWriteErrorUID)) {
  states.put(lastWriteErrorUID, new DateTimeType());
@@ -819,7 +820,13 @@ public synchronized void onWriteResponse(AsyncModbusWriteResult result) {
  * @return updated channel data
  */
  private Map<ChannelUID, State> processUpdatedValue(State numericState, boolean boolValue) {
- Map<@NonNull ChannelUID, @NonNull State> states = new HashMap<>();
+ Transformation localReadTransformation = readTransformation;
+ if (localReadTransformation == null) {
+ // We should always have transformation available if thing is initalized properly
+ logger.trace("No transformation available, aborting processUpdatedValue");
+ return new HashMap<ChannelUID, State>();
+ }
+ Map<ChannelUID, State> states = new HashMap<>();
  CHANNEL_ID_TO_ACCEPTED_TYPES.keySet().stream().forEach(channelId -> {
  ChannelUID channelUID = getChannelUID(channelId);
  if (!isLinked(channelUID)) {
@@ -840,35 +847,36 @@ private Map<ChannelUID, State> processUpdatedValue(State numericState, boolean b
  }
 
  State transformedState;
- if (readTransformation.isIdentityTransform()) {
+ if (localReadTransformation.isIdentityTransform()) {
  if (boolLikeState != null) {
  // A bit of smartness for ON/OFF and OPEN/CLOSED with boolean like items
  transformedState = boolLikeState;
  } else {
  // Numeric states always go through transformation. This allows value of 17.5 to be
  // converted to
  // 17.5% with percent types (instead of raising error)
- transformedState = readTransformation.transformState(bundleContext, acceptedDataTypes,
+ transformedState = localReadTransformation.transformState(bundleContext, acceptedDataTypes,
  numericState);
  }
  } else {
- transformedState = readTransformation.transformState(bundleContext, acceptedDataTypes, numericState);
+ transformedState = localReadTransformation.transformState(bundleContext, acceptedDataTypes,
+ numericState);
  }
 
  if (transformedState != null) {
  logger.trace(
  "Channel {} will be updated to '{}' (type {}). Input data: number value {} (value type '{}' taken into account) and bool value {}. Transformation: {}",
  channelId, transformedState, transformedState.getClass().getSimpleName(), numericState,
  readValueType, boolValue,
- readTransformation.isIdentityTransform() ? "<identity>" : readTransformation);
+ localReadTransformation.isIdentityTransform() ? "<identity>" : localReadTransformation);
  states.put(channelUID, transformedState);
  } else {
  String types = StringUtils.join(acceptedDataTypes.stream().map(cls -> cls.getSimpleName()).toArray(),
  ", ");
  logger.warn(
  "Channel {} will not be updated since transformation was unsuccessful. Channel is expecting the following data types [{}]. Input data: number value {} (value type '{}' taken into account) and bool value {}. Transformation: {}",
  channelId, types, numericState, readValueType, boolValue,
- readTransformation.isIdentityTransform() ? "<identity>" : readTransformation);
+ localReadTransformation.isIdentityTransform() ? "<identity>" : localReadTransformation);
  }
  });
 
@@ -890,6 +898,8 @@ private void updateExpiredChannels(Map<ChannelUID, State> states) {
  }
  }
 
+ // since lastState can be null, and "lastState == null" in conditional is not useless
+ @SuppressWarnings("null")
  private void updateExpiredChannel(long now, ChannelUID uid, State state) {
  @Nullable
  State lastState = channelLastState.get(uid);

bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/handler/ModbusSerialThingHandler.java

@@ -84,6 +84,7 @@ public boolean isDiscoveryEnabled() {
  }
  }
 
+ @SuppressWarnings("null") // Since endpoint in Optional.map cannot be null
  @Override
  protected String formatConflictingParameterError() {
  return String.format(

bundles/org.openhab.binding.modbus/src/main/java/org/openhab/binding/modbus/internal/handler/ModbusTcpThingHandler.java

@@ -61,6 +61,7 @@ protected void configure() throws ModbusConfigurationException {
  poolConfiguration.setReconnectAfterMillis(config.getReconnectAfterMillis());
  }
 
+ @SuppressWarnings("null") // since Optional.map is always called with NonNull argument
  @Override
  protected String formatConflictingParameterError() {
  return String.format(
@@ -72,10 +73,11 @@ protected String formatConflictingParameterError() {
 
  @Override
  public int getSlaveId() {
- if (config == null) {
+ ModbusTcpConfiguration localConfig = config;
+ if (localConfig == null) {
  throw new IllegalStateException("Poller not configured, but slave id is queried!");
  }
- return config.getId();
+ return localConfig.getId();
  }
 
  @Override