serkri / SmartEVSE-3

Smart Electric Vehicle Charging Station (EVSE)
MIT License
71 stars 27 forks source link

Feature request, read all modbus meters and also log voltage, cos phi etc #132

Open fluppie opened 1 year ago

fluppie commented 1 year ago

Currently I have a ESP32 with modbus shield from enri.nl to read all parameters / all modbusmeters. This gives 2 modbus masters which is far from ideal. I don't know if it's a lot of work to read out more registers than current and power as well as kWh import / export.

Screenshot of the ESP32 with ESPEasy software: afbeelding

The Nan values are because of modbus collisions. I think the SmartEVSE is requesting every 2 seconds data on the modbus. My mainsmeter is at 15s intervals and the PV and EV meter are at 60s intervals. To limit the number of collisions.

List with all available registers: https://github.com/reaper7/SDM_Energy_Meter/blob/master/SDM.h

afbeelding

Just a question, if not I keep using it like this. On the other hand I can save a ESP32, a 5V power supply and modbus collisions :).

dingo35 commented 1 year ago

@fluppie, perhaps you can test this version, it has one useless task removed and the modbus-handling task moved from low priority to high priority. It might make some difference, it might not.... master-c232e73d9d2.zip

fluppie commented 1 year ago

Hi @dingo35 I did some test with my second ESP32 module running ESP Easy, without this module and with HA live and shutdown as well as booting HA while logging.

putty_2ndModbusMaster.log 39 errors E0's, E2's, E4's putty_2ndModbusMaster_HAshutdown.log 37 errors E0's, E2's, E4's putty_no2ndModbusMaster.log 13 E0's putty_no2ndModbusMaster_HAshutdown.log only one E0 time-out putty_no2ndModbusMaster_HAshutdown_HAboot_runningLine4178.log Only one E2 before HA live, after HA booting (live from line 4178), 4 E0's

For the moment I only poll modbus registers that are not being polled by the SmartEVSE firmware. afbeelding afbeelding

Will do some testing with double polling those registers

dingo35 commented 1 year ago

Ok, basically the question is, without the 2nd busmaster, is this version an improvement to v1.6.5 as to modbus errors under heavy load.

From what I read not much difference?

hp197 commented 1 year ago

đź‘Ť

I also have a multi meter setup:

[mains] - [Eastron SDM630 Mains] -> [Distribution] -> [Eastron SDM630 EV] - [SmartEVSE]
                                         ^
                                   [Eastron SDM630 Solar] - [solar]

The problem I have is that I have to set SmartEVSE mains meter in all mode, to have the correct readings. But when I do, remote readout of the solar meter stops (after a restart of SmartEVSE). This is due to the MBbridge thread configured here: https://github.com/serkri/SmartEVSE-3/blob/master/SmartEVSE-3/src/evse.cpp#L2815

My current solution is to build my own patched version, with the 3 if's commented out. So that the MBbridge will always be created for all 3 meters.

hp197 commented 1 year ago

What would work for me (as resources are scarce on these devices), is by default open 5 bridges (for example modbus id 101 to 105 as always bridged to tcp). That way they dont have to be setup and used in SmartEVSE (but could if we need) and still query those devices external.

My best guess is that for most electrical utility cabinets, 5 modbus meters is about the max they will ever need.

dingo35 commented 1 year ago

tcpbridge.zip That would be one way to go, I went another way (since with your solution you can just wait until someone has a fixed modbus address meter etc.).

    //to bridge meter address (decimal)
    //activate tcp Modbus bridge, example:
    //curl -X POST http://smartevse-xxxxx.lan/modbus?bridge_meter_address=101
    //max 5 meters allowed, of which mainsmeter, evmeter and pvmeter might already be taken
    //TODO not sure what happens if you exceed this maximum, since attachServer always returns true....
    //has to be activated after every reboot

Please test thoroughly, especially what happens if you attach the 6th meter: does it simply not work, does it bump out one of the other meters?

dingo35 commented 1 year ago

@fluppie @hp197 Any progress on testing this?

fluppie commented 1 year ago

I think beginning of July I have some more time to test this version. At the moment I run the latest build with the Modem features.

dingo35 commented 1 year ago

