date:20180802

Hi Chris,



an additional flag in the items was also my first idea, but after thinking 
about it a bit I dislike it.

First, because it "breaks" the encapsulation we currently have between "Java" 
Information (Class) and Plc Information (Address) and second between it is 
not very scalable when we come up with other "traits" of the byte 
representation (e.g., Endianness).



As I really like this "you only need a suitable String to address the PLC and 
the Driver will work it out at runtime" approach which really eases 
applications as you only need one config string from file or DB or so this is 
another drawback for the "additional flag" approach.



What I would prefer currently is to encode this Information in the Address 
which is already the only point which is PLC Specific.

My idea is to add an (optional) postfix to the address which allows to pass 
informations about the storage representation of Data, e.g.,

- width

- Traits like

- unsigned

- decimal

- endianness

- ...

An example would be

"FLAGS/0/0:8U"

To Address an unsigned Byte at Offset zero.



To make it even easier to users we could provide Plc specific enums like the 
MemoryArea enum where "typical" types can be predefined that are also allowed 
as postfix.

Then, the Adress could become

"FLAGS/0/0:USINT"



This would lead to a good level of encapsulation as I could go to my S7 
programmer and ask him to give me this string for all values I'm interested in 
and its totally in his "ubiquitous language" (yay, DDD trending).



The enum would be something like this (S7 case):

public enum S7NativeRepresentations {



SINT(new 
Representation(8,Collections.singletonList(Representation.Trait.SIGNED))),

USINT(new 
Representation(8,Collections.singletonList(Representation.Trait.UNSIGNED))),

INT(new 
Representation(16,Collections.singletonList(Representation.Trait.SIGNED))),

UINT(new 
Representation(16,Collections.singletonList(Representation.Trait.UNSIGNED))),

DINT(new 
Representation(32,Collections.singletonList(Representation.Trait.SIGNED))),

UDINT(new 
Representation(32,Collections.singletonList(Representation.Trait.UNSIGNED))),

REAL(new Representation(32, 
Collections.singletonList(Representation.Trait.DECIMAL))),

LREAL(new Representation(64, 
Collections.singletonList(Representation.Trait.DECIMAL)));



private Representation representation;



S7NativeRepresentations(Representation representation) {

this.representation = representation;

}

}



Where the Representation class and the Trait enum are "Global" for all PLCs and 
are only interpreted at runtime for the casting by each PLC.

This would allow a general handling of the "representation" and address 
suffixes but with the additional flexibility of Plc type specific "Shortcuts".



As I currently see a lot of advantages with this approach its up to you to find 
the drawbacks : )



What do all of you think of this Idea?



Julian



PS.: With this information given we could give users also the flexibility to 
request the Java Type they need and could take care of widening / narrowing or 
conversion internally.



Am 02.08.18, 19:04 schrieb "Christofer Dutz" :



Hi Julian,



I do get your point and do agree that there could be problems.



Just an idea ... How about an optional field "signed" in the items which 
defaults to false?



It seems handling the signed and unsignedness of an item simply by the type 
could be a bad idea, the more I think of it.



We should also clearly define and document the expected types and their 
sizes and use the same for all drivers.



What do you all think about this?



Chris



Outlook for Android herunterladen







Von: Julian Feinauer

Gesendet: Donnerstag, 2. August, 17:14

Betreff: Re: Handling of signed / unsigned values

An: dev@plc4x.apache.org





