RAPI update loop is painfully slow

[openevse]

RAPI update loop is painfully slow

[chris1howell]

Hi Eric, The reason RAPI was slowed down was the web interface was extremely unresponsive and unstable. I was attempting to limit RAPI hogging the loop. The only real limitation is to wait 100ms between request and responce. I am open to any optimizations... Chris

…

On May 1, 2017 12:55 PM, "Eric Sandeen" ***@***.***> wrote: When the main loop calls update_rapi_values() every 5s it iterates very slowly through each call: wait 5s, call Issue new RAPI cmd wait 5s, call Parse last RAPI cmd wait 5s, call Issue new RAPI cmd wait 5s, call Parse last RAPI cmd etc. The main loop has a timer to call it once every 5s, so this means it takes 10s for each RAPI call to be made and and then read/parsed. There are currently 4 RAPI calls made, and that's before my pending patch to add 2 more (to keep energy & fault counts up to date). This behavior was introduced by 7cf4a90 <7cf4a90> fix firmwarware update by slowing down RAPI updates to 5s. The prior behavior of call/parse/call/parse iteration was done by Chris with: d1d6c2e <d1d6c2e> Significant improvement to RAPI read/write write/wait 5s/read/wait 5s/write ... surely seems slower than necessary. What are the limitations here? Clearly delay() for serial comms is undesirable, but 5s intervals must be overkill. Glyn mentioned that 7cf4a90 <7cf4a90> was to fix (unspecified) problems with http firmware updates. Some elaboration on that problem would be nice, but perhaps a better solution would be to suspend RAPI updates during the rare case of a http firmware update? Thanks, -Eric — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#5>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABvYH7_UCLZ8XZSYKh7_NP6XcG1OoWzOks5r1ji0gaJpZM4NNXub> .

[sandeen]

Thanks Chris!

So, originally with your modification, update_rapi_values() was called unconditionally in the main loop, but immediately returned if < 1s had elapsed.
int comm_Delay = 1000; //Delay between each command and read or write
So the read, write, read, write cycle worked with a 1s delay.
Glyn later modified the main loop so that it was called once every 5s, citing interference w/ http updates. Maybe we need better understanding of what was seen there. Maybe the overhead of constantly calling a function which immediately returned?

Chris, one other thought I had was the ability to send batch RAPI requests, i.e.
$G1 $G2 $G3 $G4 $G5
and get back a string of all the results for all commands:
$OK AA BB CC DD EE FF GG HH II ...
and that would eliminate lots of the serial overhead, but that would also require changes on the EVSE controller side, so may not be the best solution.
It's a pity that the ESP doesn't seem to have implemented serialEvent. :(

[glynhudson]

I'm happy for 7cf4a90 to be reverted. I did not realise the effect this would have on RAPI updates. I presumed that calling update_rapi_values() once every 5s would update the values every 5s, I had not appreciated that it takes multiple calls to update and pase the values.

Any idea how we could pause rapi update when a http upload is triggered?

Caution: I accidentally included another addition in 7cf4a90 (yes..I know sloppy) of adding WiFi.hostname("openevse"); to wifi.cpp this renames the hostname to be 'openevse' enabling access over a local network using http://openevse which is very useful. I would like to keep this change.

[sandeen]

Right now the main loop does:

    if ((millis() - Timer3) >= 5000){
      update_rapi_values();
      Timer3 = millis();
    }

so I was going to just say we could bump Timer3 out by a minute or two when an update is triggered, but then I realized that's all encapsulated in the httpUpdater library and may not be under our control. I wonder if another local server.on("/update" ...) in src/web_server.cpp could catch that or if that would wreak havoc with the udater. Could test it I guess. :)

[sandeen]

@glynhudson so what was the failure mode that you saw during the http update which prompted the longer loop?

[glynhudson]

The update file upload would get so far then hang (sometimes 60% sometimes 80 or 90% upload). I assumed this was because RAPi update interrupted the update upload. Slowing down the RAPI update seemed to fix this. I only tested once or twice but update seemed to work every time after the change. Although I only tested a couple of times.

[sandeen]

looking at @lincomatic 's RapiSender class sent offline ...

Doesn't it still place the wait-for-serial time directly in the main loop?
At least it's not a hard-coded delay() time, but it still has a :

  do {
...
  } while (!_tokenCnt && ((millis() - mss) < timeout));

loop in sendCmd, so it's going to keep loop() busy for min(serial_response_time, timeout) each time we call it.
At a high level, can't we just:

loop()
  if X seconds elapsed in millis()
    RapiLoop
      Serial.flush() // flush output buffer, hopefully empty by now anyway
      parse_last_RAPI
        // tokenize all prior responses and set variables
      launch_all_RAPI
        // send all RAPI commands in sequence, no parsing, no waiting
        // these will go to the serial output buffer and we return quickly
      return

and that way the only time we spend outside loop() is parsing the already-available responses (fast) or filling the serial output buffer (also fast) but we never sit around waiting for a response. I've been out of microcontroller programming for a while so if I'm off base, let me know, but that seems like the right general approach.

[sandeen]

https://github.com/sandeen/ESP8266_WiFi_v2.x/commits/rapiloop is an example of what I'm proposing. I'm not quite sure how to handle other commands injected by the web interface in between the rapi update loop writes & reads (or actually, how that's even handled in the current code today?)

[lincomatic]

if you send a batch of RAPI commands like that, you need to make sure

1. send/receive buffers have enough space for your commands/responses
2. you have safeguards to handle missing/corrupted RAPI responses

For (2), that's where RAPI_SEQUENCE_ID comes into play. The sequence ID that's sent with each command is echoed in each response. This way, you assure which response corresponds to a particular command, and prevent everything from getting out of sync. This also allows you to handle external RAPI commands getting injected while you're looping for your responses.

[sandeen]

FWIW, the 6 commands in the loop currently consume about 66 chars/bytes when they reply with checksums, and the esp8266 has 128 bytes, so there's some room yet. I see that the sequence ID helps here, although I haven't been able to confuse things without it yet. ;) (I'm assuming that the current wifi code w/ older firmware doesn't have any special sauce to deal with this, either?)