tcpbridge_with_modem.zip @fluppie , if it helps you, this is current master (including Arend Jan's modem code) + the patch to test, so you can keep on using the modem stuff during testing....

hp197 commented 1 year ago

Hi sorry, I haven't got time yet to test it. That's mainly because I need to rework my data collector to send a http request to open up the bridge.

arpiecodes commented 1 year ago

I also recently 'migrated' my EV meter to modbus because of the addition of the TCP Modbus bridge (before it was pushing in the relevant EV meter details through MQTT on SmartEVSE from HA, which worked very well). Now I see a lot of bogus values appearing. E.g. big jumps in amperage values for the EV meter, big jumps in 'ChargePower' aka the power in watts being transferred into the EV, while nothing is flowing through.. It seems like some sort of corruption of the values or the checksum not being checked properly. Is this a known issue?

Screenshot 2023-07-06 at 13 02 01

Screenshot 2023-07-06 at 13 02 45

PS: I am using the Sensorbox as well, with my own ESPHome fork running on it. Its hardcoded on slave address 10, EV Meter is on 11. The Sensorbox values are coming in correctly; no weird stuff happening there.

PS2: Apparently only the values that get processed inside of EVSE get scrambled. It seems like the values bridged over by Modbus TCP bridge into HA are actually valid and do not contain the same jumps. Whut?

dingo35 commented 1 year ago

Thanks for testing, this is new info since the others in this thread didnt have time to test it.

If I understand correctly : Your sensorbox on modbus 10 gives good values in SmartEVSE and over the tcp bridge; Your EVmeter on modbus 11 gives strange values in SmartEVSE but correct values over the tcp bridge. Correct?

arpiecodes commented 1 year ago

I must not forget to mention that this is with the Modbus Bridge logic incorporated in the current master branch, not the custom build versions that were shared in this pull request thread.

Your sensorbox on modbus 10 gives good values in SmartEVSE and over the tcp bridge; Your EVmeter on modbus 11 gives strange values in SmartEVSE but correct values over the tcp bridge.

The Sensorbox is not read out through the modbus bridge (only by SmartEVSE) but directly connected to HA through ESPHome.

Since the whole reason I moved the EV meter from HA to SmartEVSE was the TCP Bridge functionality, being able to still read out all values in HA as well while keeping the EVSE stuff functioning independent from my WiFI/HA etc. So I am actually not sure whether this issue also occurs without using the Modbus bridge functionality.. I may have to do some testing soon to confirm that. I do see some Modbus communication CRC/timeout errors appearing from time to time, in the debug output on telnet. I do not use load balancing btw.

arpiecodes commented 1 year ago

Small update; somehow it fixed itself; I'm not getting weird values anymore through MQTT.. I'll keep an eye on it and report back if the situation changes again.

hp197 commented 1 year ago

Holiday season started, so again a bit more time for other projects :)

I tested the code, it works but it is lacking the feature to get the current status. I have a custom written python application who read the modbus data and put them into mqtt and expose them in prometheus. You’ll have no idea at all if the activation of the bridge is already done and the only solution to this is to spam the url endpoint every x time.

I have 3 concurrent meters and works.

dingo35 commented 1 year ago

Ok thx for testing!

arpiecodes commented 1 year ago

Small update; somehow it fixed itself; I'm not getting weird values anymore through MQTT.. I'll keep an eye on it and report back if the situation changes again.

Sadly it wasn't fixed; my HA installation just wasn't querying the modbus bridge TCP server. After I 'fixed' this and started requesting all SDM630 values again, the same issue came back. It seems to directly garble the information it gets from the SDM630, as if requests for the same register are colliding or something, resulting in weird measurements.