Hi Chris, I think we have both different approaches and views. I totally 
agree with you that it should be as straight forward and easy for users of the 
API to use Plc4x. But, as far as my understanding goes, we are missing some 
information which we need for the user. The following example is with regard to 
S7. Assume we have a datablock with two values in it (and nothing more), one 
int (as S7 int -> 2 bytes) and one unsigned int (again 2 bytes), i.e., 
- | int | uint | - We usually get TIA programs and then 
write our java application (or configuration) to read values from the device. 
So, if I am not carefully I see Int and do: PlcReadRequest(Integer.class, 
".../0") PlcReadRequest(Integer.class, ".../2") Which would lead to one wrong 
result (as the Integer is casted from 4 bytes) and one Exception (Unknown 
Memory, I think, as we cross the DB boundary because we try to read 4 bytes

Re: Handling of signed / unsigned values

Hi Julian,

I do get your point and do agree that there could be problems.

Just an idea ... How about an optional field "signed" in the items which 
defaults to false?

It seems handling the signed and unsignedness of an item simply by the type 
could be a bad idea, the more I think of it.

We should also clearly define and document the expected types and their sizes 
and use the same for all drivers.

What do you all think about this?

Chris

Outlook for Android herunterladen



Von: Julian Feinauer
Gesendet: Donnerstag, 2. August, 17:14
Betreff: Re: Handling of signed / unsigned values
An: dev@plc4x.apache.org


Hi Chris, I think we have both different approaches and views. I totally agree 
with you that it should be as straight forward and easy for users of the API to 
use Plc4x. But, as far as my understanding goes, we are missing some 
information which we need for the user. The following example is with regard to 
S7. Assume we have a datablock with two values in it (and nothing more), one 
int (as S7 int -> 2 bytes) and one unsigned int (again 2 bytes), i.e., 
- | int | uint | - We usually get TIA programs and then 
write our java application (or configuration) to read values from the device. 
So, if I am not carefully I see Int and do: PlcReadRequest(Integer.class, 
".../0") PlcReadRequest(Integer.class, ".../2") Which would lead to one wrong 
result (as the Integer is casted from 4 bytes) and one Exception (Unknown 
Memory, I think, as we cross the DB boundary because we try to read 4 bytes 
from offset 2). Then, I see my failure and take Javas Short: 
PlcReadRequest(Short.class, ".../0") PlcReadRequest(Short.class, ".../2") So no 
exception here. But I get one correct value (first one) and one wrong one (the 
cast to short assumes a signed representation). From a java perspective I 
should do PlcReadRequest(Short.class, ".../0") PlcReadRequest(Integer.class, 
".../2") Because the second unsigned int (S7 UINT) is greater than java Short, 
but fits perfetctly in Javas Integer. But of couse, this would give again an 
exception. From my perspective, the point missing here is some sort of Shema 
which helps PLC4J to know the datatype in the PLC behind the scenes and takes 
care of all the narrowing or widening (or even conversion between integer and 
float types) in the background for me (in fact we could possibly return valid 
results for all 3 examples if the UINT is small enough, otherwise only the 
second example would fail). So my question about signed and unsigned is less 
about representation bot more about how we tell the S7Protocoll how to cast the 
respective byte array that is returned from the Plc. I hope this makes my 
question more clear. Julian Am 02.08.18, 11:33 schrieb "Christofer Dutz" : Hi 
Julian, regarding your question. As far as I have encountered, PLCs mostly 
transfer unsigned values and Java usually uses signed values ... this could 
generally cause problems. Fortunately as far as I know the size of the Java 
types is usually way bigger than the one of the PLC types. In case of the Int: 
The Siemens S7 Int datatypes is two bytes and the Java Integer is a 32 bit 
integer, therefore we don't have to confuse our users with any type problems. 
If however a PLC would use 32 bit integers we would be having problems. In this 
case we would have to use the next smaller datatype that fits our requested 
datatype. So in this case reading a "Java Integer" would read a "PLC Short". I 
wouldn't like to have the user have to think of the PLC datatypes when writing 
his code. Chris Am 02.08.18, 11:17 schrieb "Julian Feinauer" : Hey all, again 
me with another question : ) I started going through some examples on our PLC 
and came to a situation where we use signed and unsigned values in the PLC. 
This goes kind of back to my type system question. How could I tell the Reader 
to read me an Unsigned Int from a S7 (Usigned Int refers in this case to a two 
byte value on the PLC but return type had to be Int in Java). Is there some 
mechanism in Place to be able to do such a Thing? Or if not, do you have any 
ideas already in mind how one could introduce this (technically it's clear but 
how to give the information that we want our expected int to be read and 
interpreted as 2 byte unsigned)? Best Julian

[GitHub] chrisdutz commented on issue #10: Fixed NPE in S7PlcConnection#close.

chrisdutz commented on issue #10: Fixed NPE in S7PlcConnection#close.
URL: https://github.com/apache/incubator-plc4x/pull/10#issuecomment-409995467
 
 
   Well I Ave started working on an option that works exactly the same way we 
are doing the connection, by creating a CompletableFuture and commuting this in 
a channel.closeListener ... Will finish that option tomorrow and have a look at 
your proposal tomorrow as well. Think we are all learning more and more with 
stuff like this ;-)


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

[GitHub] JulianFeinauer commented on issue #10: Fixed NPE in S7PlcConnection#close.

JulianFeinauer commented on issue #10: Fixed NPE in S7PlcConnection#close.
URL: https://github.com/apache/incubator-plc4x/pull/10#issuecomment-409963436
 
 
   @sruehl my understanding is that netty avoids a scenario where one uses the 
