[modbus] Modbus transport API simplification #7994

ssalonen · 2020-06-23T17:55:01Z

Co-authored with @mrbig, user feedback from @Rossko57 .

As discussed in openhab/openhab-core#1435 (comment).

This is a rather large PR, with the aim of improving the modbus transport API developer experience based on feedback from @mrbig and sunspec bundle.

The main changes introduced:

callbacks are @Functional, allowing binding developer to use lambdas and much less verbose way to access the data read. The API is vaguely modeled after vertx AsyncResult: https://vertx.io/docs/apidocs/io/vertx/core/AsyncResult.html

comms.submitOneTimePoll(request, result -> {
            if (result.hasError()) {
                Exception error = (@NonNull Exception) result.getCause();
                handleError(error);
            } else {
                ModbusRegisterArray registers = (@NonNull ModbusRegisterArray) result.getRegisters();
                modelBlockReceived(registers);        
            });

Previously one had to have callback with three methods, but depending on the type of request, only two methods are actually applicable.

BasicModbusReadRequestBlueprint request = new BasicModbusReadRequestBlueprint(slaveId,
        ModbusReadFunctionCode.READ_MULTIPLE_REGISTERS, baseAddress, // Start address
        count,
        maxTries);
PollTask task = new BasicPollTaskImpl(endpoint, request, new ModbusReadCallback() {

    @Override
    public void onRegisters(ModbusReadRequestBlueprint request, ModbusRegisterArray registers) {
        handleReceived(registers);
    }

    @Override
    public void onError(ModbusReadRequestBlueprint request, Exception error) {
        handleError(error);             
    }

    @Override
    public void onBits(@Nullable ModbusReadRequestBlueprint request, @Nullable BitArray bits) {
        // this is never called by transport bundle since request is for reading registers, not "bits" 
    }
});
handler.getManagerRef().get().submitOneTimePoll(task);

The other change is that transport API calls take in relevant parameters directly, and it's not necessary to "bundle" arguments into a BasicPollTaskImpl . See example above
There were many java interfaces which had only one implementation. We have removed the interfaces and use the implementation class in many places. There were also trivial implementations which did not really add value (e.g. specialized bit array implementation for cases with single bit) I think this reduces the cognitive load when getting familiar with the transport bundle. Commits can be recognized with "abstractions removed"

The above changes are not introducing any changes in behaviour.

There is also an API change which is changing slightly how to interact with transport API: introduction of ModbusCommunicationInterface.

Now, the transport API consumers need to "open" a ModbusCommunicationInterface to interact with a particular Modbus server ("slaves"). Modbus slaves are identified by tcphost:tcpport or serialport. Once you have been finished interacting with the slave, you close() the communication interface. There can be many communication interfaces open to the same slave.

Closing the communication interface signals to transport bundle that you are not expecting any further requests for a while. The transport bundle automatically frees resources (e.g. disconnects TCP connections) immediately if the last communication interface pointing to a server is closed.

The eager cleanup of resources was one of the main motivations for this change. In addition, some of the API details became simpler as well (e.g. error handling when there are conflicting baud rates trying to be associated with the same serial port).

In practice, the communication interface is held by the thing representing the modbus slave ("tcp" or "serial" thing). Once the thing is disposed, also the communication interface is closed.

Third change is improving automatic disconnection of unused idle connections. Previously connection age was checked after every request, and disconnected if older than the configured value. This had the downside that connection pool can contain open old connections when there are no transactions.

Now, the connection pool is inspected every 10 seconds, and old idle connections are disconnected.

For the user this is quite a transparent change, and should just mean that openHAB is not keeping any connections for long periods of time.

ssalonen · 2020-06-23T17:56:00Z

Prebuilt jars for commit 9a65a4a (June 23, 2020)
snapshot-commit-9a65a4abe2e9845b351e5adcc569f6a2bb50e2c7.zip

TravisBuddy · 2020-06-23T18:23:05Z

Travis tests have failed

Hey @ssalonen,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

TravisBuddy · 2020-06-23T18:30:05Z

Travis tests have failed

Hey @ssalonen,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

TravisBuddy · 2020-06-24T10:20:49Z

Travis tests have failed

Hey @ssalonen,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

TravisBuddy · 2020-06-24T10:23:47Z

Travis tests have failed

Hey @ssalonen,
please read the following log in order to understand the failure reason. There might also be some helpful tips along the way.
It will be awesome if you fix what is wrong and commit the changes.

TravisBuddy · 2020-06-24T11:15:29Z

Travis tests were successful

Hey @ssalonen,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

...ain/java/org/openhab/binding/modbus/internal/handler/AbstractModbusEndpointThingHandler.java

...io.transport.modbus/src/main/java/org/openhab/io/transport/modbus/AsyncModbusReadResult.java

...ansport.modbus/src/main/java/org/openhab/io/transport/modbus/ModbusReadRequestBlueprint.java

...b.io.transport.modbus/src/main/java/org/openhab/io/transport/modbus/ModbusRegisterArray.java

...ansport.modbus/src/main/java/org/openhab/io/transport/modbus/internal/ModbusManagerImpl.java

...modbus/src/main/java/org/openhab/binding/modbus/internal/handler/ModbusDataThingHandler.java

...inding.modbus/src/main/java/org/openhab/binding/modbus/handler/ModbusPollerThingHandler.java

...main/java/org/openhab/binding/modbus/sunspec/internal/discovery/SunspecDiscoveryProcess.java

fwolter · 2020-06-24T21:47:31Z

comms.submitOneTimePoll(request, result -> {
            if (result.hasError()) {
                Exception error = (@NonNull Exception) result.getCause();
                handleError(error);
            } else {
                ModbusRegisterArray registers = (@NonNull ModbusRegisterArray) result.getRegisters();
                modelBlockReceived(registers);        
            });

I think error handling is mandatory. Why not adding a second functional interface to submitOneTimePoll() handling the error case?

comms.submitOneTimePoll(request, this::handleError, result -> {
...

ssalonen · 2020-06-25T08:58:47Z

@fwolter very valid suggestion! With @mrbig we tried to model other similar APIS which bundle errors and valid results into same object

We played with several different API designs but to be honest, separating error from the successful result was not one of them.

By separating the error case the API consumer can avoid an if statement. On the other hand, bundling error case to the same result object makes it clear when the operation has ended (either successfully or with error). Are there any other upsides/downsides?

Either way will certainly work. Any thoughts @mrbig ?

I am not too keen on changing this if the gained benefit is small - it is somewhat large change considering the large footprint of the tests.

fwolter · 2020-06-26T16:00:00Z

Handling result and exception paths separately reflects OOP paradigm. You don't need the casting and the getCause() invocation. Furthermore, the compiler complains, if the user forgets to handle the error.

I can't follow your argument. not being clear when the operation finished. You could specify either the error handler or the result handler is invoked, when the operation finished.

ssalonen · 2020-06-27T08:18:26Z

@fwolter I like your thinking. I think it's especially valuable that you have to handle the errors and decide what to do with them.

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

ssalonen · 2020-07-06T08:38:23Z

@mrbig (huge thanks! 👏 ) & me worked on this more and now errors are propagated via their own callback. We also use Optionals to wrap the data in callbacks

New review welcome!

ssalonen · 2020-07-06T08:39:40Z

...ansport.modbus/src/main/java/org/openhab/io/transport/modbus/internal/ModbusManagerImpl.java

+ started);
+ try {
+ executeOperation(task, false, pollOperation);
+ } catch (RuntimeException e) {


Cherry-picked 5bde9d6 during rebase in this commit.

TravisBuddy · 2020-07-06T08:53:20Z

Travis tests were successful

Hey @ssalonen,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

TravisBuddy · 2020-07-06T08:59:03Z

Travis tests were successful

Hey @ssalonen,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

...modbus/src/main/java/org/openhab/binding/modbus/internal/handler/ModbusDataThingHandler.java

...inding.modbus/src/main/java/org/openhab/binding/modbus/handler/ModbusPollerThingHandler.java

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

TravisBuddy · 2020-07-08T08:46:53Z

Travis tests were successful

Hey @ssalonen,
we found no major flaws with your code. Still you might want to look at this logfile, as we usually suggest some optional improvements.

@nonnull

* [modbus] connection closing behaviour finetuned The binding closes TCP/serial connections connections as per user configuration. It is either - every time, immediately after a transaction (reconnectAfterMillis=0) - *after* a read/write transaction, if the connection has been open "too long" (configurable with reconnectAfterMillis) We have an obvious downside to this simple logic -- the connection can remain open "indefinitely" if there are no transactions occurring. With Modbus we are quite often dealing with PLCs and other embedded systems with limited resources, and "reserving" the connection is not always something we want. Previously (2.5.x branch) connections were also closed when - a regular poll task was unregistered (i.e. we stop reading regularly from a modbus server ("slave"). Since most users have regular polling in place, so this ensured that connections do get closed. In this commit, the behaviour is adjusted such that connections are closed when last communication interface pointing to the server/serial port (i.e. "endpoint") is close()'d. With modbus binding this basically means when the tcp or serial thing is removed / disabled, the connections is closed/freed, but only if it is the last thing pointing to that server or serial port. Since modbus.sunspec binding reuses modbus serial & tcp endpoint things, the same note applies for modbus.sunspec binding. This is change in functional behaviour but in a way is logical. We can further introduce to have "delayed"/"deferred"/"debounce" connection closing connections as per the setting reconnectAfterMillis even in situations where communication interface is still open. * [modbus] Check and disconnect idle connections without transactions * [modbus] mvn spotless:apply * [modbubs] Fixed log message * [modbus] Race condition fix The CountDownLatch was used as a guard (latch.await) in many tests to wait for callbacks to be called before proceeding with assertions. Since the latch was countDown() beginning of the callback, we introduced a race condition with the subsequent assertions and updating the other counters used in the subsequent assertions. This commit updates the CountDownLatch as the last step of the callback, resolving the race condition. * [modbus] small test fix * [modbus] readcallback changed to nonnull * [modbus] Refactored ModbusCommunicationInterface to have seperate callback for result and failure However I had to dig deep to reach all the affected parts. Also there is a new callback, and a new "result" type to communicate the failures. * [modbus] SmokeTest refactored to new api * [modbus] Modbus bundle refactored to use the new api * [modbus][sunspec] refactored sunspec bundle to use the new modbus API * [modbus] refactor modbus tests to use the new api * [modbus] Removed ModbusWriteCallback interface from ModbusDataThingHandler Also reset ModbusDataThingHandler testWriteHandling generic to it's original form * [modbus] ModbusDataThingHandler does not implement ModbusReadCallback anymore Instead it has a public onReadResult method. ModbusPollerThingHandler changed to work with ModbusDataThingHandler children. * [modbus] Fixed caching in ModbusPollerThingHandler * [modbus] read modbus data as Optionals * [modbus] toString for PollResult * [modbus] fixed confusing variable name * [modbus] Disallow null callbacks * [modbus] mvn spotless:apply * [modbus] Removing Nullable decorations * [modbus] submitOneTimeWrite simplification * [modbus] Less verbose logging * [modbus] submitOneTimePoll simplification * [modbus] Less verbose logging * [modbus] Many null warnings removed * [modbus] Fix: no need for a @nonnull annotation because it is default * [modbus] Removing unneeded Nullable, using final in immutables * [modbus] Explicit functional interface * [modbus] @nullable and @NonNullByDefault aligned with coding conventions * [modbus] Collections.emptyMap instead of allocating new map every time. Signed-off-by: Sami Salonen <ssalonen@gmail.com> Co-authored-by: Nagy Attila Gábor <mrbig@sneaker.hu>

@nonnull

* [modbus] connection closing behaviour finetuned The binding closes TCP/serial connections connections as per user configuration. It is either - every time, immediately after a transaction (reconnectAfterMillis=0) - *after* a read/write transaction, if the connection has been open "too long" (configurable with reconnectAfterMillis) We have an obvious downside to this simple logic -- the connection can remain open "indefinitely" if there are no transactions occurring. With Modbus we are quite often dealing with PLCs and other embedded systems with limited resources, and "reserving" the connection is not always something we want. Previously (2.5.x branch) connections were also closed when - a regular poll task was unregistered (i.e. we stop reading regularly from a modbus server ("slave"). Since most users have regular polling in place, so this ensured that connections do get closed. In this commit, the behaviour is adjusted such that connections are closed when last communication interface pointing to the server/serial port (i.e. "endpoint") is close()'d. With modbus binding this basically means when the tcp or serial thing is removed / disabled, the connections is closed/freed, but only if it is the last thing pointing to that server or serial port. Since modbus.sunspec binding reuses modbus serial & tcp endpoint things, the same note applies for modbus.sunspec binding. This is change in functional behaviour but in a way is logical. We can further introduce to have "delayed"/"deferred"/"debounce" connection closing connections as per the setting reconnectAfterMillis even in situations where communication interface is still open. * [modbus] Check and disconnect idle connections without transactions * [modbus] mvn spotless:apply * [modbubs] Fixed log message * [modbus] Race condition fix The CountDownLatch was used as a guard (latch.await) in many tests to wait for callbacks to be called before proceeding with assertions. Since the latch was countDown() beginning of the callback, we introduced a race condition with the subsequent assertions and updating the other counters used in the subsequent assertions. This commit updates the CountDownLatch as the last step of the callback, resolving the race condition. * [modbus] small test fix * [modbus] readcallback changed to nonnull * [modbus] Refactored ModbusCommunicationInterface to have seperate callback for result and failure However I had to dig deep to reach all the affected parts. Also there is a new callback, and a new "result" type to communicate the failures. * [modbus] SmokeTest refactored to new api * [modbus] Modbus bundle refactored to use the new api * [modbus][sunspec] refactored sunspec bundle to use the new modbus API * [modbus] refactor modbus tests to use the new api * [modbus] Removed ModbusWriteCallback interface from ModbusDataThingHandler Also reset ModbusDataThingHandler testWriteHandling generic to it's original form * [modbus] ModbusDataThingHandler does not implement ModbusReadCallback anymore Instead it has a public onReadResult method. ModbusPollerThingHandler changed to work with ModbusDataThingHandler children. * [modbus] Fixed caching in ModbusPollerThingHandler * [modbus] read modbus data as Optionals * [modbus] toString for PollResult * [modbus] fixed confusing variable name * [modbus] Disallow null callbacks * [modbus] mvn spotless:apply * [modbus] Removing Nullable decorations * [modbus] submitOneTimeWrite simplification * [modbus] Less verbose logging * [modbus] submitOneTimePoll simplification * [modbus] Less verbose logging * [modbus] Many null warnings removed * [modbus] Fix: no need for a @nonnull annotation because it is default * [modbus] Removing unneeded Nullable, using final in immutables * [modbus] Explicit functional interface * [modbus] @nullable and @NonNullByDefault aligned with coding conventions * [modbus] Collections.emptyMap instead of allocating new map every time. Signed-off-by: Sami Salonen <ssalonen@gmail.com> Co-authored-by: Nagy Attila Gábor <mrbig@sneaker.hu> Signed-off-by: CSchlipp <christian@schlipp.de>

@nonnull

* [modbus] connection closing behaviour finetuned The binding closes TCP/serial connections connections as per user configuration. It is either - every time, immediately after a transaction (reconnectAfterMillis=0) - *after* a read/write transaction, if the connection has been open "too long" (configurable with reconnectAfterMillis) We have an obvious downside to this simple logic -- the connection can remain open "indefinitely" if there are no transactions occurring. With Modbus we are quite often dealing with PLCs and other embedded systems with limited resources, and "reserving" the connection is not always something we want. Previously (2.5.x branch) connections were also closed when - a regular poll task was unregistered (i.e. we stop reading regularly from a modbus server ("slave"). Since most users have regular polling in place, so this ensured that connections do get closed. In this commit, the behaviour is adjusted such that connections are closed when last communication interface pointing to the server/serial port (i.e. "endpoint") is close()'d. With modbus binding this basically means when the tcp or serial thing is removed / disabled, the connections is closed/freed, but only if it is the last thing pointing to that server or serial port. Since modbus.sunspec binding reuses modbus serial & tcp endpoint things, the same note applies for modbus.sunspec binding. This is change in functional behaviour but in a way is logical. We can further introduce to have "delayed"/"deferred"/"debounce" connection closing connections as per the setting reconnectAfterMillis even in situations where communication interface is still open. * [modbus] Check and disconnect idle connections without transactions * [modbus] mvn spotless:apply * [modbubs] Fixed log message * [modbus] Race condition fix The CountDownLatch was used as a guard (latch.await) in many tests to wait for callbacks to be called before proceeding with assertions. Since the latch was countDown() beginning of the callback, we introduced a race condition with the subsequent assertions and updating the other counters used in the subsequent assertions. This commit updates the CountDownLatch as the last step of the callback, resolving the race condition. * [modbus] small test fix * [modbus] readcallback changed to nonnull * [modbus] Refactored ModbusCommunicationInterface to have seperate callback for result and failure However I had to dig deep to reach all the affected parts. Also there is a new callback, and a new "result" type to communicate the failures. * [modbus] SmokeTest refactored to new api * [modbus] Modbus bundle refactored to use the new api * [modbus][sunspec] refactored sunspec bundle to use the new modbus API * [modbus] refactor modbus tests to use the new api * [modbus] Removed ModbusWriteCallback interface from ModbusDataThingHandler Also reset ModbusDataThingHandler testWriteHandling generic to it's original form * [modbus] ModbusDataThingHandler does not implement ModbusReadCallback anymore Instead it has a public onReadResult method. ModbusPollerThingHandler changed to work with ModbusDataThingHandler children. * [modbus] Fixed caching in ModbusPollerThingHandler * [modbus] read modbus data as Optionals * [modbus] toString for PollResult * [modbus] fixed confusing variable name * [modbus] Disallow null callbacks * [modbus] mvn spotless:apply * [modbus] Removing Nullable decorations * [modbus] submitOneTimeWrite simplification * [modbus] Less verbose logging * [modbus] submitOneTimePoll simplification * [modbus] Less verbose logging * [modbus] Many null warnings removed * [modbus] Fix: no need for a @nonnull annotation because it is default * [modbus] Removing unneeded Nullable, using final in immutables * [modbus] Explicit functional interface * [modbus] @nullable and @NonNullByDefault aligned with coding conventions * [modbus] Collections.emptyMap instead of allocating new map every time. Signed-off-by: Sami Salonen <ssalonen@gmail.com> Co-authored-by: Nagy Attila Gábor <mrbig@sneaker.hu> Signed-off-by: MPH80 <michael@hazelden.me>

@nonnull

* [modbus] connection closing behaviour finetuned The binding closes TCP/serial connections connections as per user configuration. It is either - every time, immediately after a transaction (reconnectAfterMillis=0) - *after* a read/write transaction, if the connection has been open "too long" (configurable with reconnectAfterMillis) We have an obvious downside to this simple logic -- the connection can remain open "indefinitely" if there are no transactions occurring. With Modbus we are quite often dealing with PLCs and other embedded systems with limited resources, and "reserving" the connection is not always something we want. Previously (2.5.x branch) connections were also closed when - a regular poll task was unregistered (i.e. we stop reading regularly from a modbus server ("slave"). Since most users have regular polling in place, so this ensured that connections do get closed. In this commit, the behaviour is adjusted such that connections are closed when last communication interface pointing to the server/serial port (i.e. "endpoint") is close()'d. With modbus binding this basically means when the tcp or serial thing is removed / disabled, the connections is closed/freed, but only if it is the last thing pointing to that server or serial port. Since modbus.sunspec binding reuses modbus serial & tcp endpoint things, the same note applies for modbus.sunspec binding. This is change in functional behaviour but in a way is logical. We can further introduce to have "delayed"/"deferred"/"debounce" connection closing connections as per the setting reconnectAfterMillis even in situations where communication interface is still open. * [modbus] Check and disconnect idle connections without transactions * [modbus] mvn spotless:apply * [modbubs] Fixed log message * [modbus] Race condition fix The CountDownLatch was used as a guard (latch.await) in many tests to wait for callbacks to be called before proceeding with assertions. Since the latch was countDown() beginning of the callback, we introduced a race condition with the subsequent assertions and updating the other counters used in the subsequent assertions. This commit updates the CountDownLatch as the last step of the callback, resolving the race condition. * [modbus] small test fix * [modbus] readcallback changed to nonnull * [modbus] Refactored ModbusCommunicationInterface to have seperate callback for result and failure However I had to dig deep to reach all the affected parts. Also there is a new callback, and a new "result" type to communicate the failures. * [modbus] SmokeTest refactored to new api * [modbus] Modbus bundle refactored to use the new api * [modbus][sunspec] refactored sunspec bundle to use the new modbus API * [modbus] refactor modbus tests to use the new api * [modbus] Removed ModbusWriteCallback interface from ModbusDataThingHandler Also reset ModbusDataThingHandler testWriteHandling generic to it's original form * [modbus] ModbusDataThingHandler does not implement ModbusReadCallback anymore Instead it has a public onReadResult method. ModbusPollerThingHandler changed to work with ModbusDataThingHandler children. * [modbus] Fixed caching in ModbusPollerThingHandler * [modbus] read modbus data as Optionals * [modbus] toString for PollResult * [modbus] fixed confusing variable name * [modbus] Disallow null callbacks * [modbus] mvn spotless:apply * [modbus] Removing Nullable decorations * [modbus] submitOneTimeWrite simplification * [modbus] Less verbose logging * [modbus] submitOneTimePoll simplification * [modbus] Less verbose logging * [modbus] Many null warnings removed * [modbus] Fix: no need for a @nonnull annotation because it is default * [modbus] Removing unneeded Nullable, using final in immutables * [modbus] Explicit functional interface * [modbus] @nullable and @NonNullByDefault aligned with coding conventions * [modbus] Collections.emptyMap instead of allocating new map every time. Signed-off-by: Sami Salonen <ssalonen@gmail.com> Co-authored-by: Nagy Attila Gábor <mrbig@sneaker.hu>

@nonnull

* [modbus] connection closing behaviour finetuned The binding closes TCP/serial connections connections as per user configuration. It is either - every time, immediately after a transaction (reconnectAfterMillis=0) - *after* a read/write transaction, if the connection has been open "too long" (configurable with reconnectAfterMillis) We have an obvious downside to this simple logic -- the connection can remain open "indefinitely" if there are no transactions occurring. With Modbus we are quite often dealing with PLCs and other embedded systems with limited resources, and "reserving" the connection is not always something we want. Previously (2.5.x branch) connections were also closed when - a regular poll task was unregistered (i.e. we stop reading regularly from a modbus server ("slave"). Since most users have regular polling in place, so this ensured that connections do get closed. In this commit, the behaviour is adjusted such that connections are closed when last communication interface pointing to the server/serial port (i.e. "endpoint") is close()'d. With modbus binding this basically means when the tcp or serial thing is removed / disabled, the connections is closed/freed, but only if it is the last thing pointing to that server or serial port. Since modbus.sunspec binding reuses modbus serial & tcp endpoint things, the same note applies for modbus.sunspec binding. This is change in functional behaviour but in a way is logical. We can further introduce to have "delayed"/"deferred"/"debounce" connection closing connections as per the setting reconnectAfterMillis even in situations where communication interface is still open. * [modbus] Check and disconnect idle connections without transactions * [modbus] mvn spotless:apply * [modbubs] Fixed log message * [modbus] Race condition fix The CountDownLatch was used as a guard (latch.await) in many tests to wait for callbacks to be called before proceeding with assertions. Since the latch was countDown() beginning of the callback, we introduced a race condition with the subsequent assertions and updating the other counters used in the subsequent assertions. This commit updates the CountDownLatch as the last step of the callback, resolving the race condition. * [modbus] small test fix * [modbus] readcallback changed to nonnull * [modbus] Refactored ModbusCommunicationInterface to have seperate callback for result and failure However I had to dig deep to reach all the affected parts. Also there is a new callback, and a new "result" type to communicate the failures. * [modbus] SmokeTest refactored to new api * [modbus] Modbus bundle refactored to use the new api * [modbus][sunspec] refactored sunspec bundle to use the new modbus API * [modbus] refactor modbus tests to use the new api * [modbus] Removed ModbusWriteCallback interface from ModbusDataThingHandler Also reset ModbusDataThingHandler testWriteHandling generic to it's original form * [modbus] ModbusDataThingHandler does not implement ModbusReadCallback anymore Instead it has a public onReadResult method. ModbusPollerThingHandler changed to work with ModbusDataThingHandler children. * [modbus] Fixed caching in ModbusPollerThingHandler * [modbus] read modbus data as Optionals * [modbus] toString for PollResult * [modbus] fixed confusing variable name * [modbus] Disallow null callbacks * [modbus] mvn spotless:apply * [modbus] Removing Nullable decorations * [modbus] submitOneTimeWrite simplification * [modbus] Less verbose logging * [modbus] submitOneTimePoll simplification * [modbus] Less verbose logging * [modbus] Many null warnings removed * [modbus] Fix: no need for a @nonnull annotation because it is default * [modbus] Removing unneeded Nullable, using final in immutables * [modbus] Explicit functional interface * [modbus] @nullable and @NonNullByDefault aligned with coding conventions * [modbus] Collections.emptyMap instead of allocating new map every time. Signed-off-by: Sami Salonen <ssalonen@gmail.com> Co-authored-by: Nagy Attila Gábor <mrbig@sneaker.hu>

@nonnull

* [modbus] connection closing behaviour finetuned The binding closes TCP/serial connections connections as per user configuration. It is either - every time, immediately after a transaction (reconnectAfterMillis=0) - *after* a read/write transaction, if the connection has been open "too long" (configurable with reconnectAfterMillis) We have an obvious downside to this simple logic -- the connection can remain open "indefinitely" if there are no transactions occurring. With Modbus we are quite often dealing with PLCs and other embedded systems with limited resources, and "reserving" the connection is not always something we want. Previously (2.5.x branch) connections were also closed when - a regular poll task was unregistered (i.e. we stop reading regularly from a modbus server ("slave"). Since most users have regular polling in place, so this ensured that connections do get closed. In this commit, the behaviour is adjusted such that connections are closed when last communication interface pointing to the server/serial port (i.e. "endpoint") is close()'d. With modbus binding this basically means when the tcp or serial thing is removed / disabled, the connections is closed/freed, but only if it is the last thing pointing to that server or serial port. Since modbus.sunspec binding reuses modbus serial & tcp endpoint things, the same note applies for modbus.sunspec binding. This is change in functional behaviour but in a way is logical. We can further introduce to have "delayed"/"deferred"/"debounce" connection closing connections as per the setting reconnectAfterMillis even in situations where communication interface is still open. * [modbus] Check and disconnect idle connections without transactions * [modbus] mvn spotless:apply * [modbubs] Fixed log message * [modbus] Race condition fix The CountDownLatch was used as a guard (latch.await) in many tests to wait for callbacks to be called before proceeding with assertions. Since the latch was countDown() beginning of the callback, we introduced a race condition with the subsequent assertions and updating the other counters used in the subsequent assertions. This commit updates the CountDownLatch as the last step of the callback, resolving the race condition. * [modbus] small test fix * [modbus] readcallback changed to nonnull * [modbus] Refactored ModbusCommunicationInterface to have seperate callback for result and failure However I had to dig deep to reach all the affected parts. Also there is a new callback, and a new "result" type to communicate the failures. * [modbus] SmokeTest refactored to new api * [modbus] Modbus bundle refactored to use the new api * [modbus][sunspec] refactored sunspec bundle to use the new modbus API * [modbus] refactor modbus tests to use the new api * [modbus] Removed ModbusWriteCallback interface from ModbusDataThingHandler Also reset ModbusDataThingHandler testWriteHandling generic to it's original form * [modbus] ModbusDataThingHandler does not implement ModbusReadCallback anymore Instead it has a public onReadResult method. ModbusPollerThingHandler changed to work with ModbusDataThingHandler children. * [modbus] Fixed caching in ModbusPollerThingHandler * [modbus] read modbus data as Optionals * [modbus] toString for PollResult * [modbus] fixed confusing variable name * [modbus] Disallow null callbacks * [modbus] mvn spotless:apply * [modbus] Removing Nullable decorations * [modbus] submitOneTimeWrite simplification * [modbus] Less verbose logging * [modbus] submitOneTimePoll simplification * [modbus] Less verbose logging * [modbus] Many null warnings removed * [modbus] Fix: no need for a @nonnull annotation because it is default * [modbus] Removing unneeded Nullable, using final in immutables * [modbus] Explicit functional interface * [modbus] @nullable and @NonNullByDefault aligned with coding conventions * [modbus] Collections.emptyMap instead of allocating new map every time. Signed-off-by: Sami Salonen <ssalonen@gmail.com> Co-authored-by: Nagy Attila Gábor <mrbig@sneaker.hu>

@nonnull

* [modbus] connection closing behaviour finetuned The binding closes TCP/serial connections connections as per user configuration. It is either - every time, immediately after a transaction (reconnectAfterMillis=0) - *after* a read/write transaction, if the connection has been open "too long" (configurable with reconnectAfterMillis) We have an obvious downside to this simple logic -- the connection can remain open "indefinitely" if there are no transactions occurring. With Modbus we are quite often dealing with PLCs and other embedded systems with limited resources, and "reserving" the connection is not always something we want. Previously (2.5.x branch) connections were also closed when - a regular poll task was unregistered (i.e. we stop reading regularly from a modbus server ("slave"). Since most users have regular polling in place, so this ensured that connections do get closed. In this commit, the behaviour is adjusted such that connections are closed when last communication interface pointing to the server/serial port (i.e. "endpoint") is close()'d. With modbus binding this basically means when the tcp or serial thing is removed / disabled, the connections is closed/freed, but only if it is the last thing pointing to that server or serial port. Since modbus.sunspec binding reuses modbus serial & tcp endpoint things, the same note applies for modbus.sunspec binding. This is change in functional behaviour but in a way is logical. We can further introduce to have "delayed"/"deferred"/"debounce" connection closing connections as per the setting reconnectAfterMillis even in situations where communication interface is still open. * [modbus] Check and disconnect idle connections without transactions * [modbus] mvn spotless:apply * [modbubs] Fixed log message * [modbus] Race condition fix The CountDownLatch was used as a guard (latch.await) in many tests to wait for callbacks to be called before proceeding with assertions. Since the latch was countDown() beginning of the callback, we introduced a race condition with the subsequent assertions and updating the other counters used in the subsequent assertions. This commit updates the CountDownLatch as the last step of the callback, resolving the race condition. * [modbus] small test fix * [modbus] readcallback changed to nonnull * [modbus] Refactored ModbusCommunicationInterface to have seperate callback for result and failure However I had to dig deep to reach all the affected parts. Also there is a new callback, and a new "result" type to communicate the failures. * [modbus] SmokeTest refactored to new api * [modbus] Modbus bundle refactored to use the new api * [modbus][sunspec] refactored sunspec bundle to use the new modbus API * [modbus] refactor modbus tests to use the new api * [modbus] Removed ModbusWriteCallback interface from ModbusDataThingHandler Also reset ModbusDataThingHandler testWriteHandling generic to it's original form * [modbus] ModbusDataThingHandler does not implement ModbusReadCallback anymore Instead it has a public onReadResult method. ModbusPollerThingHandler changed to work with ModbusDataThingHandler children. * [modbus] Fixed caching in ModbusPollerThingHandler * [modbus] read modbus data as Optionals * [modbus] toString for PollResult * [modbus] fixed confusing variable name * [modbus] Disallow null callbacks * [modbus] mvn spotless:apply * [modbus] Removing Nullable decorations * [modbus] submitOneTimeWrite simplification * [modbus] Less verbose logging * [modbus] submitOneTimePoll simplification * [modbus] Less verbose logging * [modbus] Many null warnings removed * [modbus] Fix: no need for a @nonnull annotation because it is default * [modbus] Removing unneeded Nullable, using final in immutables * [modbus] Explicit functional interface * [modbus] @nullable and @NonNullByDefault aligned with coding conventions * [modbus] Collections.emptyMap instead of allocating new map every time. Signed-off-by: Sami Salonen <ssalonen@gmail.com> Co-authored-by: Nagy Attila Gábor <mrbig@sneaker.hu>

@nonnull

* [modbus] connection closing behaviour finetuned The binding closes TCP/serial connections connections as per user configuration. It is either - every time, immediately after a transaction (reconnectAfterMillis=0) - *after* a read/write transaction, if the connection has been open "too long" (configurable with reconnectAfterMillis) We have an obvious downside to this simple logic -- the connection can remain open "indefinitely" if there are no transactions occurring. With Modbus we are quite often dealing with PLCs and other embedded systems with limited resources, and "reserving" the connection is not always something we want. Previously (2.5.x branch) connections were also closed when - a regular poll task was unregistered (i.e. we stop reading regularly from a modbus server ("slave"). Since most users have regular polling in place, so this ensured that connections do get closed. In this commit, the behaviour is adjusted such that connections are closed when last communication interface pointing to the server/serial port (i.e. "endpoint") is close()'d. With modbus binding this basically means when the tcp or serial thing is removed / disabled, the connections is closed/freed, but only if it is the last thing pointing to that server or serial port. Since modbus.sunspec binding reuses modbus serial & tcp endpoint things, the same note applies for modbus.sunspec binding. This is change in functional behaviour but in a way is logical. We can further introduce to have "delayed"/"deferred"/"debounce" connection closing connections as per the setting reconnectAfterMillis even in situations where communication interface is still open. * [modbus] Check and disconnect idle connections without transactions * [modbus] mvn spotless:apply * [modbubs] Fixed log message * [modbus] Race condition fix The CountDownLatch was used as a guard (latch.await) in many tests to wait for callbacks to be called before proceeding with assertions. Since the latch was countDown() beginning of the callback, we introduced a race condition with the subsequent assertions and updating the other counters used in the subsequent assertions. This commit updates the CountDownLatch as the last step of the callback, resolving the race condition. * [modbus] small test fix * [modbus] readcallback changed to nonnull * [modbus] Refactored ModbusCommunicationInterface to have seperate callback for result and failure However I had to dig deep to reach all the affected parts. Also there is a new callback, and a new "result" type to communicate the failures. * [modbus] SmokeTest refactored to new api * [modbus] Modbus bundle refactored to use the new api * [modbus][sunspec] refactored sunspec bundle to use the new modbus API * [modbus] refactor modbus tests to use the new api * [modbus] Removed ModbusWriteCallback interface from ModbusDataThingHandler Also reset ModbusDataThingHandler testWriteHandling generic to it's original form * [modbus] ModbusDataThingHandler does not implement ModbusReadCallback anymore Instead it has a public onReadResult method. ModbusPollerThingHandler changed to work with ModbusDataThingHandler children. * [modbus] Fixed caching in ModbusPollerThingHandler * [modbus] read modbus data as Optionals * [modbus] toString for PollResult * [modbus] fixed confusing variable name * [modbus] Disallow null callbacks * [modbus] mvn spotless:apply * [modbus] Removing Nullable decorations * [modbus] submitOneTimeWrite simplification * [modbus] Less verbose logging * [modbus] submitOneTimePoll simplification * [modbus] Less verbose logging * [modbus] Many null warnings removed * [modbus] Fix: no need for a @nonnull annotation because it is default * [modbus] Removing unneeded Nullable, using final in immutables * [modbus] Explicit functional interface * [modbus] @nullable and @NonNullByDefault aligned with coding conventions * [modbus] Collections.emptyMap instead of allocating new map every time. Signed-off-by: Sami Salonen <ssalonen@gmail.com> Co-authored-by: Nagy Attila Gábor <mrbig@sneaker.hu> Signed-off-by: Daan Meijer <daan@studioseptember.nl>

@nonnull

* [modbus] connection closing behaviour finetuned The binding closes TCP/serial connections connections as per user configuration. It is either - every time, immediately after a transaction (reconnectAfterMillis=0) - *after* a read/write transaction, if the connection has been open "too long" (configurable with reconnectAfterMillis) We have an obvious downside to this simple logic -- the connection can remain open "indefinitely" if there are no transactions occurring. With Modbus we are quite often dealing with PLCs and other embedded systems with limited resources, and "reserving" the connection is not always something we want. Previously (2.5.x branch) connections were also closed when - a regular poll task was unregistered (i.e. we stop reading regularly from a modbus server ("slave"). Since most users have regular polling in place, so this ensured that connections do get closed. In this commit, the behaviour is adjusted such that connections are closed when last communication interface pointing to the server/serial port (i.e. "endpoint") is close()'d. With modbus binding this basically means when the tcp or serial thing is removed / disabled, the connections is closed/freed, but only if it is the last thing pointing to that server or serial port. Since modbus.sunspec binding reuses modbus serial & tcp endpoint things, the same note applies for modbus.sunspec binding. This is change in functional behaviour but in a way is logical. We can further introduce to have "delayed"/"deferred"/"debounce" connection closing connections as per the setting reconnectAfterMillis even in situations where communication interface is still open. * [modbus] Check and disconnect idle connections without transactions * [modbus] mvn spotless:apply * [modbubs] Fixed log message * [modbus] Race condition fix The CountDownLatch was used as a guard (latch.await) in many tests to wait for callbacks to be called before proceeding with assertions. Since the latch was countDown() beginning of the callback, we introduced a race condition with the subsequent assertions and updating the other counters used in the subsequent assertions. This commit updates the CountDownLatch as the last step of the callback, resolving the race condition. * [modbus] small test fix * [modbus] readcallback changed to nonnull * [modbus] Refactored ModbusCommunicationInterface to have seperate callback for result and failure However I had to dig deep to reach all the affected parts. Also there is a new callback, and a new "result" type to communicate the failures. * [modbus] SmokeTest refactored to new api * [modbus] Modbus bundle refactored to use the new api * [modbus][sunspec] refactored sunspec bundle to use the new modbus API * [modbus] refactor modbus tests to use the new api * [modbus] Removed ModbusWriteCallback interface from ModbusDataThingHandler Also reset ModbusDataThingHandler testWriteHandling generic to it's original form * [modbus] ModbusDataThingHandler does not implement ModbusReadCallback anymore Instead it has a public onReadResult method. ModbusPollerThingHandler changed to work with ModbusDataThingHandler children. * [modbus] Fixed caching in ModbusPollerThingHandler * [modbus] read modbus data as Optionals * [modbus] toString for PollResult * [modbus] fixed confusing variable name * [modbus] Disallow null callbacks * [modbus] mvn spotless:apply * [modbus] Removing Nullable decorations * [modbus] submitOneTimeWrite simplification * [modbus] Less verbose logging * [modbus] submitOneTimePoll simplification * [modbus] Less verbose logging * [modbus] Many null warnings removed * [modbus] Fix: no need for a @nonnull annotation because it is default * [modbus] Removing unneeded Nullable, using final in immutables * [modbus] Explicit functional interface * [modbus] @nullable and @NonNullByDefault aligned with coding conventions * [modbus] Collections.emptyMap instead of allocating new map every time. Signed-off-by: Sami Salonen <ssalonen@gmail.com> Co-authored-by: Nagy Attila Gábor <mrbig@sneaker.hu>

ssalonen added the work in progress A PR that is not yet ready to be merged label Jun 23, 2020

ssalonen requested a review from a team as a code owner June 23, 2020 17:55

ssalonen force-pushed the modbus-api-simplification branch from fce24c0 to e78cf01 Compare June 24, 2020 10:13

ssalonen changed the title ~~[WIP] [modbus] Modbus transport API simplification~~ [modbus] Modbus transport API simplification Jun 24, 2020

ssalonen removed the work in progress A PR that is not yet ready to be merged label Jun 24, 2020

cpmeister requested changes Jun 24, 2020

View reviewed changes

cpmeister mentioned this pull request Jun 24, 2020

[stiebeleltron] Initial commit #7718

Merged

ssalonen mentioned this pull request Jul 4, 2020

ModbusManagerInterface refactored to have separate result and failure callbacks ssalonen/openhab2-addons#5

Merged

ssalonen added 12 commits July 6, 2020 11:14

[modbus] exceptions moved to their own namespace

20fa36e

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

[modbus] read callback refactoring

f335ec1

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

[modbus] Write callbacks converted to Functional

2db2cd2

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

[modbus] Remove abstraction (unnecessary interfaces not meant for exten)

a0a39ed

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

[modbus] ModbusManagerAPI accepts arguments directly, no Task

9cb3e59

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

[modbus] ModbusManager indirection (Supplier) removed

14fec3c

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

[modbus] BitArray abstractions removed

722e76c

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

[modbus] ModbusRegister abstractions removed

4bb288b

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

[modbus] ModbusRegisterArray abstraction removed

b22a525

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

[modbus] Write API taking parameters directly

96a895a

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

[modbus] rename classes

6b12304

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

[modbus] Communication interface to track connection usage

5e18326

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

ssalonen added 4 commits July 6, 2020 11:25

[modbus] submitOneTimePoll simplification

80e7a5f

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

[modbus] Less verbose logging

8de2bd3

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

[modbus] Many null warnings removed

c65e3a6

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

[modbus] Fix: no need for a @nonnull annotation because it is default

735d9d6

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

ssalonen force-pushed the modbus-api-simplification branch from 1b4fbf9 to 735d9d6 Compare July 6, 2020 08:30

ssalonen added 2 commits July 6, 2020 11:36

[modbus] Removing unneeded Nullable, using final in immutables

34ade13

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

[modbus] Explicit functional interface

1a8df8d

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

ssalonen requested a review from cpmeister July 6, 2020 08:37

ssalonen commented Jul 6, 2020

View reviewed changes

cpmeister requested changes Jul 7, 2020

View reviewed changes

...modbus/src/main/java/org/openhab/binding/modbus/internal/handler/ModbusDataThingHandler.java Outdated Show resolved Hide resolved

...inding.modbus/src/main/java/org/openhab/binding/modbus/handler/ModbusPollerThingHandler.java Outdated Show resolved Hide resolved

ssalonen added 2 commits July 8, 2020 11:20

[modbus] @nullable and @NonNullByDefault aligned with coding conventions

00b7f76

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

[modbus] Collections.emptyMap instead of allocating new map every time.

6a46de1

Signed-off-by: Sami Salonen <ssalonen@gmail.com>

cpmeister approved these changes Jul 10, 2020

View reviewed changes

cpmeister merged commit 350ee95 into openhab:2.5.x Jul 10, 2020

cpmeister added this to the 2.5.7 milestone Jul 10, 2020

cpmeister added the enhancement An enhancement or new feature for an existing add-on label Jul 10, 2020

ssalonen mentioned this pull request Jul 26, 2020

[modbus] NullPointerException when read fails #8205

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[modbus] Modbus transport API simplification #7994

[modbus] Modbus transport API simplification #7994

ssalonen commented Jun 23, 2020 •

edited

Loading

ssalonen commented Jun 23, 2020

TravisBuddy commented Jun 23, 2020

TravisBuddy commented Jun 23, 2020

TravisBuddy commented Jun 24, 2020

TravisBuddy commented Jun 24, 2020

TravisBuddy commented Jun 24, 2020

fwolter commented Jun 24, 2020

ssalonen commented Jun 25, 2020

fwolter commented Jun 26, 2020

ssalonen commented Jun 27, 2020

ssalonen commented Jul 6, 2020 •

edited

Loading

ssalonen Jul 6, 2020 •

edited

Loading

TravisBuddy commented Jul 6, 2020

TravisBuddy commented Jul 6, 2020

TravisBuddy commented Jul 8, 2020

[modbus] Modbus transport API simplification #7994

[modbus] Modbus transport API simplification #7994

Conversation

ssalonen commented Jun 23, 2020 • edited Loading

ssalonen commented Jun 23, 2020

TravisBuddy commented Jun 23, 2020

Travis tests have failed

TravisBuddy commented Jun 23, 2020

Travis tests have failed

TravisBuddy commented Jun 24, 2020

Travis tests have failed

TravisBuddy commented Jun 24, 2020

Travis tests have failed

TravisBuddy commented Jun 24, 2020

Travis tests were successful

fwolter commented Jun 24, 2020

ssalonen commented Jun 25, 2020

fwolter commented Jun 26, 2020

ssalonen commented Jun 27, 2020

ssalonen commented Jul 6, 2020 • edited Loading

ssalonen Jul 6, 2020 • edited Loading

Choose a reason for hiding this comment

TravisBuddy commented Jul 6, 2020

Travis tests were successful

TravisBuddy commented Jul 6, 2020

Travis tests were successful

TravisBuddy commented Jul 8, 2020

Travis tests were successful

ssalonen commented Jun 23, 2020 •

edited

Loading

ssalonen commented Jul 6, 2020 •

edited

Loading

ssalonen Jul 6, 2020 •

edited

Loading