You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@plc4x.apache.org by cd...@apache.org on 2026/06/29 18:39:47 UTC
(plc4x) 02/02: fix: Addressed duplicate tsap informaiton on cotp and s7 level.
This is an automated email from the ASF dual-hosted git repository.
chrisdutz pushed a commit to branch develop
in repository https://gitbox.apache.org/repos/asf/plc4x.git
commit 06282c42a8ce44aec9bc4c70d6fe4bf1cd5174c5
Author: Christofer Dutz <ch...@c-ware.de>
AuthorDate: Mon Jun 29 20:39:36 2026 +0200
fix: Addressed duplicate tsap informaiton on cotp and s7 level.
---
.../org/apache/plc4x/java/s7/S7CotpConnection.java | 27 ++++++-
.../java/s7/configuration/S7Configuration.java | 69 +---------------
.../S7CotpTransportConfiguration.java | 59 +++++---------
.../plc4x/java/s7/S7UserDataCapabilityTest.java | 47 +++++++++++
.../java/s7/configuration/S7ConfigurationTest.java | 25 ------
.../S7CotpTransportConfigurationTest.java | 93 ++++++++++------------
.../plc4x/java/transport/cotp/CotpTransport.java | 4 +-
.../java/transport/cotp/CotpTransportInstance.java | 24 +++---
.../cotp/config/CotpTransportConfiguration.java | 48 ++++++++---
9 files changed, 192 insertions(+), 204 deletions(-)
diff --git a/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/S7CotpConnection.java b/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/S7CotpConnection.java
index f3a834d224..4b30f3c028 100644
--- a/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/S7CotpConnection.java
+++ b/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/S7CotpConnection.java
@@ -216,7 +216,16 @@ public class S7CotpConnection extends ConnectionBase<S7Configuration> {
}
private void probeUserDataCapability() {
- boolean userConfigured = driverContext.getControllerType() != ControllerType.ANY;
+ // If the user pinned the controller type, trust it and skip the SZL probe entirely
+ // (matches the pre-SPI3 driver, which only read SZL to auto-detect the type). The
+ // UserData-service capability (browse/alarms/cyclic) is then derived from the known type.
+ if (driverContext.getControllerType() != ControllerType.ANY) {
+ boolean supported = supportsUserDataServices(driverContext.getControllerType());
+ driverContext.setUserDataServicesSupported(supported);
+ LOGGER.info("Controller type pinned to {}; skipping SZL probe (userdata-services={})",
+ driverContext.getControllerType(), supported);
+ return;
+ }
// Try the modern COMPONENT_IDENTIFICATION SZL first — works on S7-1200/1500 and is
// tolerated by S7-300/400. Fall back to the legacy MODULE_IDENTIFICATION SZL if the
// first attempt errors out or doesn't yield a recognisable article number. Either
@@ -227,9 +236,7 @@ public class S7CotpConnection extends ConnectionBase<S7Configuration> {
}
if (result != null) {
driverContext.setArticleNumber(result.articleNumber());
- if (!userConfigured) {
- driverContext.setControllerType(result.controllerType());
- }
+ driverContext.setControllerType(result.controllerType());
driverContext.setUserDataServicesSupported(true);
LOGGER.info("SZL probe ok: article='{}', controllerType={}",
result.articleNumber(), driverContext.getControllerType());
@@ -242,6 +249,18 @@ public class S7CotpConnection extends ConnectionBase<S7Configuration> {
}
}
+ /**
+ * @return whether the given (explicitly configured) controller type speaks the S7Comm
+ * UserData services that back browse, alarm and cyclic subscriptions. Used when the SZL
+ * probe is skipped because the user pinned the controller type.
+ */
+ static boolean supportsUserDataServices(ControllerType type) {
+ return switch (type) {
+ case S7_300, S7_400, S7_1200, S7_1500 -> true;
+ default -> false; // S7_200, LOGO, ANY
+ };
+ }
+
private S7SzlService.ProbeResult trySzlProbe(SzlId szlId, int szlIndex) {
try {
S7Message request = S7SzlService.buildRequest(getTpduId(), szlId, szlIndex);
diff --git a/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/configuration/S7Configuration.java b/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/configuration/S7Configuration.java
index a012cebf76..13c6cb9597 100644
--- a/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/configuration/S7Configuration.java
+++ b/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/configuration/S7Configuration.java
@@ -19,7 +19,6 @@
package org.apache.plc4x.java.s7.configuration;
import org.apache.plc4x.java.s7.readwrite.ControllerType;
-import org.apache.plc4x.java.s7.readwrite.DeviceGroup;
import org.apache.plc4x.java.spi.config.Configuration;
import org.apache.plc4x.java.spi.config.annotations.ConfigurationParameter;
import org.apache.plc4x.java.spi.config.annotations.Description;
@@ -28,47 +27,9 @@ import org.apache.plc4x.java.spi.config.annotations.defaults.StringDefaultValue;
public class S7Configuration implements Configuration {
- @ConfigurationParameter("local-rack")
- @IntDefaultValue(1)
- @Description("Rack value for the client (PLC4X device).")
- protected int localRack = 1;
-
- @ConfigurationParameter("local-slot")
- @IntDefaultValue(1)
- @Description("Slot value for the client (PLC4X device).")
- protected int localSlot = 1;
-
- @ConfigurationParameter("local-device-group")
- @StringDefaultValue("PG_OR_PC")
- @Description("Local Device Group. PG_OR_PC requests programming-device privileges from the PLC, "
- + "which is required for block introspection (browse) on most CPUs. Override to OS or OTHERS "
- + "if your CPU has no PG slot free.")
- protected DeviceGroup localDeviceGroup = DeviceGroup.PG_OR_PC;
-
- @ConfigurationParameter("local-tsap")
- @IntDefaultValue(0)
- @Description("Local Transport Service Access Point. Overrides local-rack/local-slot/local-device-group when non-zero.")
- protected int localTsap = 0;
-
- @ConfigurationParameter("remote-rack")
- @IntDefaultValue(0)
- @Description("Rack value for the remote main CPU (PLC).")
- protected int remoteRack = 0;
-
- @ConfigurationParameter("remote-slot")
- @IntDefaultValue(0)
- @Description("Slot value for the remote main CPU (PLC).")
- protected int remoteSlot = 0;
-
- @ConfigurationParameter("remote-device-group")
- @StringDefaultValue("PG_OR_PC")
- @Description("Remote Device Group.")
- protected DeviceGroup remoteDeviceGroup = DeviceGroup.PG_OR_PC;
-
- @ConfigurationParameter("remote-tsap")
- @IntDefaultValue(0)
- @Description("Remote Transport Service Access Point. Overrides remote-rack/remote-slot/remote-device-group when non-zero.")
- protected int remoteTsap = 0;
+ // Note: the COTP addressing parameters (local/remote rack, slot, device-group and the
+ // local-tsap/remote-tsap overrides) are owned by S7CotpTransportConfiguration, which is the
+ // configuration the COTP transport actually consumes to build the connection request.
@ConfigurationParameter("pdu-size")
@IntDefaultValue(1024)
@@ -111,30 +72,6 @@ public class S7Configuration implements Configuration {
+ "faster but risk swapping on transient slow responses. Default 2000 (2s).")
protected int haFailoverTimeout = 2000;
- public int getLocalRack() { return localRack; }
- public void setLocalRack(int v) { this.localRack = v; }
-
- public int getLocalSlot() { return localSlot; }
- public void setLocalSlot(int v) { this.localSlot = v; }
-
- public DeviceGroup getLocalDeviceGroup() { return localDeviceGroup; }
- public void setLocalDeviceGroup(DeviceGroup v) { this.localDeviceGroup = v; }
-
- public int getLocalTsap() { return localTsap; }
- public void setLocalTsap(int v) { this.localTsap = v; }
-
- public int getRemoteRack() { return remoteRack; }
- public void setRemoteRack(int v) { this.remoteRack = v; }
-
- public int getRemoteSlot() { return remoteSlot; }
- public void setRemoteSlot(int v) { this.remoteSlot = v; }
-
- public DeviceGroup getRemoteDeviceGroup() { return remoteDeviceGroup; }
- public void setRemoteDeviceGroup(DeviceGroup v) { this.remoteDeviceGroup = v; }
-
- public int getRemoteTsap() { return remoteTsap; }
- public void setRemoteTsap(int v) { this.remoteTsap = v; }
-
public int getPduSize() { return pduSize; }
public void setPduSize(int v) { this.pduSize = v; }
diff --git a/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/configuration/S7CotpTransportConfiguration.java b/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/configuration/S7CotpTransportConfiguration.java
index 8ec9ff5c67..4bb5357207 100644
--- a/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/configuration/S7CotpTransportConfiguration.java
+++ b/plc4j/drivers/s7/src/main/java/org/apache/plc4x/java/s7/configuration/S7CotpTransportConfiguration.java
@@ -29,9 +29,15 @@ import org.apache.plc4x.java.transport.cotp.config.CotpTransportConfiguration;
/**
* COTP transport configuration for the S7 driver. Pins the ISO-on-TCP port (102) and derives the
- * COTP local/remote TSAPs from the S7 rack/slot/device-group parameters when these are provided
- * (the {@code local-tsap}/{@code remote-tsap} parameters from the parent {@link CotpTransportConfiguration}
- * still take precedence when set explicitly to a non-zero value).
+ * COTP local/remote TSAPs from the S7 rack/slot/device-group parameters. The
+ * {@code local-tsap}/{@code remote-tsap} parameters inherited from the parent
+ * {@link CotpTransportConfiguration} take precedence when set explicitly to a non-zero value.
+ *
+ * <p>The TSAP is always a derived value - it is never stored. {@link #getLocalTsap()} and
+ * {@link #getRemoteTsap()} compute it on demand from the rack/slot/device-group parameters
+ * (or return the explicit override). This avoids any drift between the TSAP and the
+ * rack/slot inputs it is derived from, which is important because the SPI configuration
+ * parser injects field values directly via reflection and never invokes setters.
*/
public class S7CotpTransportConfiguration extends CotpTransportConfiguration {
@@ -48,9 +54,9 @@ public class S7CotpTransportConfiguration extends CotpTransportConfiguration {
protected int localSlot = 1;
@ConfigurationParameter("local-device-group")
- @StringDefaultValue("PG_OR_PC")
+ @StringDefaultValue("OTHERS")
@Description("Local Device Group.")
- protected DeviceGroup localDeviceGroup = DeviceGroup.PG_OR_PC;
+ protected DeviceGroup localDeviceGroup = DeviceGroup.OTHERS;
@ConfigurationParameter("remote-rack")
@IntDefaultValue(0)
@@ -67,33 +73,24 @@ public class S7CotpTransportConfiguration extends CotpTransportConfiguration {
@Description("Remote Device Group.")
protected DeviceGroup remoteDeviceGroup = DeviceGroup.PG_OR_PC;
- public S7CotpTransportConfiguration() {
- super();
- // Default TSAPs: derive from rack/slot/device-group right away. The parser will overwrite
- // these via setLocalTsap/setRemoteTsap (or via the rack/slot setters below) if the user
- // supplied explicit values.
- this.localTsap = S7TsapIdEncoder.encodeS7TsapId(DeviceGroup.PG_OR_PC, 1, 1) & 0xFFFF;
- this.remoteTsap = S7TsapIdEncoder.encodeS7TsapId(DeviceGroup.PG_OR_PC, 0, 0) & 0xFFFF;
- }
-
@Override
public int getDefaultPort() {
return ISO_ON_TCP_PORT;
}
- private void recomputeTsaps() {
- this.localTsap = S7TsapIdEncoder.encodeS7TsapId(
- localDeviceGroup, localRack, localSlot) & 0xFFFF;
- this.remoteTsap = S7TsapIdEncoder.encodeS7TsapId(
- remoteDeviceGroup, remoteRack, remoteSlot) & 0xFFFF;
+ @Override
+ public int getLocalTsap() {
+ return localTsap != 0
+ ? localTsap
+ : S7TsapIdEncoder.encodeS7TsapId(localDeviceGroup, localRack, localSlot) & 0xFFFF;
}
- public void setLocalRack(int v) { this.localRack = v; recomputeTsaps(); }
- public void setLocalSlot(int v) { this.localSlot = v; recomputeTsaps(); }
- public void setLocalDeviceGroup(DeviceGroup v) { this.localDeviceGroup = v; recomputeTsaps(); }
- public void setRemoteRack(int v) { this.remoteRack = v; recomputeTsaps(); }
- public void setRemoteSlot(int v) { this.remoteSlot = v; recomputeTsaps(); }
- public void setRemoteDeviceGroup(DeviceGroup v) { this.remoteDeviceGroup = v; recomputeTsaps(); }
+ @Override
+ public int getRemoteTsap() {
+ return remoteTsap != 0
+ ? remoteTsap
+ : S7TsapIdEncoder.encodeS7TsapId(remoteDeviceGroup, remoteRack, remoteSlot) & 0xFFFF;
+ }
public int getLocalRack() { return localRack; }
public int getLocalSlot() { return localSlot; }
@@ -102,16 +99,4 @@ public class S7CotpTransportConfiguration extends CotpTransportConfiguration {
public int getRemoteSlot() { return remoteSlot; }
public DeviceGroup getRemoteDeviceGroup() { return remoteDeviceGroup; }
- public void setLocalTsap(int localTsap) {
- if (localTsap != 0) {
- this.localTsap = localTsap;
- }
- }
-
- public void setRemoteTsap(int remoteTsap) {
- if (remoteTsap != 0) {
- this.remoteTsap = remoteTsap;
- }
- }
-
}
diff --git a/plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/S7UserDataCapabilityTest.java b/plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/S7UserDataCapabilityTest.java
new file mode 100644
index 0000000000..4a07c12b08
--- /dev/null
+++ b/plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/S7UserDataCapabilityTest.java
@@ -0,0 +1,47 @@
+/*
+ * 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.s7;
+
+import org.apache.plc4x.java.s7.readwrite.ControllerType;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.EnumSource;
+
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+/**
+ * Covers the controller-type → UserData-service capability mapping used when the user pins the
+ * controller type and the connect-time SZL probe is therefore skipped (see
+ * {@link S7CotpConnection#probeUserDataCapability()}, restored to the pre-SPI3 behaviour).
+ */
+class S7UserDataCapabilityTest {
+
+ @ParameterizedTest
+ @EnumSource(value = ControllerType.class, names = {"S7_300", "S7_400", "S7_1200", "S7_1500"})
+ void realCpusSpeakUserDataServices(ControllerType type) {
+ assertTrue(S7CotpConnection.supportsUserDataServices(type));
+ }
+
+ @ParameterizedTest
+ @EnumSource(value = ControllerType.class, names = {"S7_200", "LOGO", "ANY"})
+ void lowEndAndUnknownDevicesDoNot(ControllerType type) {
+ assertFalse(S7CotpConnection.supportsUserDataServices(type));
+ }
+}
diff --git a/plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/configuration/S7ConfigurationTest.java b/plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/configuration/S7ConfigurationTest.java
index 5f10b3ea07..3ffbcdc08e 100644
--- a/plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/configuration/S7ConfigurationTest.java
+++ b/plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/configuration/S7ConfigurationTest.java
@@ -19,7 +19,6 @@
package org.apache.plc4x.java.s7.configuration;
import org.apache.plc4x.java.s7.readwrite.ControllerType;
-import org.apache.plc4x.java.s7.readwrite.DeviceGroup;
import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.*;
@@ -29,14 +28,6 @@ class S7ConfigurationTest {
@Test
void defaultsMatchAnnotations() {
S7Configuration cfg = new S7Configuration();
- assertEquals(1, cfg.getLocalRack());
- assertEquals(1, cfg.getLocalSlot());
- assertEquals(DeviceGroup.PG_OR_PC, cfg.getLocalDeviceGroup());
- assertEquals(0, cfg.getLocalTsap());
- assertEquals(0, cfg.getRemoteRack());
- assertEquals(0, cfg.getRemoteSlot());
- assertEquals(DeviceGroup.PG_OR_PC, cfg.getRemoteDeviceGroup());
- assertEquals(0, cfg.getRemoteTsap());
assertEquals(1024, cfg.getPduSize());
assertEquals(8, cfg.getMaxAmqCaller());
assertEquals(8, cfg.getMaxAmqCallee());
@@ -49,14 +40,6 @@ class S7ConfigurationTest {
@Test
void allSettersRoundTrip() {
S7Configuration cfg = new S7Configuration();
- cfg.setLocalRack(2);
- cfg.setLocalSlot(3);
- cfg.setLocalDeviceGroup(DeviceGroup.OS);
- cfg.setLocalTsap(0x4711);
- cfg.setRemoteRack(4);
- cfg.setRemoteSlot(5);
- cfg.setRemoteDeviceGroup(DeviceGroup.OTHERS);
- cfg.setRemoteTsap(0x1234);
cfg.setPduSize(480);
cfg.setMaxAmqCaller(2);
cfg.setMaxAmqCallee(2);
@@ -65,14 +48,6 @@ class S7ConfigurationTest {
cfg.setHaHeartbeatInterval(8000);
cfg.setHaFailoverTimeout(3000);
- assertEquals(2, cfg.getLocalRack());
- assertEquals(3, cfg.getLocalSlot());
- assertEquals(DeviceGroup.OS, cfg.getLocalDeviceGroup());
- assertEquals(0x4711, cfg.getLocalTsap());
- assertEquals(4, cfg.getRemoteRack());
- assertEquals(5, cfg.getRemoteSlot());
- assertEquals(DeviceGroup.OTHERS, cfg.getRemoteDeviceGroup());
- assertEquals(0x1234, cfg.getRemoteTsap());
assertEquals(480, cfg.getPduSize());
assertEquals(2, cfg.getMaxAmqCaller());
assertEquals(2, cfg.getMaxAmqCallee());
diff --git a/plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/configuration/S7CotpTransportConfigurationTest.java b/plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/configuration/S7CotpTransportConfigurationTest.java
index 774403472c..b8310bc4c3 100644
--- a/plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/configuration/S7CotpTransportConfigurationTest.java
+++ b/plc4j/drivers/s7/src/test/java/org/apache/plc4x/java/s7/configuration/S7CotpTransportConfigurationTest.java
@@ -24,80 +24,75 @@ import org.junit.jupiter.api.Test;
import static org.junit.jupiter.api.Assertions.*;
+/**
+ * These tests deliberately exercise the real configuration path - parsing a connection-string
+ * parameter map via {@link ConfigurationFactory}, which injects field values directly via
+ * reflection (it never calls setters). The previous version of these tests called setters
+ * directly and so never covered the path that actually runs in production; that is how
+ * issue #2620 (remote-slot silently dropped, connection landing on slot 0) slipped through.
+ */
class S7CotpTransportConfigurationTest {
+ private S7CotpTransportConfiguration parse(String params) {
+ return new ConfigurationFactory()
+ .createConfiguration(S7CotpTransportConfiguration.class, params);
+ }
+
@Test
void defaultPort() {
assertEquals(102, new S7CotpTransportConfiguration().getDefaultPort());
}
@Test
- void defaultsDeriveTsapsFromOthersAndPgOrPc() {
- S7CotpTransportConfiguration cfg = new S7CotpTransportConfiguration();
- // Constructor seeds: local = PG_OR_PC rack=1 slot=1; remote = PG_OR_PC rack=0 slot=0.
- assertNotEquals(0, cfg.localTsap);
- assertNotEquals(0, cfg.remoteTsap);
+ void defaultsDeriveLegacyTsaps() {
+ // No addressing parameters: local defaults to OTHERS rack=1/slot=1 -> 0x0311,
+ // remote defaults to PG_OR_PC rack=0/slot=0 -> 0x0100. These match the pre-SPI3 wire bytes.
+ S7CotpTransportConfiguration cfg = parse("");
+ assertEquals(0x0311, cfg.getLocalTsap());
+ assertEquals(0x0100, cfg.getRemoteTsap());
}
@Test
- void rackSlotSettersRecomputeTsap() {
- S7CotpTransportConfiguration cfg = new S7CotpTransportConfiguration();
- int initial = cfg.localTsap;
- cfg.setLocalRack(2);
- cfg.setLocalSlot(3);
- // Rack/slot change should produce a different TSAP than the initial.
- assertNotEquals(initial, cfg.localTsap);
- assertEquals(2, cfg.getLocalRack());
- assertEquals(3, cfg.getLocalSlot());
+ void remoteSlotIsHonoured() {
+ // Regression test for #2620: remote-slot=3 must reach the called TSAP as 0x0103.
+ S7CotpTransportConfiguration cfg = parse("remote-rack=0&remote-slot=3&controller-type=S7_400");
+ assertEquals(0x0103, cfg.getRemoteTsap());
}
@Test
- void deviceGroupSetterRecomputesTsap() {
- S7CotpTransportConfiguration cfg = new S7CotpTransportConfiguration();
- int initial = cfg.localTsap;
- cfg.setLocalDeviceGroup(DeviceGroup.OS);
- assertNotEquals(initial, cfg.localTsap);
- assertEquals(DeviceGroup.OS, cfg.getLocalDeviceGroup());
+ void remoteRackAndSlotAreEncoded() {
+ // (rack << 4) | slot in the low byte, PG_OR_PC (0x01) in the high byte.
+ S7CotpTransportConfiguration cfg = parse("remote-rack=2&remote-slot=5");
+ assertEquals(0x0125, cfg.getRemoteTsap());
}
@Test
- void invalidDeviceGroupInConnectionStringFailsLoudly() {
- // Now that the field is enum-typed, ConfigurationFactory rejects unknown values at
- // parse time instead of silently keeping the default — what we want.
- assertThrows(IllegalArgumentException.class, () -> new ConfigurationFactory()
- .createConfiguration(S7CotpTransportConfiguration.class, "local-device-group=NOT_A_GROUP"));
+ void localRackSlotAndDeviceGroupAreEncoded() {
+ S7CotpTransportConfiguration cfg = parse("local-rack=2&local-slot=3&local-device-group=OS");
+ assertEquals(DeviceGroup.OS, cfg.getLocalDeviceGroup());
+ // OS (0x02) high byte, (rack=2 << 4) | slot=3 = 0x23 low byte.
+ assertEquals(0x0223, cfg.getLocalTsap());
}
@Test
- void remoteSettersWork() {
- S7CotpTransportConfiguration cfg = new S7CotpTransportConfiguration();
- int initial = cfg.remoteTsap;
- cfg.setRemoteRack(1);
- cfg.setRemoteSlot(2);
- cfg.setRemoteDeviceGroup(DeviceGroup.OTHERS);
- assertNotEquals(initial, cfg.remoteTsap);
- assertEquals(1, cfg.getRemoteRack());
- assertEquals(2, cfg.getRemoteSlot());
- assertEquals(DeviceGroup.OTHERS, cfg.getRemoteDeviceGroup());
+ void explicitTsapOverridesDerivedValue() {
+ S7CotpTransportConfiguration cfg = parse("remote-slot=3&remote-tsap=4660&local-tsap=18193");
+ // Explicit overrides win even though rack/slot would derive something else.
+ assertEquals(0x1234, cfg.getRemoteTsap());
+ assertEquals(0x4711, cfg.getLocalTsap());
}
@Test
- void explicitTsapOverridesDerived() {
- S7CotpTransportConfiguration cfg = new S7CotpTransportConfiguration();
- cfg.setLocalTsap(0x4711);
- cfg.setRemoteTsap(0x1234);
- assertEquals(0x4711, cfg.localTsap);
- assertEquals(0x1234, cfg.remoteTsap);
+ void zeroTsapKeepsDerivedValue() {
+ // The 0 sentinel means "not set" - the rack/slot-derived value must be used.
+ S7CotpTransportConfiguration cfg = parse("remote-slot=3&remote-tsap=0&local-tsap=0");
+ assertEquals(0x0103, cfg.getRemoteTsap());
+ assertEquals(0x0311, cfg.getLocalTsap());
}
@Test
- void zeroTsapIsIgnoredKeepingDerivedValue() {
- S7CotpTransportConfiguration cfg = new S7CotpTransportConfiguration();
- int derivedLocal = cfg.localTsap;
- int derivedRemote = cfg.remoteTsap;
- cfg.setLocalTsap(0);
- cfg.setRemoteTsap(0);
- assertEquals(derivedLocal, cfg.localTsap);
- assertEquals(derivedRemote, cfg.remoteTsap);
+ void invalidDeviceGroupInConnectionStringFailsLoudly() {
+ assertThrows(IllegalArgumentException.class,
+ () -> parse("local-device-group=NOT_A_GROUP"));
}
}
diff --git a/plc4j/transports/cotp/src/main/java/org/apache/plc4x/java/transport/cotp/CotpTransport.java b/plc4j/transports/cotp/src/main/java/org/apache/plc4x/java/transport/cotp/CotpTransport.java
index 7ac85fdc71..5a9a248479 100644
--- a/plc4j/transports/cotp/src/main/java/org/apache/plc4x/java/transport/cotp/CotpTransport.java
+++ b/plc4j/transports/cotp/src/main/java/org/apache/plc4x/java/transport/cotp/CotpTransport.java
@@ -103,8 +103,8 @@ public class CotpTransport implements Transport<CotpTransportConfiguration> {
LOGGER.debug("Creating COTP transport instance for {}:{}", host, port);
LOGGER.debug("COTP Configuration: localTsap=0x{}, remoteTsap=0x{}, tpduSize={}",
- Integer.toHexString(cotpConfig.localTsap),
- Integer.toHexString(cotpConfig.remoteTsap),
+ Integer.toHexString(cotpConfig.getLocalTsap()),
+ Integer.toHexString(cotpConfig.getRemoteTsap()),
cotpConfig.cotpTpduSize);
return new CotpTransportInstance(host, port, cotpConfig, auditLog);
diff --git a/plc4j/transports/cotp/src/main/java/org/apache/plc4x/java/transport/cotp/CotpTransportInstance.java b/plc4j/transports/cotp/src/main/java/org/apache/plc4x/java/transport/cotp/CotpTransportInstance.java
index 069c4fbea3..ddea20e829 100644
--- a/plc4j/transports/cotp/src/main/java/org/apache/plc4x/java/transport/cotp/CotpTransportInstance.java
+++ b/plc4j/transports/cotp/src/main/java/org/apache/plc4x/java/transport/cotp/CotpTransportInstance.java
@@ -132,8 +132,8 @@ public class CotpTransportInstance extends BaseTransportInstance<CotpTransportCo
// Build Connection Request using generated classes
COTPPacketConnectionRequest connectionRequest = buildConnectionRequest();
LOGGER.info("COTP CR: localTsap=0x{}, remoteTsap=0x{}",
- Integer.toHexString(getConfiguration().localTsap),
- Integer.toHexString(getConfiguration().remoteTsap));
+ Integer.toHexString(getConfiguration().getLocalTsap()),
+ Integer.toHexString(getConfiguration().getRemoteTsap()));
TPKTPacket tpktRequest = new TPKTPacket(connectionRequest);
@@ -180,7 +180,7 @@ public class CotpTransportInstance extends BaseTransportInstance<CotpTransportCo
connected = true;
getAuditLog().write(AuditLogEventType.CONNECT, String.format(
"COTP connection established (localTsap=0x%04X, remoteTsap=0x%04X, tpduSize=%d)",
- getConfiguration().localTsap, getConfiguration().remoteTsap, getConfiguration().cotpTpduSize));
+ getConfiguration().getLocalTsap(), getConfiguration().getRemoteTsap(), getConfiguration().cotpTpduSize));
return;
} else if (cotpPacket instanceof COTPPacketTpduError) {
throw new TransportException("COTP connection rejected by remote");
@@ -193,8 +193,8 @@ public class CotpTransportInstance extends BaseTransportInstance<CotpTransportCo
"COTP connection timeout - no response from remote after %dms. " +
"TSAP: 0x%04X → 0x%04X, TPDU size: %d bytes",
getConfiguration().cotpConnectionTimeout,
- getConfiguration().localTsap,
- getConfiguration().remoteTsap,
+ getConfiguration().getLocalTsap(),
+ getConfiguration().getRemoteTsap(),
getConfiguration().cotpTpduSize
);
throw new TransportException(errorMsg);
@@ -204,8 +204,8 @@ public class CotpTransportInstance extends BaseTransportInstance<CotpTransportCo
"COTP connection failed (TSAP: 0x%04X → 0x%04X, TPDU: %d bytes). " +
"Check if PLC is reachable and configured to accept connections. " +
"Error: %s",
- getConfiguration().localTsap,
- getConfiguration().remoteTsap,
+ getConfiguration().getLocalTsap(),
+ getConfiguration().getRemoteTsap(),
getConfiguration().cotpTpduSize,
e.getMessage()
);
@@ -221,16 +221,18 @@ public class CotpTransportInstance extends BaseTransportInstance<CotpTransportCo
List<COTPParameter> parameters = new ArrayList<>();
// Source TSAP parameter
+ int localTsap = getConfiguration().getLocalTsap();
byte[] srcTsapBytes = new byte[]{
- (byte) ((getConfiguration().localTsap >> 8) & 0xFF),
- (byte) (getConfiguration().localTsap & 0xFF)
+ (byte) ((localTsap >> 8) & 0xFF),
+ (byte) (localTsap & 0xFF)
};
parameters.add(new COTPParameterCallingTsap(srcTsapBytes));
// Destination TSAP parameter
+ int remoteTsap = getConfiguration().getRemoteTsap();
byte[] dstTsapBytes = new byte[]{
- (byte) ((getConfiguration().remoteTsap >> 8) & 0xFF),
- (byte) (getConfiguration().remoteTsap & 0xFF)
+ (byte) ((remoteTsap >> 8) & 0xFF),
+ (byte) (remoteTsap & 0xFF)
};
parameters.add(new COTPParameterCalledTsap(dstTsapBytes));
diff --git a/plc4j/transports/cotp/src/main/java/org/apache/plc4x/java/transport/cotp/config/CotpTransportConfiguration.java b/plc4j/transports/cotp/src/main/java/org/apache/plc4x/java/transport/cotp/config/CotpTransportConfiguration.java
index 936414ae2a..38acab0028 100644
--- a/plc4j/transports/cotp/src/main/java/org/apache/plc4x/java/transport/cotp/config/CotpTransportConfiguration.java
+++ b/plc4j/transports/cotp/src/main/java/org/apache/plc4x/java/transport/cotp/config/CotpTransportConfiguration.java
@@ -33,22 +33,34 @@ import org.apache.plc4x.java.transport.tcp.config.TcpTransportConfiguration;
public class CotpTransportConfiguration extends TcpTransportConfiguration implements TransportConfiguration {
/**
- * Local TSAP (Transport Service Access Point) identifier.
- * Used in COTP connection request. Default is 0x0100.
+ * Default local TSAP used when no explicit {@code local-tsap} is configured.
+ */
+ public static final int DEFAULT_LOCAL_TSAP = 0x0311;
+
+ /**
+ * Default remote TSAP used when no explicit {@code remote-tsap} is configured.
+ */
+ public static final int DEFAULT_REMOTE_TSAP = 0x0100;
+
+ /**
+ * Raw local TSAP (Transport Service Access Point) override. A value of {@code 0} means
+ * "not set". Always read the effective value via {@link #getLocalTsap()} rather than this
+ * field directly: subclasses may derive the TSAP from other parameters (e.g. rack/slot).
*/
@ConfigurationParameter("local-tsap")
@Description("Local TSAP (Transport Service Access Point) identifier.")
- @IntDefaultValue(0x0311)
- public int localTsap = 0x0311;
+ @IntDefaultValue(0)
+ public int localTsap = 0;
/**
- * Remote TSAP (Transport Service Access Point) identifier.
- * Used in COTP connection request. Default is 0x0102.
+ * Raw remote TSAP (Transport Service Access Point) override. A value of {@code 0} means
+ * "not set". Always read the effective value via {@link #getRemoteTsap()} rather than this
+ * field directly: subclasses may derive the TSAP from other parameters (e.g. rack/slot).
*/
@ConfigurationParameter("remote-tsap")
@Description("Remote TSAP (Transport Service Access Point) identifier.")
- @IntDefaultValue(0x0100)
- public int remoteTsap = 0x0100;
+ @IntDefaultValue(0)
+ public int remoteTsap = 0;
/**
* COTP PDU size for data transmission.
@@ -84,11 +96,27 @@ public class CotpTransportConfiguration extends TcpTransportConfiguration implem
// COTP typically uses default TCP-settings but you can override
}
+ /**
+ * @return the effective local TSAP: the explicit {@code local-tsap} override when set
+ * (non-zero), otherwise {@link #DEFAULT_LOCAL_TSAP}. Subclasses may override to derive it.
+ */
+ public int getLocalTsap() {
+ return localTsap != 0 ? localTsap : DEFAULT_LOCAL_TSAP;
+ }
+
+ /**
+ * @return the effective remote TSAP: the explicit {@code remote-tsap} override when set
+ * (non-zero), otherwise {@link #DEFAULT_REMOTE_TSAP}. Subclasses may override to derive it.
+ */
+ public int getRemoteTsap() {
+ return remoteTsap != 0 ? remoteTsap : DEFAULT_REMOTE_TSAP;
+ }
+
@Override
public String toString() {
return "CotpTransportConfiguration{" +
- "localTsap=0x" + Integer.toHexString(localTsap) +
- ", remoteTsap=0x" + Integer.toHexString(remoteTsap) +
+ "localTsap=0x" + Integer.toHexString(getLocalTsap()) +
+ ", remoteTsap=0x" + Integer.toHexString(getRemoteTsap()) +
", cotpTpduSize=" + cotpTpduSize +
", cotpConnectionTimeout=" + cotpConnectionTimeout +
", protocolClass=" + protocolClass +