feat(plc4j/modbus): Cleaned up and added some more code comments expl…

…aining why things are done the way they are.
apache · Feb 22, 2022 · 13d53e6 · 13d53e6
1 parent 51657e8
commit 13d53e6
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 9 deletions.
diff --git a/...vers/modbus/src/main/java/org/apache/plc4x/java/modbus/discovery/ModbusPlcDiscoverer.java b/...vers/modbus/src/main/java/org/apache/plc4x/java/modbus/discovery/ModbusPlcDiscoverer.java
@@ -112,7 +112,21 @@ private void executeDiscovery(CompletableFuture<PlcDiscoveryResponse> future, Pl
                 final OutputStream outputStream = socket.getOutputStream();
                 final InputStream inputStream = new BufferedInputStream(socket.getInputStream());
 
-                // Iterate over all possible unit-identifiers
+                // As we not only need to provide the IP but also the unit-identifier, we need
+                // to iterate over all possible values until we find one that works.
+                // Unfortunately the way devices react to invalid requests differ:
+                // - My heating system doesn't sends an error response for an invalid uint-identifier
+                // - The Modbus Server on a S7 simply accepts any unit-identifier
+                // - Modbus pal only responds to correct unit-identifiers and doesn't send anything
+                //   for invalid ones.
+                //
+                // So-far the only way I have found to reliably check if a Modbus device exists,
+                // was by trying to read a coil or register. Even if Modbus generally supports
+                // commands for diagnosing connections, it turns out none of these were actually
+                // supported by any device I came across. The spec is a bit unclear here, but
+                // it seems as if these are only supported on Serial (Modbus RTU)
+                // TODO: We should probably not only try to read a coil, but try any of the types and if one works, that's a match.
+                // Possibly we can fine tune this to speed up things.
                 int transactionIdentifier = 1;
                 for(short unitIdentifier = 1; unitIdentifier <= 247; unitIdentifier++) {
                     ModbusTcpADU packet = new ModbusTcpADU(transactionIdentifier++, unitIdentifier,
@@ -177,21 +191,15 @@ private void executeDiscovery(CompletableFuture<PlcDiscoveryResponse> future, Pl
                             ModbusTcpADU response = ModbusTcpADU.staticParse(readBuffer, true);
                             PlcDiscoveryItem discoveryItem;
                             if (!response.getPdu().getErrorFlag()) {
-                                //final ModbusPDUReadDeviceIdentificationResponse identificationResponse =
-                                //    (ModbusPDUReadDeviceIdentificationResponse) response.getPdu();
-                                // TODO: Unfortunately we need to find a device that's actually correctly responding to this request in order to implement this.
                                 discoveryItem = new DefaultPlcDiscoveryItem(
-                                    "modbus", "tcp", possibleAddress.getHostAddress(), Collections.singletonMap("unit-identifier", Integer.toString(unitIdentifier)), "known");
+                                    "modbus", "tcp", possibleAddress.getHostAddress(), Collections.singletonMap("unit-identifier", Integer.toString(unitIdentifier)), "unknown");
                                 discoveryItems.add(discoveryItem);
 
                                 // Give a handler the chance to react on the found device.
                                 if(handler != null) {
                                     handler.handle(discoveryItem);
                                 }
                                 break;
-                            /*} else {
-                                final ModbusPDUError errorPdu = (ModbusPDUError) response.getPdu();
-                                logger.info("Got error at address: {} with unit-identifier {} error {}", possibleAddress, unitIdentifier, errorPdu.getExceptionCode().toString());*/
                             }
                         } catch (ParseException e) {
                             // Ignore.

diff --git a/protocols/modbus/src/main/resources/protocols/modbus/modbus.mspec b/protocols/modbus/src/main/resources/protocols/modbus/modbus.mspec
@@ -238,7 +238,7 @@
 
         // Remark: Even if the Modbus spec states that supporting this type of request is mandatory
         // I have not come across a single device that really supported it. Some devices just reacted
-        // with an error, however the Modbus support of my S7 1200 just terminates the connection.
+        // with an error.
         ['false','0x2B','false'     ModbusPDUReadDeviceIdentificationRequest
             [const  uint 8                       meiType  0x0E]
             [simple ModbusDeviceInformationLevel level        ]