You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@plc4x.apache.org by GitBox <gi...@apache.org> on 2020/07/17 02:49:10 UTC
[GitHub] [plc4x] hutcheb opened a new pull request #172: Feature/modbus add additional address formats and change lowest register to 1.
hutcheb opened a new pull request #172:
URL: https://github.com/apache/plc4x/pull/172
- Address formats similar to 400001, 000001 were added
- Address formats similar to 4x00001, 0x000001 were added.
- Changed the lowest address the user can reference to 1 instead of 0.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a change in pull request #172: Feature/modbus add additional address formats and change lowest register to 1.
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on a change in pull request #172:
URL: https://github.com/apache/plc4x/pull/172#discussion_r456315294
##########
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:
Aaaahh yeah ... now I got you ... and yes ... indeed "address" would be the decremented value ... so yeah ... just ignore my comment ;-)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a change in pull request #172: Feature/modbus add additional address formats and change lowest register to 1.
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on a change in pull request #172:
URL: https://github.com/apache/plc4x/pull/172#discussion_r456321362
##########
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:
Yeah .. in that case I would prefer "5 or more" digits (You can probably just do 5-6 digits)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [plc4x] hutcheb commented on a change in pull request #172: Feature/modbus add additional address formats and change lowest register to 1.
Posted by GitBox <gi...@apache.org>.
hutcheb commented on a change in pull request #172:
URL: https://github.com/apache/plc4x/pull/172#discussion_r456333241
##########
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:
Yeah that makes more sense, I'll change that.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [plc4x] hutcheb commented on a change in pull request #172: Feature/modbus add additional address formats and change lowest register to 1.
Posted by GitBox <gi...@apache.org>.
hutcheb commented on a change in pull request #172:
URL: https://github.com/apache/plc4x/pull/172#discussion_r456313688
##########
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:
this.address stores the address sent over the wire starting at 0 instead of address 1. I didn't want to have a check that checks if the value is < 0 and have the exception say you can't have an address < 1 as it might be confusing.
In the constructor for each memory area I subtract 1 from the address before storing it in this.address.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [plc4x] hutcheb commented on a change in pull request #172: Feature/modbus add additional address formats and change lowest register to 1.
Posted by GitBox <gi...@apache.org>.
hutcheb commented on a change in pull request #172:
URL: https://github.com/apache/plc4x/pull/172#discussion_r456321048
##########
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:
Calling the getMatcher mehod checks for the case that the pattern isn't found by raising the exception. I moved the exception to that method so I didn't have to worry about returning a null value.
I found that the find() call then needs to be added as I wasn't calling the matches() method to find the first match. We can ignore the result of this as the getMatcher method has already confirmed that at least one instance will be found.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz merged pull request #172: Feature/modbus add additional address formats and change lowest register to 1.
Posted by GitBox <gi...@apache.org>.
chrisdutz merged pull request #172:
URL: https://github.com/apache/plc4x/pull/172
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a change in pull request #172: Feature/modbus add additional address formats and change lowest register to 1.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
[GitHub] [plc4x] chrisdutz commented on a change in pull request #172: Feature/modbus add additional address formats and change lowest register to 1.
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on a change in pull request #172:
URL: https://github.com/apache/plc4x/pull/172#discussion_r456329012
##########
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:
Ups seems that response wen't to the wrong comment ... sorry ... well I just noticed that IntelliJ was complaining ... And yeah ... if it wouldn't match, it wouldn't pass the getMatcher() call ... so how about instead of creating one matcher to check if it matches and then to return a new one ... how about returning the instance you used to check?
##########
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:
Yeah .. in that case I would prefer "5 or more" digits (You can probably just do 5-6 digits)
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [plc4x] hutcheb commented on a change in pull request #172: Feature/modbus add additional address formats and change lowest register to 1.
Posted by GitBox <gi...@apache.org>.
hutcheb commented on a change in pull request #172:
URL: https://github.com/apache/plc4x/pull/172#discussion_r456318413
##########
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 wasn't too sure about this because depending on the device they will list the Modbus address as 40001 or 400001. The extra 0 seems to have been added when the larger memory areas started to be used.
In Proworx NXT when referencing Modbus addresses you can just type 41 and it will always take the first digit as the memory area, the rest will be the address. I wasn't sure about extending the format to this extent.
It doesn't make sense to have more than 5 digits for the address though or values greater than 65536. I can fix this up.
----------------------------------------------------------------
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:
users@infra.apache.org