chrisdutz commented on code in PR #2673:
URL: https://github.com/apache/plc4x/pull/2673#discussion_r3724123802
##########
plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/EipTcpConnection.java:
##########
@@ -767,12 +765,7 @@ private static byte[] toAnsi(String tag) throws
BufferException {
segments.add(newSegment);
lengthBytes += newSegment.getLengthInBytes();
}
- WriteBufferByteBased buffer = new WriteBufferByteBased(new
byte[lengthBytes],
- WithByteBasedOption.WithByteOrder("LITTLE_ENDIAN"),
- WithOption.WithUnsignedIntegerEncoding("unsigned-binary"),
- WithOption.WithSignedIntegerEncoding("twos-complement"),
- WithOption.WithFloatEncoding("IEEE754"),
Review Comment:
I'm a bit surprized this would work ... if I look in mspec, the type
PathSegment doesn't define the different encodings. So they should be null if
you remove them all. It should require setting the unsigned integer and the
string encodings. Also would you be changing the endianess from little-endian
to big-endian.
##########
plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/EipTcpConnection.java:
##########
@@ -93,7 +91,7 @@ public class EipTcpConnection extends
PollingSubscriptionConnectionBase<EIPConfi
private final NullAddressItem nullAddressItem = new NullAddressItem();
private final List<PathSegment> routingAddress = new ArrayList<>();
private short connectionPathSize = 0;
- private final int connectionSerialNumber =
ThreadLocalRandom.current().nextInt();
+ private final int connectionSerialNumber =
ThreadLocalRandom.current().nextInt(1, 0xFFFF);
Review Comment:
Great job spotting this ... indeed the field is defined as unsigned int 16
bit ... and for that 0xFFFF is indeed the max value.
##########
plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/EipTcpConnection.java:
##########
@@ -767,12 +765,7 @@ private static byte[] toAnsi(String tag) throws
BufferException {
segments.add(newSegment);
lengthBytes += newSegment.getLengthInBytes();
}
- WriteBufferByteBased buffer = new WriteBufferByteBased(new
byte[lengthBytes],
- WithByteBasedOption.WithByteOrder("LITTLE_ENDIAN"),
- WithOption.WithUnsignedIntegerEncoding("unsigned-binary"),
- WithOption.WithSignedIntegerEncoding("twos-complement"),
- WithOption.WithFloatEncoding("IEEE754"),
Review Comment:
If you look at EipTcpMessageCodec this should take care of creating the
write buffer with a call to "createWriteBuffer" ... this should also
automatically handle the case of a big-endian and little-endian device. So I
think your system might be a big-endian one and for that reason this part of
the code probably failed and removing the byte order should make it default to
big-endian.
##########
plc4j/drivers/eip/src/main/java/org/apache/plc4x/java/eip/base/EipTcpConnection.java:
##########
@@ -795,8 +788,7 @@ private PlcReadResponse decodeReadResponse(CipService p,
PlcReadRequest readRequ
List<CipService> arr = new ArrayList<>(nb);
try {
byte[] servicesData = responses.getServicesData();
- ReadBufferByteBased read = new
ReadBufferByteBased(servicesData,
- WithByteBasedOption.WithByteOrder("LITTLE_ENDIAN"));
+ ReadBufferByteBased read =
messageCodec.createReadBuffer(servicesData);
Review Comment:
As mentioned in the other change ... I assume the problem you were having
was that your system was a big-endian one and all of mine were little-endian
ones. This change looks a lot better than the one in lines 774++
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