I guess I can try removing the 'double register' reads from HA and see if that fixes the issue (trust the bridge will then forward all incoming data from the SmartEVSE's own requests to HA as well).

dingo35 commented 1 year ago

I would appreciate it if you could test disabling the "double register reads"!

arpiecodes commented 1 year ago

I would appreciate it if you could test disabling the "double register reads"!

This didn't work sadly. For now I've resorted again to disabling the readings from HA. It's been stable ever since again.

dingo35 commented 1 year ago

This is strange, since the HA readings (both MQTT or REST API) dont force any extra readings from the modbus.

It looks like some overload (memory and/or cpu) that is not happening when reading directly through modbus tcp...

arpiecodes commented 11 months ago

I also noticed quite some errors (timeouts, CRC mismatches) inside of the telnet debug output. It indeed looked to be some kind of overload happening. The errors occurred at both the SDM630 as EV Meter and the Sensorbox v2 as MainsMeter (even though it runs my custom ESPHome FW). I also noticed the Sensorbox being slow and noticing the DSMR component calls were stalling the loop with 0.2s every time it was updating it's values.

Knowing this, I did a couple of things;

Overall, all these things seem to result in a much more stable, happy modbus climate for the EVSE. Even after re-enabling the modbus TCP bridge and having HA query the SDM630 through the EVSE, the readings do not contain the weird value spikes anymore. So I'd call this success.

Now, since this touches some internals (e.g. extra task created, modbus value request loop slowed down, etc) I am a bit unsure if I should publish it as a branch yet until I've done some more testings. So I'll report back next week whether my issues came back or if it's definitively solved now. I do not have slave EVSE's nor use the load balancing feature, so maybe someone who does could test the 'improvements' in their set-up before we actually consider merging it.

dingo35 commented 11 months ago

Sounds great, but your third point worries me: how would these changes impact other users with "standard" sensorbox v2 firmware?

Also: the stalling of 0.2s occurs within the sensorbox with custom firmware, or within the SmartEVSE loop? If its within the sensorbox, who cares, because this is the primary and only task of the sensorbox; and if you would care, is it possible to solve this delay (it shouldnt be memory or cpu intense to get that dsmr value).

So when the dsmr freq is reduced from 2s to 5s, now the SmartEVSE reads that value every 200ms, so reading the same value 25 times?

Since the problem of "strange values" arised after I places the MQTT publish-calls in the updatecurrentdata routine, and diminished when I placed a mutex, I am looking at replacing the MQTT library with a threadsafe version, because I think that will solve the root cause of the problem. And that would give us the opportunity to move more publish-calls to places where variables are changed (as intended by MQTT philosophy), instead of publishing every fixed time-interval.

I cannot test anything until december, and will be reluctant to merge any major changes I couldnt test myself...

arpiecodes commented 11 months ago

Sounds great, but your third point worries me: how would these changes impact other users with "standard" sensorbox v2 firmware?

It shouldn't. Since SmartEVSE is the master on the modbus, it will query the Sensorbox when it sees fit. The standard Sensorbox firmware has a wildly different way to get/parse the specific values from the energy meter (custom build, just a couple of values). It'll just respond if asked.

Also: the stalling of 0.2s occurs within the sensorbox with custom firmware, or within the SmartEVSE loop? If its within the sensorbox, who cares, because this is the primary and only task of the sensorbox; and if you would care, is it possible to solve this delay (it shouldnt be memory or cpu intense to get that dsmr value).

This has been changed in my custom ESPHome firmware, so on the Sensorbox and not on the EVSE. Changing the frequency of polling the actual data doesn't affect SmartEVSE in any way. The ESPHome version of DSMR polls and processes all of the values from the Smart Meter (not just 5), so it takes some time to complete it's job which is why it stalls the loop for 0.2s. This normally wouldn't be a problem if you use multi-threading on an ESP, but the developers of ESPHome are convinced one core is all you will ever need with ESPHome and thus give no option to actually utilise the second thread for anything (except internal stuff such as WiFi etc). So this is more an ESPHome thingy than anything else.

So when the dsmr freq is reduced from 2s to 5s, now the SmartEVSE reads that value every 200ms, so reading the same value 25 times?

That isn't the case since the SmartEVSE uses some fancy loop that queues up multiple readings and kind of iterates over them. In the original 100ms timer it states it will query the meters every 2 seconds. So using a 200ms timer would logically result in a request every 4 seconds. I suppose I could also try to have the ESPHome Sensorbox update it's values every 4 seconds, see how that fares. I think it mostly was struggling with the Modbus readouts/calls every 2 seconds while also doing a DSMR readout every 2 seconds. Sometimes coinciding and causing delays due to the stalls the DSMR component was causing when reading out it's values. They're both using Serial IO (modbus + readout) which can be blocking, so that makes sense.

But above probably doesn't do much for the strange value issue, as that seems to be happening inside of the SmartEVSE, not the Sensorbox. It may well explain the timeouts I was seeing at requests for Sensorbox values (but that'd be very specific to my own use-case).

Since the problem of "strange values" arised after I places the MQTT publish-calls in the updatecurrentdata routine, and diminished when I placed a mutex, I am looking at replacing the MQTT library with a threadsafe version, because I think that will solve the root cause of the problem.

I see. I didn't even think of this yet, sounds like the proper fix indeed.

I cannot test anything until december, and will be reluctant to merge any major changes I couldnt test myself...

Totally get that! No worries. I will be using this custom version for myself as it seems to fix my issues. I agree with you that we shouldn't merge anything that can't properly be tested.

The mentioned changes are found here btw; https://github.com/arpiecodes/SmartEVSE-3/pull/7/commits/0205b73ffc9e9c1c50b767a9ff272588157935cf. Not saying this is the proper solution for now or that we should merge it as such, but just to give an indication of what I've done here.

dingo35 commented 11 months ago

