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/09/30 11:38:47 UTC
[GitHub] [plc4x] hutcheb opened a new pull request #192: Refactor Field Handler Classes
hutcheb opened a new pull request #192:
URL: https://github.com/apache/plc4x/pull/192
First attempt at refactoring the field handler class, currently troubleshooting issue with new PlcValue of method.
----------------------------------------------------------------
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 #192: Refactor Field Handler Classes
Posted by GitBox <gi...@apache.org>.
hutcheb commented on a change in pull request #192:
URL: https://github.com/apache/plc4x/pull/192#discussion_r500123620
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/value/PlcBYTE.java
##########
@@ -169,6 +169,30 @@ public PlcBYTE(@JsonProperty("value") short value) {
}
}
+ @Override
+ @JsonIgnore
+ public boolean isBoolean() {
+ return true;
+ }
+
+ @Override
+ @JsonIgnore
+ public boolean getBoolean() {
+ return (value != null) && !value.equals(0);
+ }
+
+ @Override
+ @JsonIgnore
+ public boolean isByte() {
+ return (value != null) && (value <= Byte.MAX_VALUE) && (value >= Byte.MIN_VALUE);
+ }
+
+ @Override
+ @JsonIgnore
+ public byte getByte() {
+ return value.byteValue();
+ }
+
Review comment:
There's no reason to have the getBYTE,getINT,getREAL,etc... they don't get used anymore. I added these in the previous PR so we can customize the method that gets called in the data-io template by using the case name.
----------------------------------------------------------------
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 #192: Refactor Field Handler Classes
Posted by GitBox <gi...@apache.org>.
hutcheb commented on a change in pull request #192:
URL: https://github.com/apache/plc4x/pull/192#discussion_r500139132
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/value/PlcBYTE.java
##########
@@ -169,6 +169,30 @@ public PlcBYTE(@JsonProperty("value") short value) {
}
}
+ @Override
+ @JsonIgnore
+ public boolean isBoolean() {
+ return true;
+ }
+
+ @Override
+ @JsonIgnore
+ public boolean getBoolean() {
+ return (value != null) && !value.equals(0);
+ }
Review comment:
I was having trouble with how we would do this. I was initially thinking that if we request to read a WORD we should parse the return value as a PlcList of PlcBOOLs instead of a PlcWORD but wasn't sure how to implement this easily.
But your idea of returning a list from a getBoolean method seems reasonable.
----------------------------------------------------------------
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 #192: Refactor Field Handler Classes
Posted by GitBox <gi...@apache.org>.
chrisdutz merged pull request #192:
URL: https://github.com/apache/plc4x/pull/192
----------------------------------------------------------------
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 pull request #192: Refactor Field Handler Classes
Posted by GitBox <gi...@apache.org>.
hutcheb commented on pull request #192:
URL: https://github.com/apache/plc4x/pull/192#issuecomment-703093701
StaticSerialzer when writing booleans needs some work.
----------------------------------------------------------------
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 #192: Refactor Field Handler Classes
Posted by GitBox <gi...@apache.org>.
hutcheb commented on a change in pull request #192:
URL: https://github.com/apache/plc4x/pull/192#discussion_r500144795
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/value/PlcBYTE.java
##########
@@ -181,6 +205,90 @@ public short getShort() {
return value;
}
+ @Override
+ @JsonIgnore
+ public boolean isInteger() {
+ return true;
+ }
+
+ @Override
+ @JsonIgnore
+ public int getInteger() {
+ return value.intValue();
+ }
+
Review comment:
I think there a at least two use cases for these methods. To use them when parsing the value within the dataio classes. The other is by the user when they need to parse the value to a specific type.
A generic function to be used when parsing like getValue() would be good.
For the user the getInteger, getByte, etc.. might be good as I can't think of an easy way to pass a type to the function and have it return the data type requested without just putting the existing functions within a switch statement.
----------------------------------------------------------------
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 pull request #192: Refactor Field Handler Classes
Posted by GitBox <gi...@apache.org>.
hutcheb commented on pull request #192:
URL: https://github.com/apache/plc4x/pull/192#issuecomment-703242394
Bit of a mismatch of a bunch of issues:-
- Implemented WriteBuffer - Big Integer and floating point functions.
- Implemented staticserialzer support for PlcLists
- Moved Modbus to use the static serialiser so as not to duplicate serializing code
- Refactored the field handler code, esp for Modbus, no uses a class lookup within PlcValues. Other protocols haven't been tested.
- Added support within the hello world write example for writing arrays.
- Fix for bit lengths within the WriteBuffer writeXXX functions.
----------------------------------------------------------------
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
Re: Cost of Exceptions [was: chrisdutz commented on a change in pull
request #192: Refactor Field Handler Classes]
Posted by Ben Hutcheson <be...@gmail.com>.
Hi Niclas,
I'm definitely in favour of handling errors better. There have been a few
cases I've seen where errors are thrown but the actual error shown doesn't
explain what went wrong, or the error reported is different to the initial
error.
When using the kafka connector (but is probably in the scraper or
pooledconnection somewhere) I noticed that it will try to poll the device
at the same rate as normal (1000ms) when an error occurs. I'm in favour of
making this configurable or using a back-off algorithm.
Kind Regards
Ben
On Tue, Oct 6, 2020 at 5:37 AM Niclas Hedhman <ni...@hedhman.org> wrote:
> On Tue, Oct 6, 2020 at 9:33 AM GitBox <gi...@apache.org> wrote:
>
> > One thing I've learnt recently, is that exceptions are extremely
> > expensive, as creating the stack trace is expensive. is this code that is
> > likely to produce any of "InstantiationException |
> IllegalAccessException |
> > InvocationTargetException | NoSuchMethodException" in normal execution?
> If
> > yes, we should probably change this and other parts where we use similar
> > patterns to check first instead of silently catching exceptions.
> >
>
> Chris is right; Exceptions are incredibly expensive and I have seen
> instances where a few PLCs are put offline, and the supervisory system goes
> into a snail pace, either due to excessive exceptions, aggressive retries
> (without back-off), or both and sometimes with other "oh, didn't think of
> that" ooopsies.
>
> In general, all faults in "real world" should not throw exceptions, and
> should be viewed as "normality". And unfortunately, Java programmers (even
> those in this field) tend to not think in "error scenarios" and only in
> "sunny day paths".
>
> Personally, I got over the "exception craze" about 20 years ago, and try to
> be exception-free. A few tricks to get there;
>
> 1. Event driven systems doesn't need "return values", hence a lot less
> exceptions. Error conditions are different events and subscribers can
> specialize in the handling. Making a system event driven instead of the
> request/response type, takes a fair amount of unlearning and mind bending,
> but worth it.
>
> 2. Try to figure out ahead of time if exceptions may be thrown by code I
> don't control.
>
> 3. Don't create nor rethrow exceptions. If in dev/test mode, log an error
> report containing all possible states that could affect the outcome and
> then System.exit() as to indicate that there is a serious problem that
> should not be ignored. In production, the error report is sent to
> "operations" (containing info to forward to developers if needed) and tries
> to recover.
>
>
> // Niclas
>
Cost of Exceptions [was: chrisdutz commented on a change in pull
request #192: Refactor Field Handler Classes]
Posted by Niclas Hedhman <ni...@hedhman.org>.
On Tue, Oct 6, 2020 at 9:33 AM GitBox <gi...@apache.org> wrote:
> One thing I've learnt recently, is that exceptions are extremely
> expensive, as creating the stack trace is expensive. is this code that is
> likely to produce any of "InstantiationException | IllegalAccessException |
> InvocationTargetException | NoSuchMethodException" in normal execution? If
> yes, we should probably change this and other parts where we use similar
> patterns to check first instead of silently catching exceptions.
>
Chris is right; Exceptions are incredibly expensive and I have seen
instances where a few PLCs are put offline, and the supervisory system goes
into a snail pace, either due to excessive exceptions, aggressive retries
(without back-off), or both and sometimes with other "oh, didn't think of
that" ooopsies.
In general, all faults in "real world" should not throw exceptions, and
should be viewed as "normality". And unfortunately, Java programmers (even
those in this field) tend to not think in "error scenarios" and only in
"sunny day paths".
Personally, I got over the "exception craze" about 20 years ago, and try to
be exception-free. A few tricks to get there;
1. Event driven systems doesn't need "return values", hence a lot less
exceptions. Error conditions are different events and subscribers can
specialize in the handling. Making a system event driven instead of the
request/response type, takes a fair amount of unlearning and mind bending,
but worth it.
2. Try to figure out ahead of time if exceptions may be thrown by code I
don't control.
3. Don't create nor rethrow exceptions. If in dev/test mode, log an error
report containing all possible states that could affect the outcome and
then System.exit() as to indicate that there is a serious problem that
should not be ignored. In production, the error report is sent to
"operations" (containing info to forward to developers if needed) and tries
to recover.
// Niclas
[GitHub] [plc4x] chrisdutz commented on a change in pull request #192: Refactor Field Handler Classes
Posted by GitBox <gi...@apache.org>.
chrisdutz commented on a change in pull request #192:
URL: https://github.com/apache/plc4x/pull/192#discussion_r500057457
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/value/PlcBYTE.java
##########
@@ -169,6 +169,30 @@ public PlcBYTE(@JsonProperty("value") short value) {
}
}
+ @Override
+ @JsonIgnore
+ public boolean isBoolean() {
+ return true;
+ }
+
+ @Override
+ @JsonIgnore
+ public boolean getBoolean() {
+ return (value != null) && !value.equals(0);
+ }
+
+ @Override
+ @JsonIgnore
+ public boolean isByte() {
+ return (value != null) && (value <= Byte.MAX_VALUE) && (value >= Byte.MIN_VALUE);
+ }
+
+ @Override
+ @JsonIgnore
+ public byte getByte() {
+ return value.byteValue();
+ }
+
Review comment:
We now have a "byte getByte()" and a "short getBYTE()" method, I think it should only be the one or the other ... I would like to opt for the "byte getByte()". But right now I'm unsure which implications that would have.
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/value/PlcBYTE.java
##########
@@ -169,6 +169,30 @@ public PlcBYTE(@JsonProperty("value") short value) {
}
}
+ @Override
+ @JsonIgnore
+ public boolean isBoolean() {
+ return true;
+ }
+
+ @Override
+ @JsonIgnore
+ public boolean getBoolean() {
+ return (value != null) && !value.equals(0);
+ }
Review comment:
In general a bit-string (at least in my perception of it, represents an array of booleans. So if for example I read one byte, from the point of the API it should be a list of 8 boolean values ... I think we need to allow this somehow. Right now the only way would be to get the "byte" value and to pick that appart.
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/value/PlcBYTE.java
##########
@@ -181,6 +205,90 @@ public short getShort() {
return value;
}
+ @Override
+ @JsonIgnore
+ public boolean isInteger() {
+ return true;
+ }
+
+ @Override
+ @JsonIgnore
+ public int getInteger() {
+ return value.intValue();
+ }
+
Review comment:
Instead of adding these for every type, wouldn't it make sense to have something like a "PlcIECBitStringValue", "PlcIECIntegerValue", ...?
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/value/PlcBYTE.java
##########
@@ -181,6 +205,90 @@ public short getShort() {
return value;
}
+ @Override
+ @JsonIgnore
+ public boolean isInteger() {
+ return true;
+ }
+
+ @Override
+ @JsonIgnore
+ public int getInteger() {
+ return value.intValue();
+ }
+
Review comment:
But thinking of it ... perhaps making it generic and passing in the type would help ...
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/value/PlcValues.java
##########
@@ -355,6 +356,28 @@ public static PlcValue of(Map<String, PlcValue> map) {
return new PlcStruct(map);
}
+ private static PlcValue constructorHelper(Constructor<?> constructor, Object value) {
+ try {
+ return (PlcValue) constructor.newInstance(value);
+ } catch (InstantiationException | IllegalAccessException | InvocationTargetException e) {
+ throw new PlcIncompatibleDatatypeException(value.getClass());
+ }
+ }
+
+ public static PlcValue of(Object[] values, Class clazz) {
+ //Encode values to the type defined in clazz
+ try {
+ Constructor<?> constructor = clazz.getDeclaredConstructor(values[0].getClass());
+ if(values.length == 1) {
+ return ((PlcValue) constructor.newInstance(values[0]));
+ } else {
+ return PlcValues.of(Arrays.stream(values).map(value -> (constructorHelper(constructor, value))).collect(Collectors.toList()));
+ }
Review comment:
One thing I've learnt recently, is that exceptions are extremely expensive, as creating the stack trace is expensive. is this code that is likely to produce any of "InstantiationException | IllegalAccessException | InvocationTargetException | NoSuchMethodException" in normal execution? If yes, we should probably change this and other parts where we use similar patterns to check first instead of silently catching exceptions.
----------------------------------------------------------------
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 #192: Refactor Field Handler Classes
Posted by GitBox <gi...@apache.org>.
hutcheb commented on a change in pull request #192:
URL: https://github.com/apache/plc4x/pull/192#discussion_r500131203
##########
File path: plc4j/api/src/main/java/org/apache/plc4x/java/api/value/PlcValues.java
##########
@@ -355,6 +356,28 @@ public static PlcValue of(Map<String, PlcValue> map) {
return new PlcStruct(map);
}
+ private static PlcValue constructorHelper(Constructor<?> constructor, Object value) {
+ try {
+ return (PlcValue) constructor.newInstance(value);
+ } catch (InstantiationException | IllegalAccessException | InvocationTargetException e) {
+ throw new PlcIncompatibleDatatypeException(value.getClass());
+ }
+ }
+
+ public static PlcValue of(Object[] values, Class clazz) {
+ //Encode values to the type defined in clazz
+ try {
+ Constructor<?> constructor = clazz.getDeclaredConstructor(values[0].getClass());
+ if(values.length == 1) {
+ return ((PlcValue) constructor.newInstance(values[0]));
+ } else {
+ return PlcValues.of(Arrays.stream(values).map(value -> (constructorHelper(constructor, value))).collect(Collectors.toList()));
+ }
Review comment:
The exception would be raised if there wasn't a class (PlcBYTE, PlcINT, etc..) that matched the datatype that was passed. It shouldn't be raised in normal operation.
----------------------------------------------------------------
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