Feature Request: Add support for modbus tcp (client)

[marcovaneck]

Some meters have a beside a Modbus-RTU connection, also a Modbus-TCP option. Having this option would save time to pull cables/rely on other components, or simply buy an off the shelve modbus-rtu-tcp converter at the place of the meter.

During my quick investigation it seems some refactoring is required to make this possible. Of course most in the modbus area, but some concepts need to change as well. For example the address of a meter can be 1, the same as the loadbalancer (SmartEVSE master). So it should become the address of a ModbusClient, not complex but will make the change bigger.

How does the maintainer(s) look at a change like this?

[dingo35]

It all depends on the amount of change in relation to the actual gain of function it brings.... and also on the usage of resources. Look at the style of coding and see if you can appreciate joining it... if so make a PR , and I will be happy to look at it.

[dingo35]

O, and when a meter is so inflexible it doesnt even have the flexibility to change a meter address, it is probably not worth investing (time or money) in such a device....

[marcovaneck]

@dingo35 I've made my first refactor, can you have a quick look if it goes in the right direction. For now I didn't create a pull-request since it doesn't add new functionality. You can see the changes on Changes

btw. I didn't find a way to change the ID of my SolarEdge Inverter when it's connected via Modbus-TCP. on RS485 it isn't a problem.

[dingo35]

Well this is a lot of change and no gain of function, so that is not a good sign :-) .

But without kidding, I'm not sure what you are trying to accomplish but it looks like you are messing up every modbus function call by adding this container parameter, without adding something.

I think (but again, not sure what you are trying here) I would go the exact opposite direction:
#ifdef MODBUS_TCP
ModbusClientTCP MBclient(whatever_params_are_needed);
#else
ModbusClientRTU MBclient(PIN_RS485_DIR);
#endif

or a runtime variant if you want to have both RTU and TCP enabled (if the library permits that?!?)....

[marcovaneck]

I want to support both mechanisms at the same time. Due to the setup of the code (globals) I couldn't reuse the existing the modbus functions, that's why I introduced the container parameter. Now I can start adding the tcp variant, and simply use the same modbus functions. By doing this all new modbus features will be available for both implementations.

[dingo35]

I would lookup the low level send and receive functions, and duplicate those calls for both RTU and TCP clients. Then you only add a few lines of code and a lot of functionality.....

[marcovaneck]

Unfortunately that would mean I need to duplicate all modbus related methods. I installed the 'modbustcp' branch and noticed some occasionally strange readings from my sdm630 modbus-rtu ev meter. The logging I do via MQTT, due to time-limitations I installed the normal 1.7.3 version (the compiled version from github). Strange enough I still receive strange readings occasionally. Note: before I ran a custom version with all MQTT calls in the mqttPublishData() method (without the std::lock_guardstd::mutex lck(pub_mtx); fix)

For now I'll continue in this direction, and try to investigate/fix if there is still and MQTT problem.

[dingo35]

Im not aware of any modbustcp branch, the modbustcp bridge stuff is in the master branch ?!?

[marcovaneck]

Sorry, the branch is still on my fork, https://github.com/marcovaneck/SmartEVSE-3/tree/modbustcp I can create an Pull Request of it, but not everything is finished.

[arpiecodes]

Unfortunately that would mean I need to duplicate all modbus related methods. I installed the 'modbustcp' branch and noticed some occasionally strange readings from my sdm630 modbus-rtu ev meter. The logging I do via MQTT, due to time-limitations I installed the normal 1.7.3 version (the compiled version from github). Strange enough I still receive strange readings occasionally. Note: before I ran a custom version with all MQTT calls in the mqttPublishData() method (without the std::lock_guardstd::mutex lck(pub_mtx); fix)

For now I'll continue in this direction, and try to investigate/fix if there is still and MQTT problem.

Are those strange values very big spikes in values suddenly appearing from a (for example) modbus attached EV meter? If so, I also saw those weird values while I also read out the values using HA through the Modbus bridge, and I also had no idea what it was. I am testing some adjustments I did to the code inside of SmartEVSE with regards to modbus tasks and the frequency of polling the EV meters to see if that helps. Seems to work good so far. But I need a couple of days before I know for sure.

[marcovaneck]

Are those strange values very big spikes in values suddenly appearing from a (for example) modbus attached EV meter? If so, I also saw those weird values while I also read out the values using HA through the Modbus bridge, and I also had no idea what it was. I am testing some adjustments I did to the code inside of SmartEVSE with regards to modbus tasks and the frequency of polling the EV meters to see if that helps. Seems to work good so far. But I need a couple of days before I know for sure.

Yep, the spikes are appearing from my SDM630v2 EV meter. The incorrect readings are easy to find in my situation, l2/l3 are not connected so should always by 0, but sometimes they are 2147483600/1000/600. It can also be an implementation fault inside the the EV meter, but there is no way to check this. If the polling frequency change doesn't solve the issue, you might try to stick the ModbusClientRTU and ModbusBridge to the same core.

My hardcoded ModbusTCP PV meter is working without issues. Except my assumption the SolarEdge EMConfig is only the Sunspec's Meter, not the full sunspec.
Unfortunately there are other projects taking up my time at the moment.

[hp197]

No, this is known behaviour for SDM meters. I've multiple and even if you read them directly they sometimes give these values. Let say (the firmware of) SDM meters don't seem to be of the best quality. I think it would be best to ignore readings above spec+10% (64A + 10% = ~70A).

[arpiecodes]

No, this is known behaviour for SDM meters. I've multiple and even if you read them directly they sometimes give these values. Let say (the firmware of) SDM meters don't seem to be of the best quality. I think it would be wise t ignore readings above spec+10% (64A + 10% = ~70A).

I don't think ignoring incorrect readings is the way to go, since there are a lot of phantom readings that look to have sane numbers for the input they're being read for. Maybe we can make some sort of 'expected value' feature that 'knows' which register we last attempted to read out, and only accepts a value if that same register returns a value next. But that may not actually solve anything.

And even then, the modbus TCP bridge doesn't really care what gets requested when and how, as this is not under the EVSE's control. The only way to prevent double calls (registers being read simultaneously or too rapidly) would be to lock the modbus for the time SmartEVSE is querying it, so the bridge cannot query it at the same time for the same device address. That may just work.

So maybe it's a matter of reducing load on the SDM for readouts, somehow making it happy. When I had it directly connected to my HA with a sane scan interval, and when only connected to SmartEVSE, there were no such phantom values. So whatever it is, I do think it's caused by some higher load scenario where maybe too much is requested at the same time and the SDM cannot cope.