Copilot commented on code in PR #2651:
URL: https://github.com/apache/plc4x/pull/2651#discussion_r3620751974
##########
plc4j/drivers/slmp/src/main/java/org/apache/plc4x/java/slmp/SlmpDataType.java:
##########
@@ -90,6 +93,64 @@ public PlcValue decode(byte[] responseData, int quantity) {
}
}
+ /**
+ * Encode {@code quantity} elements of this type to little-endian SLMP
request bytes
+ * ({@code 2 * wordsPerElement} per element). Returns {@code null} on a
type/arity mismatch
+ * (caller maps to INVALID_DATA), symmetric with {@link #decode}.
+ */
+ public byte[] encode(PlcValue value, int quantity) {
+ int totalBytes = quantity * wordsPerElement * 2;
+ WriteBufferByteBased buffer = new WriteBufferByteBased(new
byte[totalBytes],
+ WithByteBasedOption.WithByteOrder("LITTLE_ENDIAN"),
+ WithOption.WithUnsignedIntegerEncoding("unsigned-binary"),
+ WithOption.WithSignedIntegerEncoding("twos-complement"),
+ WithOption.WithFloatEncoding("IEEE754"));
+ try {
+ if (quantity == 1) {
+ if (value == null || value.isList()) {
+ return null;
+ }
+ writeOne(buffer, value);
+ } else {
+ if (value == null || !value.isList() || value.getList().size()
!= quantity) {
+ return null;
+ }
+ for (PlcValue element : value.getList()) {
+ writeOne(buffer, element);
+ }
+ }
+ return buffer.getBytes();
+ } catch (BufferException | PlcRuntimeException e) {
+ LOGGER.warn("Failed to encode SLMP {} value", this, e);
+ return null;
+ }
Review Comment:
encode() is documented to return null on type/arity mismatch, but the
current catch logs WARN with a full stack trace for PlcRuntimeException
(including PlcIncompatibleDatatypeException). That makes expected user-input
errors noisy in logs and can look like an internal fault. Consider treating
PlcIncompatibleDatatypeException as a normal INVALID_DATA case (no WARN/stack
trace), and keep WARN for genuine buffer/encoding failures.
##########
plc4j/drivers/slmp/src/test/java/org/apache/plc4x/java/slmp/SlmpDriverTest.java:
##########
@@ -0,0 +1,40 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.plc4x.java.slmp;
+
+import org.junit.jupiter.api.Test;
+
+import java.lang.reflect.Method;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+class SlmpDriverTest {
+
+ private final SlmpDriver driver = new SlmpDriver();
+
+ @Test
+ void capabilities() throws Exception {
+ // Use reflection to access the protected can* methods.
+ for (String name : new String[]{"canRead", "canWrite"}) {
+ Method m = SlmpDriver.class.getDeclaredMethod(name);
+ m.setAccessible(true);
+ assertEquals(Boolean.TRUE, m.invoke(driver), name + " should
return true");
+ }
+ }
+}
Review Comment:
This test uses reflection to access canRead/canWrite, but the test class is
in the same package as SlmpDriver, so protected access already works. Avoiding
reflection makes the test simpler and less brittle (e.g., no setAccessible
warnings/failures under stricter module/access settings).
##########
plc4j/drivers/slmp/src/main/java/org/apache/plc4x/java/slmp/tag/SlmpTag.java:
##########
@@ -135,7 +135,7 @@ public static SlmpTag of(String addressString) {
long numberOfPoints = (long) quantity * dataType.getWordsPerElement();
if (numberOfPoints > MAX_POINTS) {
throw new PlcInvalidTagException("requested " + numberOfPoints + "
words exceeds the v0 single-frame "
- + "Batch Read ceiling of " + MAX_POINTS + " (no optimizer to
split): " + addressString);
+ + "batch Read/Write ceiling of " + MAX_POINTS + " (no
optimizer to split): " + addressString);
}
Review Comment:
The exception message uses mixed/odd capitalization "batch Read/Write". For
readability and consistency with the earlier "Batch Read" wording, consider
using "Batch Read/Write" (or all-lowercase "batch read/write").
--
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]