event loop executors threads for blocking and thus, risks a deadlock. This 
should not be the case in this situation because the EventLoops Thread wich 
does the listener notification only counts down the latch.
   
   But as this solution seems uglier than it should be it is possibly a good 
idea to go over to the netty list and ask there (I am totally new to netty).


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

Re: Handling of signed / unsigned values

Hi Chris,



I think we have both different approaches and views.

I totally agree with you that it should be as straight forward and easy for 
users of the API to use Plc4x.

But, as far as my understanding goes, we are missing some information which we 
need for the user.



The following example is with regard to S7.

Assume we have a datablock with two values in it (and nothing more), one int 
(as S7 int -> 2 bytes) and one unsigned int (again 2 bytes), i.e.,



-

| int | uint |

-



We usually get TIA programs and then write our java application (or 
configuration) to read values from the device.

So, if I am not carefully I see Int and do:



PlcReadRequest(Integer.class, ".../0")

PlcReadRequest(Integer.class, ".../2")



Which would lead to one wrong result (as the Integer is casted from 4 bytes) 
and one Exception (Unknown Memory, I think, as we cross the DB boundary because 
we try to read 4 bytes from offset 2).



Then, I see my failure and take Javas Short:



PlcReadRequest(Short.class, ".../0")

PlcReadRequest(Short.class, ".../2")



So no exception here.

But I get one correct value (first one) and one wrong one (the cast to short 
assumes a signed representation).



From a java perspective I should do



PlcReadRequest(Short.class, ".../0")

PlcReadRequest(Integer.class, ".../2")



Because the second unsigned int (S7 UINT) is greater than java Short, but fits 
perfetctly in Javas Integer.

But of couse, this would give again an exception.



From my perspective, the point missing here is some sort of Shema which helps 
PLC4J to know the datatype in the PLC behind the scenes and takes care of all 
the narrowing or widening (or even conversion between integer and float types) 
in the background for me (in fact we could possibly return valid results for 
all 3 examples if the UINT is small enough, otherwise only the second example 
would fail).



So my question about signed and unsigned is less about representation bot more 
about how we tell the S7Protocoll how to cast the respective byte array that is 
returned from the Plc.



I hope this makes my question more clear.



Julian





Am 02.08.18, 11:33 schrieb "Christofer Dutz" :



Hi Julian,







regarding your question.







As far as I have encountered, PLCs mostly transfer unsigned values and Java 
usually uses signed values ... this could generally cause problems. 



Fortunately as far as I know the size of the Java types is usually way 
bigger than the one of the PLC types. 







In case of the Int: The Siemens S7 Int datatypes is two bytes and the Java 
Integer is a 32 bit integer, therefore we don't have to confuse our users with 
any type problems.



If however a PLC would use 32 bit integers we would be having problems. In 
this case we would have to use the next smaller datatype that fits our 
requested datatype. So in this case reading a "Java Integer" would read a "PLC 
Short". I wouldn't like to have the user have to think of the PLC datatypes 
when writing his code. 











Chris







Am 02.08.18, 11:17 schrieb "Julian Feinauer" :







Hey all,











again me with another question : )











I started going through some examples on our PLC and came to a 
situation where we use signed and unsigned values in the PLC.







This goes kind of back to my type system question.







How could I tell the Reader to read me an Unsigned Int from a S7 
(Usigned Int refers in this case to a two byte value on the PLC but return type 
had to be Int in Java).







Is there some mechanism in Place to be able to do such a Thing?











Or if not, do you have any ideas already in mind how one could 
introduce this (technically it's clear but how to give the information that we 
want our expected int to be read and interpreted as 2 byte unsigned)?











Best







Julian

[GitHub] sruehl commented on issue #10: Fixed NPE in S7PlcConnection#close.

sruehl commented on issue #10: Fixed NPE in S7PlcConnection#close.
URL: https://github.com/apache/incubator-plc4x/pull/10#issuecomment-409959876
 
 
   one note at this point. As far as I understand netty throws this exception 
