helgeerbe / OpenDTU-OnBattery

Software for ESP32 to talk to Hoymiles Inverters and Victrons MPPT battery chargers (Ve.Direct)
GNU General Public License v2.0
263 stars 56 forks source link

http power requests too frequent #579

Closed Fribur closed 5 days ago

Fribur commented 6 months ago

What happened?

Before elaborating here in GitHub: Discord chat might be more efficient, but not sure which handle/channel I can use?

To the bug: I configured "Individual HTTP requests per phase" for a Shelly EM, and have a hunch the Shelly EM cannot keep up with this bombardment of request as I get relatively frequently a [HttpPowerMeter] Getting the power of phase 1 failed. Error: Error(http://192.168.1.94/emeter/0): connection refused. Every once and a while I also get [HttpPowerMeter] Getting the power of phase 1 failed. Error: Error(http://192.168.1.94/emeter/0): read Timeout, followed by getting spammed with PowerMeter Timeout! 30952 ms ...To see this timeout I made the following Log change:

    if (millis() - PowerMeter.getLastPowerMeterUpdate() > (30 * 1000)) {
        Hoymiles.getMessageOutput()->printf("PowerMeter Timeout! %ld ms\r\n", millis() - PowerMeter.getLastPowerMeterUpdate());
        //shutdown(Status::PowerMeterTimeout);
        return;
    }

All the while I can issue the same command via Google chrome on my computer and get a response just fine. Is there a way to limit the request to e.g. 1 per second (1st sec = phase 1 request, 2nd sec = phase 2 request etc pp)? I see config.PowerMeter.Interval in the code, but cannot see it in the webinterface? Would this interval also space out the requests per phase (so that the poor Shelly EM does not have to deal with 2 request within milliseconds)?

To Reproduce Bug

Configure "Individual HTTP requests per phase" on a Shelly EM, and query phase 1 with http://yourIP/emeter/0 and Phase 2 with http://yourIP/emeter/1 (JSON path = power)

Expected Behavior

no "connection refused" or timeouts for any of the phases as I am not getting any of that when using on my computer Google Chrome for issuing the same command (even if I re-send twice per second) [HttpPowerMeter] Getting the power of phase 1 failed. Error: Error(http://192.168.1.94/emeter/0): connection refused

Install Method

Self-Compiled

What git-hash/version of OpenDTU?

08bc181

Relevant log/trace output

No response

Anything else?

No response

Fribur commented 6 months ago

in addition to the changes described in https://github.com/helgeerbe/OpenDTU-OnBattery/issues/585, I noticed the function getPowerTotal(bool forceUpdate=True) is responsible for quite a few unnecessary requests to the PowerMeter that are ignoring the 10 second config.PowerMeter.Interval. The following changes reduced drastically the sporadic "connection refused" bug and quite a bit of spam in the console:

PowerLimiter.cpp, line 434:

-int32_t newPowerLimit = round(PowerMeter.getPowerTotal());
+int32_t newPowerLimit = round(PowerMeter.getPowerTotal(false)); //why should this loop force readPowerMeter() update when the PowerMeter loop is doing this already?

PowerMeter.cpp, line 158:

-MessageOutput.printf("PowerMeterClass: TotalPower: %5.2f\r\n", getPowerTotal());
+MessageOutput.printf("PowerMeterClass: TotalPower: %5.2f\r\n", getPowerTotal(false)); //getPowerTotal(true) would trigger readPowerMeter() again-->why? was just called

I still get infrequently a connection refused for the second phase. Could this be an issue with creating WiFiClient and HTTPClient for each requests in HttpPowerMeterClass::updateValues() in a loop? I will try tomorrow to NOT query each phase in the same HttpPowerMeterClass::updateValues() cycle, but rather just 1 phase per update. Alternatively, I strongly suspect all Phases have mostly the exact same IP address, so maybe I try also to recycle the HTTPClient and WiFiClient instead of creating and destroying new ones for each phase.

spcqike commented 6 months ago

Forceupdate only runs once every 1000ms, so calling getPowerTotal() twice should only forceUpdate once.

As powermeter loop runs once every 10s (or configured interval) and the powerLimiter loop runs more frequent, powermeter loop shouldn’t trigger an update at all.

The idea is to have get a powermeter value as fresh as possible. That the calculation of powerlimiter is as accurate as possible. If you stop force updating your powermeter value from within powerlimiter, it may be that your powermeter reading is up to 10s old.

This only applies for https+json, SDM1 and SDM3 as only these can be queried from openDTU-onBattery

Fribur commented 6 months ago

Thanks for the explanation, makes sense.

Still, this 1000ms limit does not apply to the double whammy in powermeterclass::loop() as _lastPowerMeterCheck is updated after the MessageOutput. Here instead of a re-cursive call to same function just for a console log entry one should really just calculate the sum of the power values that where just established, or do the recursive call with (false). This double whammy triggers right now quite a few of the “connection refused“ for me.

readPowerMeter();

MessageOutput.printf("PowerMeterClass: TotalPower: %5.2f\r\n", getPowerTotal());

mqtt();

_lastPowerMeterCheck = millis();
schlimmchen commented 6 months ago

The whole PowerMeterClass is in dare need of a refactor. You guys proof it by having a discussion about when the HTTP request is actually send (forceUpdate flag) or not. The specific ways of getting a power meter reading are vastly different, but all of them are mangled into this once class, leading to all sorts of problems.

Here instead of a re-cursive call to same function just for a console log entry one should really just calculate the sum of the power values that where just established

Yes. And once there is a more abstract version of PowerMeter with the respective specific provider that collects the values, nobody would do it like this.

spcqike commented 6 months ago

_lastPowerMeterCheck is updated after the MessageOutput

no, not _lastPowerMeterCheck but _lastPowerMeterUpdate, which is updated within readPowerMeter() itself.

https://github.com/helgeerbe/OpenDTU-OnBattery/blob/2c1e145575a5030b96165a45d6fdf97b8832c607/src/PowerMeter.cpp#L193-L200

so getPowerTotal() should skip the second run of readPowerMeter()

schlimmchen commented 6 months ago

no, not _lastPowerMeterCheck but _lastPowerMeterUpdate, which is updated within readPowerMeter() itself.

One more very confusing implementation detail that warrants a refactor by itself.

ottelo9 commented 3 months ago

I also just saw that the values from my meter (Tasmota) are not queried at the set interval, but almost every second. If I switch off DPL, the query only comes at the set PowerMeter interval (for me it was set to 27s for testing).

Is there any workaround for this problem?

If I activate DPL, the query first comes every 27s, then suddenly every 1-3s. Tasmota is flooded with queries. image

Logs: opendtu https://pastebin.com/RWpvsBQw

tasmota (StatusSNS) https://pastebin.com/19yahXcq

2024.03.23

spcqike commented 3 months ago

Is there any workaround for this problem?

why would you call it a problem? Does it make any trouble?

IMO it’s a feature. You don’t need to wait 27s (or 10s as standard interval) to get the actual power meter reading. Imagine you switch off your electric kettle and still produce 2kW out of your battery for another 25s (or 8s) because your last power meter reading is 2s old. Wouldn’t this be such a waste and a real problem?

ottelo9 commented 3 months ago

1) I have two opendtu and both try to get the meter values from my Tasmota device (ESP8266). Tasmota send the values to my HA device via MQTT, too. And I run a big Script on Tasmota with graphs etc. I don't know if it's a good idea to stress the little ESP like that. 2) As I wrote in this Discussion https://github.com/helgeerbe/OpenDTU-OnBattery/discussions/246#discussioncomment-8676506 I have two opendtu (one with battery) and if the power consumption is stable, no problems. But with clouds and often change between sun and shade then the DPL runs crazy. Therefore I set the powermeter intervall of my second opendtu to 27s. My idea was: The first control works fast and the second very slowly. So it can't start swinging. But then I read (and see my logs) that the DPL ignores the powermeter interval? Do I have to change the DPL intervall to 27s?

spcqike commented 3 months ago

I don't know if it's a good idea to stress the little ESP like that.

that’s what it’s made for. A server or device with <80% utilization is oversized for its purpose / use case.

What’s the loadavg of your Tasmota? Even if that’s not necessarily the real utilization, but it’s some point to start and look at.

As long as your Tasmota doesn’t make any trouble, like not answering or acting weird, missing mqtt messages or whatever, I wouldn’t mind querying it as often as I want to.

ottelo9 commented 3 months ago

Ok thanks for that hint, good point.

Tasmota Loadavg 39-70%. Ok enough power for this...

schlimmchen commented 2 weeks ago

Please have a look at #1077. All PowerMeter providers now run on their own, adjustable schedule. The DPL will not request new values in between.