diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index dc197ee..6719fe5 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -6,7 +6,10 @@ on: jobs: build: - runs-on: ubuntu-latest + runs-on: ${{ matrix.os }} + strategy: + matrix: + os: [ubuntu-latest, macos-latest, windows-latest] steps: - uses: actions/checkout@v4 diff --git a/modbus-serial/src/main/java/com/digitalpetri/modbus/serial/client/SerialPortClientTransport.java b/modbus-serial/src/main/java/com/digitalpetri/modbus/serial/client/SerialPortClientTransport.java index a7b8c24..451de3d 100644 --- a/modbus-serial/src/main/java/com/digitalpetri/modbus/serial/client/SerialPortClientTransport.java +++ b/modbus-serial/src/main/java/com/digitalpetri/modbus/serial/client/SerialPortClientTransport.java @@ -5,6 +5,8 @@ import com.digitalpetri.modbus.ModbusRtuResponseFrameParser.Accumulated; import com.digitalpetri.modbus.ModbusRtuResponseFrameParser.ParserState; import com.digitalpetri.modbus.client.ModbusRtuClientTransport; +import com.digitalpetri.modbus.exceptions.ModbusConnectException; +import com.digitalpetri.modbus.exceptions.ModbusException; import com.digitalpetri.modbus.internal.util.ExecutionQueue; import com.digitalpetri.modbus.serial.SerialPortTransportConfig; import com.fazecast.jSerialComm.SerialPort; @@ -27,67 +29,97 @@ public class SerialPortClientTransport implements ModbusRtuClientTransport { private final ExecutionQueue executionQueue; - private final SerialPort serialPort; + private volatile SerialPort serialPort; private final SerialPortTransportConfig config; public SerialPortClientTransport(SerialPortTransportConfig config) { this.config = config; - serialPort = SerialPort.getCommPort(config.serialPort()); - - serialPort.setComPortParameters( - config.baudRate(), - config.dataBits(), - config.stopBits(), - config.parity(), - config.rs485Mode()); - executionQueue = new ExecutionQueue(config.executor()); } /** * Return the underlying {@link SerialPort} used by this transport. * + *

The serial port is lazily instantiated on first access. + * * @return the configured {@link SerialPort} instance. + * @throws ModbusException if the serial port could not be created. */ - public SerialPort getSerialPort() { - return serialPort; + public SerialPort getSerialPort() throws ModbusException { + SerialPort sp = this.serialPort; + if (sp == null) { + synchronized (this) { + sp = this.serialPort; + if (sp == null) { + try { + sp = SerialPort.getCommPort(config.serialPort()); + sp.setComPortParameters( + config.baudRate(), + config.dataBits(), + config.stopBits(), + config.parity(), + config.rs485Mode()); + this.serialPort = sp; + } catch (Exception e) { + throw new ModbusException( + "failed to get comm port '%s'".formatted(config.serialPort()), e); + } + } + } + } + return sp; } @Override public synchronized CompletableFuture connect() { - if (serialPort.isOpen()) { + SerialPort sp; + try { + sp = getSerialPort(); + } catch (ModbusException e) { + return CompletableFuture.failedFuture( + new ModbusConnectException(e.getMessage(), e)); + } + + if (sp.isOpen()) { return CompletableFuture.completedFuture(null); } else { - if (serialPort.openPort()) { + if (sp.openPort()) { frameParser.reset(); // note: no-op if already added from previous connect() - serialPort.addDataListener(new ModbusRtuDataListener()); + sp.addDataListener(new ModbusRtuDataListener()); return CompletableFuture.completedFuture(null); } else { return CompletableFuture.failedFuture( - new Exception( + new ModbusConnectException( "failed to open port '%s', lastErrorCode=%d" - .formatted(config.serialPort(), serialPort.getLastErrorCode()))); + .formatted(config.serialPort(), sp.getLastErrorCode()))); } } } + /** + * {@inheritDoc} + * + *

The returned {@link CompletionStage} may complete exceptionally with a {@link + * ModbusException} if the serial port could not be closed. + */ @Override public synchronized CompletableFuture disconnect() { - if (serialPort.isOpen()) { - if (serialPort.closePort()) { + SerialPort sp = this.serialPort; + if (sp != null && sp.isOpen()) { + if (sp.closePort()) { frameParser.reset(); return CompletableFuture.completedFuture(null); } else { return CompletableFuture.failedFuture( - new Exception( + new ModbusException( "failed to close port '%s', lastErrorCode=%d" - .formatted(config.serialPort(), serialPort.getLastErrorCode()))); + .formatted(config.serialPort(), sp.getLastErrorCode()))); } } else { return CompletableFuture.completedFuture(null); @@ -96,11 +128,23 @@ public synchronized CompletableFuture disconnect() { @Override public boolean isConnected() { - return serialPort.isOpen(); + SerialPort sp = this.serialPort; + return sp != null && sp.isOpen(); } + /** + * {@inheritDoc} + * + *

The returned {@link CompletionStage} may complete exceptionally with a {@link + * ModbusException} if the transport is not connected or if writing to the serial port fails. + */ @Override public CompletionStage send(ModbusRtuFrame frame) { + SerialPort sp = this.serialPort; + if (sp == null || !sp.isOpen()) { + return CompletableFuture.failedFuture(new ModbusException("not connected")); + } + ByteBuffer buffer = ByteBuffer.allocate(256); try { @@ -114,10 +158,10 @@ public CompletionStage send(ModbusRtuFrame frame) { int totalWritten = 0; while (totalWritten < data.length) { - int written = serialPort.writeBytes(data, data.length - totalWritten, totalWritten); + int written = sp.writeBytes(data, data.length - totalWritten, totalWritten); if (written == -1) { - int errorCode = serialPort.getLastErrorCode(); - throw new Exception( + int errorCode = sp.getLastErrorCode(); + throw new ModbusException( "failed to write to port '%s', lastErrorCode=%d" .formatted(config.serialPort(), errorCode)); } diff --git a/modbus-serial/src/main/java/com/digitalpetri/modbus/serial/server/SerialPortServerTransport.java b/modbus-serial/src/main/java/com/digitalpetri/modbus/serial/server/SerialPortServerTransport.java index b882035..eb5a36c 100644 --- a/modbus-serial/src/main/java/com/digitalpetri/modbus/serial/server/SerialPortServerTransport.java +++ b/modbus-serial/src/main/java/com/digitalpetri/modbus/serial/server/SerialPortServerTransport.java @@ -4,6 +4,8 @@ import com.digitalpetri.modbus.ModbusRtuRequestFrameParser; import com.digitalpetri.modbus.ModbusRtuRequestFrameParser.Accumulated; import com.digitalpetri.modbus.ModbusRtuRequestFrameParser.ParserState; +import com.digitalpetri.modbus.exceptions.ModbusConnectException; +import com.digitalpetri.modbus.exceptions.ModbusException; import com.digitalpetri.modbus.exceptions.UnknownUnitIdException; import com.digitalpetri.modbus.internal.util.ExecutionQueue; import com.digitalpetri.modbus.serial.SerialPortTransportConfig; @@ -35,66 +37,95 @@ public class SerialPortServerTransport implements ModbusRtuServerTransport { private final ExecutionQueue executionQueue; - private final SerialPort serialPort; + private volatile SerialPort serialPort; private final SerialPortTransportConfig config; public SerialPortServerTransport(SerialPortTransportConfig config) { this.config = config; - serialPort = SerialPort.getCommPort(config.serialPort()); - - serialPort.setComPortParameters( - config.baudRate(), - config.dataBits(), - config.stopBits(), - config.parity(), - config.rs485Mode()); - executionQueue = new ExecutionQueue(config.executor()); } /** * Return the underlying {@link SerialPort} used by this transport. * + *

The serial port is lazily instantiated on first access. + * * @return the configured {@link SerialPort} instance. + * @throws ModbusException if the serial port could not be created. */ - public SerialPort getSerialPort() { - return serialPort; + public SerialPort getSerialPort() throws ModbusException { + SerialPort sp = this.serialPort; + if (sp == null) { + synchronized (this) { + sp = this.serialPort; + if (sp == null) { + try { + sp = SerialPort.getCommPort(config.serialPort()); + sp.setComPortParameters( + config.baudRate(), + config.dataBits(), + config.stopBits(), + config.parity(), + config.rs485Mode()); + this.serialPort = sp; + } catch (Exception e) { + throw new ModbusException( + "failed to get comm port '%s'".formatted(config.serialPort()), e); + } + } + } + } + return sp; } @Override public CompletionStage bind() { - if (serialPort.isOpen()) { + SerialPort sp; + try { + sp = getSerialPort(); + } catch (ModbusException e) { + return CompletableFuture.failedFuture(new ModbusConnectException(e)); + } + + if (sp.isOpen()) { return CompletableFuture.completedFuture(null); } else { - if (serialPort.openPort()) { + if (sp.openPort()) { frameParser.reset(); - serialPort.addDataListener(new ModbusRtuDataListener()); + sp.addDataListener(new ModbusRtuDataListener()); return CompletableFuture.completedFuture(null); } else { return CompletableFuture.failedFuture( - new Exception( + new ModbusConnectException( "failed to open port '%s', lastErrorCode=%d" - .formatted(config.serialPort(), serialPort.getLastErrorCode()))); + .formatted(config.serialPort(), sp.getLastErrorCode()))); } } } + /** + * {@inheritDoc} + * + *

The returned {@link CompletionStage} may complete exceptionally with a {@link + * ModbusException} if the serial port could not be closed. + */ @Override public CompletionStage unbind() { - if (serialPort.isOpen()) { - if (serialPort.closePort()) { + SerialPort sp = this.serialPort; + if (sp != null && sp.isOpen()) { + if (sp.closePort()) { frameParser.reset(); return CompletableFuture.completedFuture(null); } else { return CompletableFuture.failedFuture( - new Exception( + new ModbusException( "failed to close port '%s', lastErrorCode=%d" - .formatted(config.serialPort(), serialPort.getLastErrorCode()))); + .formatted(config.serialPort(), sp.getLastErrorCode()))); } } else { return CompletableFuture.completedFuture(null); @@ -157,10 +188,10 @@ private void onFrameReceived(ModbusRtuFrame requestFrame) { pdu.get(data, 1, pdu.remaining()); crc.get(data, data.length - 2, crc.remaining()); + SerialPort sp = SerialPortServerTransport.this.serialPort; int totalWritten = 0; while (totalWritten < data.length) { - int written = - serialPort.writeBytes(data, data.length - totalWritten, totalWritten); + int written = sp.writeBytes(data, data.length - totalWritten, totalWritten); if (written == -1) { logger.error("Error writing frame to serial port"); diff --git a/modbus-serial/src/test/java/com/digitalpetri/modbus/serial/SerialPortGetCommPortBehaviorTest.java b/modbus-serial/src/test/java/com/digitalpetri/modbus/serial/SerialPortGetCommPortBehaviorTest.java new file mode 100644 index 0000000..972b108 --- /dev/null +++ b/modbus-serial/src/test/java/com/digitalpetri/modbus/serial/SerialPortGetCommPortBehaviorTest.java @@ -0,0 +1,194 @@ +package com.digitalpetri.modbus.serial; + +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import com.digitalpetri.modbus.exceptions.ModbusConnectException; +import com.digitalpetri.modbus.exceptions.ModbusException; +import com.digitalpetri.modbus.serial.client.SerialPortClientTransport; +import com.digitalpetri.modbus.serial.server.SerialPortServerTransport; +import com.fazecast.jSerialComm.SerialPort; +import com.fazecast.jSerialComm.SerialPortInvalidPortException; +import java.util.concurrent.CompletableFuture; +import java.util.concurrent.ExecutionException; +import org.junit.jupiter.api.Nested; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.condition.EnabledOnOs; +import org.junit.jupiter.api.condition.OS; + +/** + * Tests documenting the behavior of {@link SerialPort#getCommPort(String)} with non-existent ports + * and how the serial transport classes handle it. + * + *

The behavior of {@code getCommPort} differs by platform: + * + *

+ * + *

This behavioral difference is why the transport classes defer the {@code getCommPort} call to + * connect/bind time rather than calling it in the constructor. On Linux/macOS the deferred call + * allows the exception to be caught and wrapped as a {@link ModbusConnectException}. On Windows the + * constructor would have succeeded either way, but the deferred approach keeps the behavior + * consistent across platforms. + */ +class SerialPortGetCommPortBehaviorTest { + + private static final String BOGUS_UNIX_PORT = "/dev/this_port_does_not_exist_xyz"; + private static final String BOGUS_WINDOWS_PORT = "COM999"; + + @Nested + @EnabledOnOs({OS.LINUX, OS.MAC}) + class GetCommPortOnLinuxMac { + + @Test + void getCommPortThrowsForNonExistentPortFile() { + // On Linux/Mac, getCommPort checks File.exists() for the port descriptor. + // A path that doesn't exist on the filesystem causes an immediate exception. + assertThrows(SerialPortInvalidPortException.class, () -> SerialPort.getCommPort(BOGUS_UNIX_PORT)); + } + + @Test + void getCommPortThrowsForWindowsStyleDescriptor() { + // A Windows-style descriptor like "COM999" also fails on Linux/Mac because + // it doesn't exist on the filesystem (not under /dev/ either). + assertThrows(SerialPortInvalidPortException.class, () -> SerialPort.getCommPort(BOGUS_WINDOWS_PORT)); + } + } + + @Nested + @EnabledOnOs(OS.WINDOWS) + class GetCommPortOnWindows { + + @Test + void getCommPortDoesNotThrowForNonExistentComPort() { + // On Windows, getCommPort rewrites the descriptor to \\.\COMx format + // and does not validate that the port exists. The SerialPort object is + // created successfully; failure is deferred to openPort(). + SerialPort sp = assertDoesNotThrow(() -> SerialPort.getCommPort(BOGUS_WINDOWS_PORT)); + assertNotNull(sp); + } + + @Test + void getCommPortWithNonExistentComPortFailsToOpen() { + SerialPort sp = SerialPort.getCommPort(BOGUS_WINDOWS_PORT); + assertFalse(sp.openPort(), "opening a non-existent COM port should fail"); + } + + @Test + void getCommPortDoesNotThrowForUnixStyleDescriptor() { + // On Windows, getCommPort rewrites any descriptor to \\.\ format + // without filesystem validation, so even a Unix-style path succeeds. + SerialPort sp = assertDoesNotThrow(() -> SerialPort.getCommPort(BOGUS_UNIX_PORT)); + assertNotNull(sp); + } + + @Test + void getCommPortWithUnixStyleDescriptorFailsToOpen() { + SerialPort sp = SerialPort.getCommPort(BOGUS_UNIX_PORT); + assertFalse(sp.openPort(), "opening a Unix-style port on Windows should fail"); + } + } + + @Nested + class ClientTransport { + + @Test + void constructorDoesNotCallGetCommPort() { + // Construction must always succeed regardless of port name. + // getCommPort is deferred to getSerialPort()/connect(). + assertDoesNotThrow( + () -> SerialPortClientTransport.create(cfg -> cfg.setSerialPort(BOGUS_UNIX_PORT))); + } + + @Test + @EnabledOnOs({OS.LINUX, OS.MAC}) + void getSerialPortThrowsOnLinuxMac() { + SerialPortClientTransport transport = + SerialPortClientTransport.create(cfg -> cfg.setSerialPort(BOGUS_UNIX_PORT)); + + // On Linux/Mac, getCommPort throws for non-existent ports, + // which getSerialPort wraps as ModbusException. + ModbusException ex = assertThrows(ModbusException.class, transport::getSerialPort); + assertTrue(ex.getMessage().contains(BOGUS_UNIX_PORT)); + } + + @Test + @EnabledOnOs(OS.WINDOWS) + void getSerialPortSucceedsOnWindows() throws ModbusException { + SerialPortClientTransport transport = + SerialPortClientTransport.create(cfg -> cfg.setSerialPort(BOGUS_WINDOWS_PORT)); + + // On Windows, getCommPort succeeds for non-existent COM ports, + // so getSerialPort also succeeds. Failure is deferred to connect/openPort. + assertDoesNotThrow(transport::getSerialPort); + } + + @Test + void connectFailsWithModbusConnectException() { + // On all platforms, connect with a bogus port results in ModbusConnectException. + // On Linux/Mac: getSerialPort() throws, caught and wrapped. + // On Windows: getSerialPort() succeeds, but openPort() returns false. + String port = isWindows() ? BOGUS_WINDOWS_PORT : BOGUS_UNIX_PORT; + SerialPortClientTransport transport = + SerialPortClientTransport.create(cfg -> cfg.setSerialPort(port)); + + CompletableFuture future = transport.connect(); + ExecutionException ex = assertThrows(ExecutionException.class, future::get); + assertInstanceOf(ModbusConnectException.class, ex.getCause()); + } + } + + @Nested + class ServerTransport { + + @Test + void constructorDoesNotCallGetCommPort() { + assertDoesNotThrow( + () -> SerialPortServerTransport.create(cfg -> cfg.setSerialPort(BOGUS_UNIX_PORT))); + } + + @Test + @EnabledOnOs({OS.LINUX, OS.MAC}) + void getSerialPortThrowsOnLinuxMac() { + SerialPortServerTransport transport = + SerialPortServerTransport.create(cfg -> cfg.setSerialPort(BOGUS_UNIX_PORT)); + + ModbusException ex = assertThrows(ModbusException.class, transport::getSerialPort); + assertTrue(ex.getMessage().contains(BOGUS_UNIX_PORT)); + } + + @Test + @EnabledOnOs(OS.WINDOWS) + void getSerialPortSucceedsOnWindows() { + SerialPortServerTransport transport = + SerialPortServerTransport.create(cfg -> cfg.setSerialPort(BOGUS_WINDOWS_PORT)); + + assertDoesNotThrow(transport::getSerialPort); + } + + @Test + void bindFailsWithModbusConnectException() { + String port = isWindows() ? BOGUS_WINDOWS_PORT : BOGUS_UNIX_PORT; + SerialPortServerTransport transport = + SerialPortServerTransport.create(cfg -> cfg.setSerialPort(port)); + + CompletableFuture future = transport.bind().toCompletableFuture(); + ExecutionException ex = assertThrows(ExecutionException.class, future::get); + assertInstanceOf(ModbusConnectException.class, ex.getCause()); + } + } + + private static boolean isWindows() { + return System.getProperty("os.name", "").toLowerCase().contains("win"); + } +}