Add a way to access multiple devices on the same connection

[MartijnVdS]

I have a box that allows me to talk to multiple (micro) inverters over a single modbus-tcp connection, but that box doesn't handle more than one TCP connection at a time.

To query all inverters using the current API, my program would need to set up an AsyncClient (using AsyncClient::new(), wait for discovery, query the inverter parameters, then close the connection, and do all again for the next inverter.

If it then wants to query the first inverter again later, it needs set up a fresh AsyncClient (making sure no other clients are active, or the gateway box will not accept the connection), which runs discovery again, etc.

I think it would be useful to have a way to switch between inverters as part of AsyncClient (tokio_modbus has context.set_slave(Slave(device_id)) for this).

This probably requires some other changes -- like a way to track what the "active device" is, and a way to store discovery data for each device on a connection.

[MartijnVdS]

I've been looking into writing a patch to add this, but I'm not sure what the best strategy would be to implement it.

Idea 1:

• Create a new "multi device capable" version of AsyncClient (AsyncMultiDeviceClient or something) that stores its models and unknown_models in a HashMap keyed on device_id
• Give it a set_device(device_id) that calls discover_models if the device_id was not used before (= is not present in the discovery data hashmap).
• Need to ensure read_model is not called before set_device
  • One option would be to add an "initial device id" parameter to new() so a device_id is always set.
  • Another option would be to move read_models to a separate type that's returned by set_device (so calling read_models too early is not possible).

Idea 2:

• Adjust the current AsyncClient to store models and unknown_models in a HashMap, as in idea 1
• Add set_device to the existing AsyncClient.
• Add an optional argument to new() to set a default device id (with a default value). new() then calls set_device(device_id); this will break compatibility for existing callers who construct an AsyncClient with a client that's configured with a different device id (connect_slave(..., device_id)).
  • There's no way to read a good default value for the device id, because tokio_modbus clients have no way to query the current slave id, it's "write only".

[bikeshedder]

I don't really like the set_slave API of tokio_modbus.
Making this part of the AsyncClient API would be a very leaky abstraction.

To make this API pretty I think the following two steps are needed:

1. Change the tokio_modbus::Context wrapper into a Arc<Mutex<Context>> so it can be shared among multiple AsyncClient instances. This would also allow you to use the Context outside of the sunspec crate if ever needed.
2. Make AsyncClient aware of this and add a async fn device(&self, slave_id: SlaveId) -> Result<AsyncDevice> method that takes care of the discovery using the shared tokio_modbus::Context

Every call to read_model, read_point and write_point will first set the slave_id and then perform the actual operation.

I had a look at this and adding this is fairly simple. This adds another layer of indirection but since modbus is rather slow anyways and not used in high-performance code this doesn't really matter.

I've created a PR that adds this feature. Could you please give this a try?

• Add multi-device support #19

Just for the fun of it I also added a devices() method which runs a device discovery for all slave addresses (0..=255). I don't know how useful this is and it probably needs an additional timeout configuration, too.

[MartijnVdS]

This works for me.

While trying this, I had a separate problem, where it would not talk to one brand of inverter (APSystems) -- that seems to have been caused by it not responding to queries on address 0, which causes the Modbus request to time out, which in turn causes discover_models to stop trying the other configured DEFAULT_DISCOVERY_ADDRESSES.

[bikeshedder]

That's actually a bug in 0.7.0. When creating that AsyncClient interface I didn't handle the timeout case properly:

I've updated the feature branch with this fix: 144ce98

[MartijnVdS]

That makes it work -- at the cost of a timeout per connection :)

For reference, the Python implementation (from the SunSpec alliance's own GitHub organization) checks the base address in the following order: [40000, 0, 50000]

https://github.com/sunspec/pysunspec2/blob/master/sunspec2/modbus/client.py#L193

[MartijnVdS]

But if I want to do that in my own tool, I can just pass in a custom Config

[bikeshedder]

If the Python reference implementation uses [40000, 0, 50000] I think it's a good idea to switch to these defaults in this crate, too.

[bikeshedder]

I just released sunspec 0.7.1 on crates.io with the timeout fix and the modified discovery addresses:

📦 https://crates.io/crates/sunspec/0.7.1

This PR/branch has been rebased on the latest main and includes those fixes, too.