to indicate that it dislikes when the thread gets blocked. By using @chrisdutz 
or @JulianFeinauer code we might just trick the detection of netty. Im not 
quite sure but I think this is then more a hack/workaround rather then the 
intended use of netty. What I found what should come close to the intended 
behavior ist 
[this](https://netty.io/4.0/api/io/netty/util/concurrent/SingleThreadEventExecutor.html#addShutdownHook-java.lang.Runnable-).
 Maybe we should ask on the netty list how it is expected to do such things...


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

[GitHub] JulianFeinauer commented on issue #10: Fixed NPE in S7PlcConnection#close.

JulianFeinauer commented on issue #10: Fixed NPE in S7PlcConnection#close.
URL: https://github.com/apache/incubator-plc4x/pull/10#issuecomment-409958048
 
 
   @chrisdutz your fixes in the master made it work for me also.
   As I found a solution to avoid the blocking exception in the meanwhile I 
combined your code with mine.
   I think the only obvious solution is, as i propose, to use a CountDownLatch 
to communicate between the ChannelCloseFuture and the close method on another 
ThreadPool than the EventLoopGroup (which netty forbids).
   Is this solution feasible for you?


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

How to handle datatype optimizations

Hi all,

I just had a little technical discussion with Sebastian and I thought it would 
be worth to include you guys.

We were discussing how we handle datatypes and optimized access to fields.

Sebastian is currently working on the Modbus integration. Here digital inputs 
can be accessed via so-called coils which are nothing else than Boolean values.

The main point of the discussion was, that I initially had the position, that 
if I want to read multiple consequent bits, that I should be able to for 
example read one or more bytes.
As I know that for Siemens this is exactly how reading the Boolean values is 
used quite often. My argument was that I would like PLC4X to allow the same 
optimizations no matter what protocol we are using.

But in this discussion, Sebastian managed to convince me that it is indeed 
better to not do this.
During this discussion I realized that I was doing some sort of optimization I 
knew the driver could support for one protocol and forcing the other drivers to 
allow the same.
If I want to read multiple consequent bits, for example, I should implement my 
application to do exactly that.
Optimizing the access path to group multiple bits to one byte request should be 
handled by the driver.
This would be the only true driver-independent way to implement the application.

I know that the way Siemens memory is setup, I could not prevent someone from 
reading all digital inputs in one byte, but the request going over the wire 
should be exactly the same if I request one byte or I request 8 subsequent bits 
because ideally the driver should perform the optimization for me. We will have 
to implement this sort of optimization anyway as if I read bytes 2,3,4,6 it is 
always better to simply read one array containing bytes 2-6 and simply ignore 
byte 5.

Do you agree on this?

Chris

[GitHub] chrisdutz commented on a change in pull request #10: Fixed NPE in S7PlcConnection#close.

chrisdutz commented on a change in pull request #10: Fixed NPE in 
S7PlcConnection#close.
URL: https://github.com/apache/incubator-plc4x/pull/10#discussion_r207224977
 
 

 ##
 File path: 
plc4j/protocols/s7/src/test/java/org/apache/plc4x/java/s7/connection/S7PlcConnectionIT.java
 ##
 @@ -58,7 +58,12 @@ public void setUp() {
 @After
 public void tearDown() {
 if(s7PlcConnection.isConnected()) {
-s7PlcConnection.close();
+try {
+s7PlcConnection.close();
+} catch (NullPointerException e) {
 
 Review comment:
   Hi Julian ... I just updated master ... could you check if this solves your 
problem?


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

[GitHub] JulianFeinauer commented on a change in pull request #10: Fixed NPE in S7PlcConnection#close.

JulianFeinauer commented on a change in pull request #10: Fixed NPE in 
S7PlcConnection#close.
URL: https://github.com/apache/incubator-plc4x/pull/10#discussion_r207200954
 
 

 ##
 File path: 
plc4j/protocols/s7/src/test/java/org/apache/plc4x/java/s7/connection/S7PlcConnectionIT.java
 ##
 @@ -58,7 +58,12 @@ public void setUp() {
 @After
 public void tearDown() {
 if(s7PlcConnection.isConnected()) {
-s7PlcConnection.close();
+try {
+s7PlcConnection.close();
+} catch (NullPointerException e) {
 
 Review comment:
   @chrisdutz how is your setup?
   For me, this removed the exception here for me with my S7.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

[GitHub] sruehl commented on a change in pull request #10: Fixed NPE in S7PlcConnection#close.

sruehl commented on a change in pull request #10: Fixed NPE in 
S7PlcConnection#close.
URL: https://github.com/apache/incubator-plc4x/pull/10#discussion_r207198528
 
 

 ##
 File path: 
plc4j/protocols/s7/src/test/java/org/apache/plc4x/java/s7/connection/S7PlcConnectionIT.java
 ##
 @@ -58,7 +58,12 @@ public void setUp() {
 @After
 public void tearDown() {
 if(s7PlcConnection.isConnected()) {
-s7PlcConnection.close();
+try {
+s7PlcConnection.close();
+} catch (NullPointerException e) {
 
 Review comment:
   apparently blocking operations are not allowed (`await()`)


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

[GitHub] sruehl commented on a change in pull request #10: Fixed NPE in S7PlcConnection#close.

sruehl commented on a change in pull request #10: Fixed NPE in 
S7PlcConnection#close.
URL: https://github.com/apache/incubator-plc4x/pull/10#discussion_r207197726
 
 

 ##
 File path: 
plc4j/protocols/s7/src/test/java/org/apache/plc4x/java/s7/connection/S7PlcConnectionIT.java
 ##
 @@ -58,7 +58,12 @@ public void setUp() {
 @After
 public void tearDown() {
 if(s7PlcConnection.isConnected()) {
-s7PlcConnection.close();
+try {
+s7PlcConnection.close();
+} catch (NullPointerException e) {
 
 Review comment:
   route cause of failing test is 
`io.netty.util.concurrent.BlockingOperationException` and only NPE gets caught 
so the test still fails


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

[GitHub] JulianFeinauer commented on a change in pull request #10: Fixed NPE in S7PlcConnection#close.

JulianFeinauer commented on a change in pull request #10: Fixed NPE in 
S7PlcConnection#close.
URL: https://github.com/apache/incubator-plc4x/pull/10#discussion_r207178132
 
 

 ##
 File path: 
plc4j/protocols/s7/src/main/java/org/apache/plc4x/java/s7/connection/S7PlcConnection.java
 ##
 @@ -183,14 +194,19 @@ public void close() {
 (short) 0x, (short) 0x000F, DisconnectReason.NORMAL, 
Collections.emptyList(),
 null);
 ChannelFuture sendDisconnectRequestFuture = 
channel.writeAndFlush(disconnectRequest);
-sendDisconnectRequestFuture.addListener((ChannelFutureListener) 
future -> {
-// Close the session itself.
-channel.closeFuture().await();
+// Wait if the PLC closes the connection
+try {
+// TODO 02.08.18 jf: Do we have global constants for things 
like timeouts?
 
 Review comment:
   fixed and removed todo.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

[GitHub] sruehl commented on a change in pull request #10: Fixed NPE in S7PlcConnection#close.

sruehl commented on a change in pull request #10: Fixed NPE in 
S7PlcConnection#close.
URL: https://github.com/apache/incubator-plc4x/pull/10#discussion_r207148665
 
 

 ##
 File path: 
plc4j/protocols/s7/src/main/java/org/apache/plc4x/java/s7/connection/S7PlcConnection.java
 ##
 @@ -183,14 +194,19 @@ public void close() {
 (short) 0x, (short) 0x000F, DisconnectReason.NORMAL, 
Collections.emptyList(),
 null);
 ChannelFuture sendDisconnectRequestFuture = 
channel.writeAndFlush(disconnectRequest);
-sendDisconnectRequestFuture.addListener((ChannelFutureListener) 
future -> {
-// Close the session itself.
-channel.closeFuture().await();
+// Wait if the PLC closes the connection
+try {
+// TODO 02.08.18 jf: Do we have global constants for things 
like timeouts?
+channel.closeFuture().await(1_000);
+} catch (InterruptedException e) {
+logger.warn("Connection was not closed by PLC, has to be 
closed from driver side now.", e);
+} finally {
+// close the session itself.
 channel.eventLoop().parent().shutdownGracefully();
 
 Review comment:
   One thing to note here. If we put this in the finally blog  a potential 
thrown exception is omitted. Especially as the method is called `gracefully`. I 
would suggest to put it outside a finally or wrap it with a try catch ignore.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

[GitHub] sruehl commented on a change in pull request #10: Fixed NPE in S7PlcConnection#close.

sruehl commented on a change in pull request #10: Fixed NPE in 
S7PlcConnection#close.
URL: https://github.com/apache/incubator-plc4x/pull/10#discussion_r207149145
 
 

 ##
 File path: 
plc4j/protocols/s7/src/main/java/org/apache/plc4x/java/s7/connection/S7PlcConnection.java
 ##
 @@ -183,14 +194,19 @@ public void close() {
 (short) 0x, (short) 0x000F, DisconnectReason.NORMAL, 
Collections.emptyList(),
 null);
 ChannelFuture sendDisconnectRequestFuture = 
channel.writeAndFlush(disconnectRequest);
-sendDisconnectRequestFuture.addListener((ChannelFutureListener) 
future -> {
-// Close the session itself.
-channel.closeFuture().await();
+// Wait if the PLC closes the connection
+try {
+// TODO 02.08.18 jf: Do we have global constants for things 
like timeouts?
+channel.closeFuture().await(1_000);
+} catch (InterruptedException e) {
+logger.warn("Connection was not closed by PLC, has to be 
closed from driver side now.", e);
+} finally {
+// close the session itself.
 channel.eventLoop().parent().shutdownGracefully();
 
 Review comment:
   example code: 
   ```
   try {
   try {
   throw new RuntimeException("Important information");
   } finally {
   throw new OutOfMemoryError("Something failed gracefully");
   }
   } catch (Throwable e) {
   assert e.class == OutOfMemoryError.class;
   }
   ```


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

[GitHub] sruehl commented on a change in pull request #10: Fixed NPE in S7PlcConnection#close.

sruehl commented on a change in pull request #10: Fixed NPE in 
S7PlcConnection#close.
URL: https://github.com/apache/incubator-plc4x/pull/10#discussion_r207145613
 
 

 ##
 File path: 
plc4j/protocols/s7/src/main/java/org/apache/plc4x/java/s7/connection/S7PlcConnection.java
 ##
 @@ -183,14 +194,19 @@ public void close() {
 (short) 0x, (short) 0x000F, DisconnectReason.NORMAL, 
Collections.emptyList(),
 null);
 ChannelFuture sendDisconnectRequestFuture = 
channel.writeAndFlush(disconnectRequest);
-sendDisconnectRequestFuture.addListener((ChannelFutureListener) 
future -> {
-// Close the session itself.
-channel.closeFuture().await();
+// Wait if the PLC closes the connection
+try {
+// TODO 02.08.18 jf: Do we have global constants for things 
like timeouts?
+channel.closeFuture().await(1_000);
+} catch (InterruptedException e) {
+logger.warn("Connection was not closed by PLC, has to be 
closed from driver side now.", e);
 
 Review comment:
   Here we should add a `Thread.currentThread().interrupt();` after the 
logging. (Sonar would tell you this later anyway ;)


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

Re: Handling of signed / unsigned values

Hi Julian,



regarding your question.



As far as I have encountered, PLCs mostly transfer unsigned values and Java 
usually uses signed values ... this could generally cause problems. 

Fortunately as far as I know the size of the Java types is usually way bigger 
than the one of the PLC types. 



In case of the Int: The Siemens S7 Int datatypes is two bytes and the Java 
Integer is a 32 bit integer, therefore we don't have to confuse our users with 
any type problems.

If however a PLC would use 32 bit integers we would be having problems. In this 
case we would have to use the next smaller datatype that fits our requested 
datatype. So in this case reading a "Java Integer" would read a "PLC Short". I 
wouldn't like to have the user have to think of the PLC datatypes when writing 
his code. 





Chris



Am 02.08.18, 11:17 schrieb "Julian Feinauer" :



Hey all,





again me with another question : )





I started going through some examples on our PLC and came to a situation 
where we use signed and unsigned values in the PLC.



This goes kind of back to my type system question.



How could I tell the Reader to read me an Unsigned Int from a S7 (Usigned 
Int refers in this case to a two byte value on the PLC but return type had to 
be Int in Java).



Is there some mechanism in Place to be able to do such a Thing?





Or if not, do you have any ideas already in mind how one could introduce 
this (technically it's clear but how to give the information that we want our 
expected int to be read and interpreted as 2 byte unsigned)?





Best



Julian

[GitHub] asfgit closed pull request #9: Added some javadoc to S7 communication path and several todos that ma…

asfgit closed pull request #9: Added some javadoc to S7 communication path and 
several todos that ma…
URL: https://github.com/apache/incubator-plc4x/pull/9
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git 
a/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcReadRequest.java
 
b/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcReadRequest.java
index 96db08f0e..456f1f3d9 100644
--- 
a/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcReadRequest.java
+++ 
b/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcReadRequest.java
@@ -25,9 +25,24 @@ Licensed to the Apache Software Foundation (ASF) under one
 import java.util.List;
 import java.util.Objects;
 
+/**
+ * Request to read one or more values from a plc.
+ * The {@link PlcReadRequest} is a container for one or more {@link 
ReadRequestItem}s that are contained.
+ *
+ * By default this is NOT typesafe as it can contain {@link ReadRequestItem}s 
for different types.
+ * If there are only {@link ReadRequestItem}s of the same type one can use the 
{@link TypeSafePlcReadRequest} to enforce
+ * type safety.
+ *
+ * Provides a builder for object construction through {@link 
PlcReadRequest#builder()}.
+ *
+ * TODO 01.08.2018 jf: Can we make constructors private and force users to use 
the Builder to enforce immutability?
+ *
+ * @see TypeSafePlcReadRequest
+ */
 public class PlcReadRequest extends PlcRequest> {
 
 public PlcReadRequest() {
+// Exists for construction of inherited classes
 }
 
 public PlcReadRequest(ReadRequestItem requestItem) {
diff --git 
a/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcReadResponse.java
 
b/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcReadResponse.java
index 78ebdea77..136f47848 100644
--- 
a/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcReadResponse.java
+++ 
b/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/PlcReadResponse.java
@@ -25,6 +25,16 @@ Licensed to the Apache Software Foundation (ASF) under one
 import java.util.List;
 import java.util.Optional;
 
+/**
+ * Response to a {@link PlcReadRequest}.
+ * Contains the values read from the PLC but untyped.
+ *
+ * Values are extracted using the {@link ReadRequestItem}s that were send in 
the read request.
+ *
+ * If only a variables of one type are requested it is better to use
+ * {@link org.apache.plc4x.java.api.messages.specific.TypeSafePlcReadRequest} 
which leads to a
+ * {@link org.apache.plc4x.java.api.messages.specific.TypeSafePlcReadResponse}.
+ */
 public class PlcReadResponse extends PlcResponse, ReadRequestItem> {
 
 public PlcReadResponse(PlcReadRequest request, ReadResponseItem 
responseItems) {
diff --git 
a/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/items/ReadRequestItem.java
 
b/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/items/ReadRequestItem.java
index bc77dc518..3f4f68e0a 100644
--- 
a/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/items/ReadRequestItem.java
+++ 
b/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/items/ReadRequestItem.java
@@ -22,6 +22,18 @@ Licensed to the Apache Software Foundation (ASF) under one
 
 import java.util.Objects;
 
+/**
+ * Encapsulats one {@link RequestItem} that could be read multiple times, 
i.e., from the given Address on
+ * {@link ReadRequestItem#size} number of Items with equal datatype are read.
+ *
+ * Thus,
+ * 
+ * new ReadRequestItem(Int.class, adress, 5)
+ * 
+ * basically reads 5 consecutive integers starting at the given {@link 
Address}.
+ *
+ * @param  Generic Type of expected Datatype.
+ */
 public class ReadRequestItem extends RequestItem {
 
 private final int size;
@@ -52,6 +64,8 @@ public boolean equals(Object o) {
 return false;
 }
 ReadRequestItem that = (ReadRequestItem) o;
+// TODO 01.18.18 jf: we should also call the comparison of super at 
least otherwise this can lead to unwanted behavior.
+// Perhaps we should generate a UUID or something for each ReadRequest 
to have a good equality comparison
 return size == that.size;
 }
 
diff --git 
a/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/items/ReadResponseItem.java
 
b/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/items/ReadResponseItem.java
index f7736e879..97cb36e7c 100644
--- 
a/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/items/ReadResponseItem.java
+++ 
b/plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/items/ReadResponseItem.java
@@ -24,6 +24,12 @@ Licensed to the Apache Software Foundation (ASF) under one
 import java.util.List;
 import java.util.Objects;
 
+/**
+ * Response to a {@link ReadRequestItem}.
+ * Can

Handling of signed / unsigned values

Hey all,


again me with another question : )


I started going through some examples on our PLC and came to a situation where 
we use signed and unsigned values in the PLC.

This goes kind of back to my type system question.

How could I tell the Reader to read me an Unsigned Int from a S7 (Usigned Int 
refers in this case to a two byte value on the PLC but return type had to be 
Int in Java).

Is there some mechanism in Place to be able to do such a Thing?


Or if not, do you have any ideas already in mind how one could introduce this 
(technically it's clear but how to give the information that we want our 
expected int to be read and interpreted as 2 byte unsigned)?


Best

Julian

Re: Feedback to the TODO items of Julians first Pull Request

Ah and I missed the suggestion to rename the VarParameter to something with
Command in it:

This is a well-established name for this part of the package, so you will find
reference to parameters all over the internet. Secondly these classes are all
internal classes used by the driver implementation. No user should ever use
these, so I think we should stick to the well-established names.

Chris

Am 02.08.18, 10:47 schrieb "Christofer Dutz" :

Hi,

for the sake of completeness I am bringing aggregated feedback I also added
on the pull request here, so it’s archived.

I would probably remove the TODO items from the code, as I think these
should rather be discussions on the list and/or Jira issues and we shouldn’t be
having discussions in the code (just my opinion)

* Using UUID for equality checks: I would hesitate using these, as
generating a UUID consumes a lot of CPU power and each UUID might consume more
memory than the objects payload.

* Cleanup of the request map in the S7Protocol (and others): I
absolutely agree on this. This was one of the things I knew we will be needing
but was hesitant to start working on this, as I didn’t have an idea on how to
do this Netty-style. Fortunately when looking into the Enthernet/IP drivers
internals, the author there is doing exactly this sort of thing:
https://github.com/digitalpetri/ethernet-ip/blob/master/cip-client/src/main/java/com/digitalpetri/enip/cip/CipClient.java
(Have a look at the “pending” and “timeouts” properties).

* Making the items immutable … I agree :-)

But as I mentioned, this is just my opinion.

Chris

[GitHub] JulianFeinauer commented on a change in pull request #9: Added some javadoc to S7 communication path and several todos that ma…

JulianFeinauer commented on a change in pull request #9: Added some javadoc to 
S7 communication path and several todos that ma…
URL: https://github.com/apache/incubator-plc4x/pull/9#discussion_r207149237
 
 

 ##
 File path: 
plc4j/protocols/s7/src/main/java/org/apache/plc4x/java/s7/netty/model/params/VarParameter.java
 ##
 @@ -24,6 +24,13 @@ Licensed to the Apache Software Foundation (ASF) under one
 
 import java.util.List;
 
+/**
+ * "Command" Message to inform PLC of wanted operation.
+ * Also contains {@link VarParameter#items} that hold detailed information on 
the "Command", e.g.,
+ * addresses to read or to write to.
+ *
+ * TODO 01.08.18 jf: Could this be renamed to Something like Command as this 
seems to be the command message pattern?
 
 Review comment:
   Okay, the correlation makes sense.
   But its good to know that my assumption was right.
   It simply helped my understanding of the process to see them as that.


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

Feedback to the TODO items of Julians first Pull Request

Hi,

for the sake of completeness I am bringing aggregated feedback I also added on 
the pull request here, so it’s archived.

I would probably remove the TODO items from the code, as I think these should 
rather be discussions on the list and/or Jira issues and we shouldn’t be having 
discussions in the code (just my opinion)


  *   Using UUID for equality checks: I would hesitate using these, as 
generating a UUID consumes a lot of CPU power and each UUID might consume more 
memory than the objects payload.
  *   Cleanup of the request map in the S7Protocol (and others): I absolutely 
agree on this. This was one of the things I knew we will be needing but was 
hesitant to start working on this, as I didn’t have an idea on how to do this 
Netty-style. Fortunately when looking into the Enthernet/IP drivers internals, 
the author there is doing exactly this sort of thing: 
https://github.com/digitalpetri/ethernet-ip/blob/master/cip-client/src/main/java/com/digitalpetri/enip/cip/CipClient.java
 (Have a look at the “pending” and “timeouts” properties).
  *   Making the items immutable … I agree :-)

But as I mentioned, this is just my opinion.

Chris

[GitHub] chrisdutz commented on a change in pull request #9: Added some javadoc to S7 communication path and several todos that ma…

chrisdutz commented on a change in pull request #9: Added some javadoc to S7 
communication path and several todos that ma…
URL: https://github.com/apache/incubator-plc4x/pull/9#discussion_r207142023
 
 

 ##
 File path: 
plc4j/api/src/main/java/org/apache/plc4x/java/api/messages/items/ReadRequestItem.java
 ##
 @@ -52,6 +64,8 @@ public boolean equals(Object o) {
 return false;
 }
 ReadRequestItem that = (ReadRequestItem) o;
+// TODO 01.18.18 jf: we should also call the comparison of super at 
least otherwise this can lead to unwanted behavior.
 
 Review comment:
   Aren't we doing exactly this super check 4 lines above?


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

[GitHub] JulianFeinauer opened a new pull request #10: Fixed NPE in S7PlcConnection#close.

JulianFeinauer opened a new pull request #10: Fixed NPE in 
S7PlcConnection#close.
URL: https://github.com/apache/incubator-plc4x/pull/10
 
 
   Fix for the NPE in S7PlcConnection's close method.
   After fixing the NPE there was a netty blockingException, thus I had to 
refactor the program logic a bit.
   Please check if this is suitable the way I dit this (I tried to add enough 
comment to make it understandable).


This is an automated message from the Apache Git Service.
To respond to the message, please log on 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


With regards,
Apache Git Services

AW: Exception on closing S7PlcConnection