Anyway, whether or not commands get batched, isn't it desirable to not wait for a serial reply outside the main loop()?

[lincomatic]

If you want to reduce latency, yes, handling the replies asynchronously is better. But the way the current code is implemented makes me cringe. Yes, it will work most of the time. But if it gets out of sync, it can get completely hosed. And yes, I’ve seen the responses get out of sync before. That’s why I added the sequence ID in the first place. From: Eric Sandeen [mailto:[email protected]] Sent: Friday, May 5, 2017 3:12 PM To: OpenEVSE/ESP8266_WiFi_v2.x Cc: lincomatic; Mention Subject: Re: [OpenEVSE/ESP8266_WiFi_v2.x] RAPI update loop is painfully slow (#5) FWIW, the 6 commands in the loop currently consume about 66 chars/bytes when they reply with checksums, and the esp8266 has 128 bytes, so there's some room yet. I see that the sequence ID helps here, although I haven't been able to confuse things without it yet. ;) (I'm assuming that the current wifi code w/ older firmware doesn't have any special sauce to deal with this, either?) Anyway, whether or not commands get batched, isn't it desirable to not wait for a serial reply outside the main loop()? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<#5 (comment)>, or mute the thread<https://github.com/notifications/unsubscribe-auth/ABWzalnyhzbtyECohENlMB-piWOlXREkks5r2561gaJpZM4NNXub>.

[sandeen]

Sure, I realize that what I have in that branch isn't particularly robust, it was just to demonstrate a proposal. (although consuming the buffer before sending a new batch of commands should help get things back in sync if it ever gets off, I think)

[lincomatic]

OK, just want to make sure you guys implement it in a reliable fashion. The RAPI code that I looked at a while back was cringe worthy.

Consuming the input buffer before sending out a new batch is a good idea. But it still won't fix the scenario where a RAPI command is entered asynchronously while your batch is being processed. The async command will end up with a (wrong) response from your batch commands, and then your batch will be out of sync. If you use sequence ID's you can verify order of the responses and recover gracefully

[sandeen]

@lincomatic I was wondering the other day whether it would be useful to (optionally) just echo back the command that was issued along with its response, i.e something like:

$GU
$OK $GU 12345 67890^A8

At that point there could just be a mapping from commands to state/status variables and a single parsing loop that handles whatever responses come in based on issued RAPI commands.

[glynhudson]

This issue seems to be fixed now, implemented in #20 ?

Can this issue be closed now?

Nice work guys 👍

[jeremypoulter]

The underlying issue is still there, as none of the timings have changed.

The new web UI currently polls a bunch of values when loading and this looks to be fairly reliably, so I am thinking we change the pattern a little to wait for say 10s then read all the values.

A further enhancement might be to drive the reading of the values based on the updates it the solar/grid IE feeds (if enabled) so the sequence would be:

1. feed up date
2. Read values
3. do Solar divert calculations

This will mean the solar divert calculations are done with the an up-to-date state information rather than values that could be 30 seconds or so out of date (not that the OpenEVSE info has that much of an effect on diversion...).

This may or may not need #33 to keep the Web UI responsive though.