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 <[email protected]> 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 +
