Replace modbus driver with modbus-tk *Discussion*

[acedrew]

I'm making this issue to solicit discussion regarding removing duplicate modbus drivers, and discussing new features that may need to be added to work in every use case, as well as discuss desired direction for project.

Description of Issue

VOLTTRON as a project has a lot of fragmentation in how people use the platform, and generally leaderships' position has been that they're all correct. While this position certainly avoids alienating established users, I believe a more cohesive, clear path for new users will enable better ecosystem growth.

The modbus vs modbus-tk driver is an example of the kind of choice a new user shouldn't have to make, they're already stuck with the unenviable task of selecting a message bus, then they have to read the docs for two different modbus drivers, with no guiding direction about which one is better supported by the community.

Recently certain issues have been raised (though not filed here? please reference the issue if I missed it) that the modbus-tk driver is lacking certain functionality that the modbus driver has. I've recently been working extensively with the modbus-tk driver and am already looking at changes in the core client behavior to support new functionality, so I'd like to gain input here on what else needs to be added/fixed in order for the modbus-tk driver to fully replace modbus.

I'm also of the opinion that we should seek to remove the Master/Slave terminology from our implementation of the Modbus communications, we can easily provide backwards compatibility for configurations, and a note in the documentation defining our replacement terms and historical context. The OpenSource Hardware Association has recently made similar changes for their documentation of the SPI Bus https://www.oshwa.org/a-resolution-to-redefine-spi-signal-names

Currently I'm working on/aware of the following:

• Multi clients connecting to multiple peripherals on the same bus (assuming via TCP?)
• Extracting/Setting bit flag values that are stored outside of the Boolean registers (with minimal peripheral requests)
• Padding registers to allow requests of non-contiguous regions to be consolidated into single requests

[acedrew]

On the padding registers, you think it's more useful to have padding registers or a tolerance in the device config for contiguous? Right now the driver checks if the registers are contiguous, and if not, starts a new request, it might be easier to have it check that the gap between registers is not more than config.contiguous_tolerance and if not, fill the request with padding at that stage.

[acedrew]

This has become an active topic for us again. Looking at the structure of both modbus and modbus-tk, the only reason why the modbus driver can talk to multiple devices behind a single ModbusTCP gateway is that it's not reusing connections at all, that means there's a TCP handshake for each device behind the gateway, because it closes the connection after reading each one.

To either improve the performance of modbus or enable this Very common use case for modbus-tk, we need a way of creating socket singletons shared across the entire platform.driver, or we have to completely refactor into something like bacnet_proxy. Thoughts from the team here? I'm assuming I could create a default dict in the platform.driver that gets passed in to all interfaces as a kwarg, with (ip,port) tuple keys and socket instance values, the defaultdict could have a custom init that just creates the socket if it doesn't exist. Is there a pattern that's already being used in the platform for connection sharing that I should look at instead? There would need to be locks as well.

[acedrew]

Thinking about this more, I think trying to use modbus-tk on serial bus devices will see similar issues, if it's trying to create a handle on the serial file for each device/interface instance?

[shwethanidd]

@acedrew: We would like to discuss this further. @rlutes reported problems with modbus-tk driver as well and we want to know if it's the same and discuss possible fixes.

[bonicim]

This has become an active topic for us again. Looking at the structure of both modbus and modbus-tk, the only reason why the modbus driver can talk to multiple devices behind a single ModbusTCP gateway is that it's not reusing connections at all, that means there's a TCP handshake for each device behind the gateway, because it closes the connection after reading each one.

To either improve the performance of modbus or enable this Very common use case for modbus-tk, we need a way of creating socket singletons shared across the entire platform.driver, or we have to completely refactor into something like bacnet_proxy. Thoughts from the team here? I'm assuming I could create a default dict in the platform.driver that gets passed in to all interfaces as a kwarg, with (ip,port) tuple keys and socket instance values, the defaultdict could have a custom init that just creates the socket if it doesn't exist. Is there a pattern that's already being used in the platform for connection sharing that I should look at instead? There would need to be locks as well.

I was to able to reproduce this scenario where two drivers pointing at the same IP address does not work as expected; see #2953. As for your solutions to this problem (e.g. creating socket singletons accessbile across the entire platform.driver, or refactoring PlatformDriver to be like bacnet_proxy, I second @shwethanidd's idea of setting a up a meeting to discuss the issue and potential solutions.

[acedrew]

I was able to find a working solution, though not a particularly performant solution via the following steps

1. Pass the threadsafe=True when initializing TcpMaster
2. Create a thread/process wide lock dictionary, storing a gevent.lock.BoundedSemaphore for each target (host, port) tuple.

When using threadsafe=True Modbus-TK is meant to use a global lock to prevent communications happening out of order. Unfortunately, the structure of the modbus_tk interface in volttron breaks the execute calls up in such a way that devices requests get interleaved in a non-deterministic way, further when using this option by itself, without the lock dictionary, it still frequently resulted in conflicts and failed reads. Also, the global lock does not care if communications are happening with multiple devices, forcing everything into synchronous flow.

By using the lock dictionary, it also forces calls to happen synchronously, but per client socket vs globally. However, there were still issues when using this method without threadsafe=true in the TcpMaster unit, this needs a bit more testing, I'll take a deeper look this evening actually.