chrisdutz commented on a change in pull request #172:
URL: https://github.com/apache/plc4x/pull/172#discussion_r456296593
##########
File path:
plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/field/ModbusFieldDiscreteInput.java
##########
@@ -26,21 +26,40 @@ Licensed to the Apache Software Foundation (ASF) under one
public class ModbusFieldDiscreteInput extends ModbusField {
public static final Pattern ADDRESS_PATTERN =
Pattern.compile("discrete-input:" + ModbusField.ADDRESS_PATTERN);
+ public static final Pattern ADDRESS_SHORTER_PATTERN = Pattern.compile("1"
+ ModbusField.ADDRESS_PATTERN);
+ public static final Pattern ADDRESS_SHORT_PATTERN = Pattern.compile("1x" +
ModbusField.ADDRESS_PATTERN);
public ModbusFieldDiscreteInput(int address, Integer quantity) {
super(address, quantity);
}
- public static ModbusFieldDiscreteInput of(String addressString) throws
PlcInvalidFieldException {
- Matcher matcher = ADDRESS_PATTERN.matcher(addressString);
- if (!matcher.matches()) {
- throw new PlcInvalidFieldException(addressString, ADDRESS_PATTERN);
+ public static boolean matches(String addressString) {
+ return ADDRESS_PATTERN.matcher(addressString).matches() ||
+ ADDRESS_SHORTER_PATTERN.matcher(addressString).matches() ||
+ ADDRESS_SHORT_PATTERN.matcher(addressString).matches();
+ }
+
+ public static Matcher getMatcher(String addressString) throws
PlcInvalidFieldException {
+ Matcher matcher;
+ if (ADDRESS_PATTERN.matcher(addressString).matches()) {
+ matcher = ADDRESS_PATTERN.matcher(addressString);
+ } else if (ADDRESS_SHORT_PATTERN.matcher(addressString).matches()) {
+ matcher = ADDRESS_SHORT_PATTERN.matcher(addressString);
+ } else if (ADDRESS_SHORTER_PATTERN.matcher(addressString).matches()) {
+ matcher = ADDRESS_SHORTER_PATTERN.matcher(addressString);
+ } else {
+ throw new PlcInvalidFieldException(addressString, ADDRESS_PATTERN);
}
- int address = Integer.parseInt(matcher.group("address"));
+ return matcher;
+ }
+
+ public static ModbusFieldDiscreteInput of(String addressString) throws
PlcInvalidFieldException {
+ Matcher matcher = getMatcher(addressString);
+ matcher.find();
Review comment:
You are ignoring the result of the find ... what's it meant for?
##########
File path:
plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/field/ModbusField.java
##########
@@ -28,33 +28,33 @@ Licensed to the Apache Software Foundation (ASF) under one
public abstract class ModbusField implements PlcField {
public static final Pattern ADDRESS_PATTERN =
Pattern.compile("(?<address>\\d+)(\\[(?<quantity>\\d+)])?");
+ protected static final int protocolAddressOffset = 1;
private final int address;
private final int quantity;
public static ModbusField of(String addressString) throws
PlcInvalidFieldException {
- Matcher matcher =
ModbusFieldCoil.ADDRESS_PATTERN.matcher(addressString);
- if(matcher.matches()) {
+ if(ModbusFieldCoil.matches(addressString)) {
return ModbusFieldCoil.of(addressString);
}
- matcher =
ModbusFieldDiscreteInput.ADDRESS_PATTERN.matcher(addressString);
- if(matcher.matches()) {
+ if(ModbusFieldDiscreteInput.matches(addressString)) {
return ModbusFieldDiscreteInput.of(addressString);
}
- matcher =
ModbusFieldHoldingRegister.ADDRESS_PATTERN.matcher(addressString);
- if(matcher.matches()) {
+ if(ModbusFieldHoldingRegister.matches(addressString)) {
return ModbusFieldHoldingRegister.of(addressString);
}
- matcher =
ModbusFieldInputRegister.ADDRESS_PATTERN.matcher(addressString);
- if(matcher.matches()) {
+ if(ModbusFieldInputRegister.matches(addressString)) {
return ModbusFieldInputRegister.of(addressString);
}
throw new PlcInvalidFieldException("Unable to parse address: " +
addressString);
}
protected ModbusField(int address, Integer quantity) {
this.address = address;
+ if ((this.address + protocolAddressOffset) <= 0) {
+ throw new IllegalArgumentException("address must be greater then
zero. Was " + (this.address + protocolAddressOffset));
Review comment:
Same as above
##########
File path:
plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/field/ModbusField.java
##########
@@ -28,33 +28,33 @@ Licensed to the Apache Software Foundation (ASF) under one
public abstract class ModbusField implements PlcField {
public static final Pattern ADDRESS_PATTERN =
Pattern.compile("(?<address>\\d+)(\\[(?<quantity>\\d+)])?");
+ protected static final int protocolAddressOffset = 1;
private final int address;
private final int quantity;
public static ModbusField of(String addressString) throws
PlcInvalidFieldException {
- Matcher matcher =
ModbusFieldCoil.ADDRESS_PATTERN.matcher(addressString);
- if(matcher.matches()) {
+ if(ModbusFieldCoil.matches(addressString)) {
return ModbusFieldCoil.of(addressString);
}
- matcher =
ModbusFieldDiscreteInput.ADDRESS_PATTERN.matcher(addressString);
- if(matcher.matches()) {
+ if(ModbusFieldDiscreteInput.matches(addressString)) {
return ModbusFieldDiscreteInput.of(addressString);
}
- matcher =
ModbusFieldHoldingRegister.ADDRESS_PATTERN.matcher(addressString);
- if(matcher.matches()) {
+ if(ModbusFieldHoldingRegister.matches(addressString)) {
return ModbusFieldHoldingRegister.of(addressString);
}
- matcher =
ModbusFieldInputRegister.ADDRESS_PATTERN.matcher(addressString);
- if(matcher.matches()) {
+ if(ModbusFieldInputRegister.matches(addressString)) {
return ModbusFieldInputRegister.of(addressString);
}
throw new PlcInvalidFieldException("Unable to parse address: " +
addressString);
}
protected ModbusField(int address, Integer quantity) {
this.address = address;
+ if ((this.address + protocolAddressOffset) <= 0) {
Review comment:
Are you sure this shouldn't be "this.address - protocolAddressOffset"
... as far as I remember something "400001" would refer to address
holding-register address 0 ...
##########
File path:
plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/field/ModbusFieldCoil.java
##########
@@ -26,17 +26,37 @@ Licensed to the Apache Software Foundation (ASF) under one
public class ModbusFieldCoil extends ModbusField {
public static final Pattern ADDRESS_PATTERN = Pattern.compile("coil:" +
ModbusField.ADDRESS_PATTERN);
+ public static final Pattern ADDRESS_SHORTER_PATTERN = Pattern.compile("0"
+ ModbusField.ADDRESS_PATTERN);
Review comment:
I would probably use a second ADDRESS_PATTERN here as this would also
mach "01" ... I think it has to be 5 digits, correct? So perhaps using a
"FIVE_DIGIT_MODBUS_PATTERN" instead would be a good idea?
##########
File path:
plc4j/drivers/modbus/src/main/java/org/apache/plc4x/java/modbus/field/ModbusFieldDiscreteInput.java
##########
@@ -26,21 +26,40 @@ Licensed to the Apache Software Foundation (ASF) under one
public class ModbusFieldDiscreteInput extends ModbusField {
public static final Pattern ADDRESS_PATTERN =
Pattern.compile("discrete-input:" + ModbusField.ADDRESS_PATTERN);
+ public static final Pattern ADDRESS_SHORTER_PATTERN = Pattern.compile("1"
+ ModbusField.ADDRESS_PATTERN);
+ public static final Pattern ADDRESS_SHORT_PATTERN = Pattern.compile("1x" +
ModbusField.ADDRESS_PATTERN);
public ModbusFieldDiscreteInput(int address, Integer quantity) {
super(address, quantity);
}
- public static ModbusFieldDiscreteInput of(String addressString) throws
PlcInvalidFieldException {
- Matcher matcher = ADDRESS_PATTERN.matcher(addressString);
- if (!matcher.matches()) {
- throw new PlcInvalidFieldException(addressString, ADDRESS_PATTERN);
+ public static boolean matches(String addressString) {
+ return ADDRESS_PATTERN.matcher(addressString).matches() ||
+ ADDRESS_SHORTER_PATTERN.matcher(addressString).matches() ||
+ ADDRESS_SHORT_PATTERN.matcher(addressString).matches();
+ }
+
+ public static Matcher getMatcher(String addressString) throws
PlcInvalidFieldException {
+ Matcher matcher;
+ if (ADDRESS_PATTERN.matcher(addressString).matches()) {
+ matcher = ADDRESS_PATTERN.matcher(addressString);
+ } else if (ADDRESS_SHORT_PATTERN.matcher(addressString).matches()) {
+ matcher = ADDRESS_SHORT_PATTERN.matcher(addressString);
+ } else if (ADDRESS_SHORTER_PATTERN.matcher(addressString).matches()) {
+ matcher = ADDRESS_SHORTER_PATTERN.matcher(addressString);
+ } else {
+ throw new PlcInvalidFieldException(addressString, ADDRESS_PATTERN);
}
- int address = Integer.parseInt(matcher.group("address"));
+ return matcher;
+ }
+
+ public static ModbusFieldDiscreteInput of(String addressString) throws
PlcInvalidFieldException {
+ Matcher matcher = getMatcher(addressString);
+ matcher.find();
Review comment:
perhaps a "matches(...)" check at the beginning would be good ... I mean
you do the check in the FieldHandler, but one could call the of method from
somewhere else ... so instead of "matcher.find()" I would probably do a
"if(!matcher.matches())" and throw an InvalidFieldException if it doesn't match.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