@arpiecodes I found this MQTT library that claims to be thread-safe: https://github.com/cyijun/ESP32MQTTClient

I don't think it'll take too much effort to move the library to this one and give it a test, after removing the mutex's I placed in the code.

fluppie commented 11 months ago

Nice, curious of the outcome. I did "discover" that the bogus values are sometimes kWh meter reading or voltage etc. So it's maybe a timing error? That it receives a register value from before or after the one it thinks it's requesting?

dingo35 commented 11 months ago

@fluppie That is very interesting, it supports the theory that the non-threadsafeness of the library causes the problem...

arpiecodes commented 11 months ago

Sad to report my code changes didn't completely alleviate the 'weird values' issue. They did help for the timeout/CRC modbus errors.

One thing I do not quite understand; why would the addition of the MQTT client be the issue? Since it is not really writing anything incase of EVMeter/MainsMeter being received through modbus, just reading them out (and then sending out to the MQTT broker). UpdateCurrentData is not called there if there was no received messages/commands over MQTT. Correct me if I'm wrong.

I do see the issue when you have multiple threads triggering the UpdateCurrent data, but then still it's weird it's only happening to modbus received values. Wouldn't logically the mutex be placed on the modbus calls/logic instead?

To me it'd make perfect sense if the collision would happen in the case the BridgeModbus connected client requests a value from the meter while SmartEVSE also requests it at the same time (or any situation where multiple requests are done for whatever reason). I'd imagine that to be a very weird situation for the meter to handle and it would probably cause issues. This gets supported by the observation that it only seems to happen (for me at least) while having the ModbusBridge in use reading out the same meter. I suppose because I lowered the interval for the SmartEVSE readouts from 2s to 4s, it made the issue less frequent, but didn't fully solve it.

dingo35 commented 11 months ago

The modbus logic (and protocol) is supposed to be thread safe; in the protocol the master sends data requests (through a single routine that "rotates" all the different meters/functions), and the (single) receiving routine sorts them out. So the only collisions would be when the "rotating sending routine" is called before the last call was finished. You took care of that possibility by putting that in a 200ms loop.

Also, AFAIK the problem only arises with the MQTT interface, not with the REST API. And then it only appeared when I put the MQTT publish current data call in the updatecurrents routine.

So in my view the strange values have to do with the MQTT library. Timeouts and CRC errors on the modbus would result in not updating the data requested, NOT in invalid data. So reducing timeouts and CRC errors is always a good thing, but IMHO another subject.

arpiecodes commented 11 months ago

The modbus logic (and protocol) is supposed to be thread safe; in the protocol the master sends data requests (through a single routine that "rotates" all the different meters/functions), and the (single) receiving routine sorts them out. So the only collisions would be when the "rotating sending routine" is called before the last call was finished.

I see how it's thread safe for sending/receiving parts, and the part of SmartEVSE requesting those values by itself. But; what does the ModbusBridge have to bring to the table here? It's not part of that routine rotation method inside of the SmarEVSE, but requests values on it's own behalf (as requested by its clients). The single routine inside of the calls made by SmartEVSE itself won't help there.

Also, AFAIK the problem only arises with the MQTT interface, not with the REST API.

How did you test/verify this? MQTT is obviously very easy to log values and see the spikes as they occur. Through API it's probably a lot harder to catch them (but I still saw them happen there as well - just didn't try with MQTT logic disabled/removed).

dingo35 commented 11 months ago

True! (as to the bridge).

REST API: I stand corrected, I didnt know strange values were detected there too!

fluppie commented 11 months ago

Here you can see voltage being logged as current. Doesn't happen often, only a few times per week. But I'm polling "only" every 15s instead of the SmartEVSE every two seconds or so.

afbeelding

arpiecodes commented 11 months ago

Screenshot 2023-10-02 at 17 27 57

Interesting to see in my case it's always the value 2387W, which seems to (in this case) match the total KWh charged since reset;

Screenshot 2023-10-02 at 17 33 29

EDIT: Yep, I also confirmed the earlier spikes are showing 2386W with contactors closed and KWh since reset being 2386.x.

dingo35 commented 8 months ago

I found a threadsafe MQTT library, that should/would solve the strange outlier values on the API. It doesn't use the PubSubClient underlying library, but another, ESP-IDF library that looks a lot more reliable, although it is pretty expensive in flash ram usage: threadsafe_mqtt.zip

@arpiecodes @fluppie Could you test whether this solves /diminishes the problem?

dingo35 commented 8 months ago

Ok Scatman_II on tweakers reports still problems, this is a version on the old library where the current-publishments are synchronized with the other publishments: publish_synced.zip