serkri / SmartEVSE-3

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

Failsafe protection when loadbalancing #71

Closed daninden closed 1 year ago

daninden commented 1 year ago

I am about to install a new smartevse which I want to use for solar charging and load balancing. As I want to provide the current measurements myself (from my P1 meter using the api), I was wondering if there was any failsafe in the code to prevent overcharging when no up-to-date data is available.

I was checking the source code for that, and the only thing I could find was the following

        if (MainsMeter == EM_API && phasesLastUpdate < (time(NULL) - 60)) {
            phasesLastUpdate=0;
            Irms[0] = 0;
            Irms[1] = 0;
            Irms[2] = 0;
            Isum = 0; 
            UpdateCurrentData();
        }

First thing, for me it seems that 60 seconds is a bit long when load balancing. Secondly, I think the reset is done reversed, it will reset to 0, allowing maximal charging. So if there is no data available, it will think that all main phases have no current, and the charging current can be increased. Thirdly, this check might also be beneficial for the regular modbus updates I think (so not only for the API).

rvdgaag commented 1 year ago

I'm not a programmer but I experienced when overloading the Modbus with a second master the SMARTEVSE was not able to receive the data from the Eastron meters and completely shut down the charging.

arpiecodes commented 1 year ago

The modbus meters have this protection because it has a counter that counts down from 10 to 0. Every time it receives a measurement this timer will reset to 10. If the countdown reaches 0, the NO COMM error is triggered. It also does this for the Sensorbox communication (if used).

While reading the code I also came to the same conclusion as you. There is currently no error triggered nor protection for overcurrent protection when using API values for the MainsMeter. It knows there is no fresh data and it will reset all measurements to zero, but it will not trigger any shutdown of the charging in progress. It would actually make it worse that way.

I tried to address this issue by also using the same timeout timer in the main logic for the API-based MainsMeter. If it has not received any value for 10 seconds, it will trigger the NO COMM error and should in theory shut down charging operation.

If you want you could test it by compiling my pull request, which also has some other goodies/features I was missing; https://github.com/serkri/SmartEVSE-3/pull/69.

daninden commented 1 year ago

@synegic

i think your solution might do the trick.. however.. i'm not sure if for me personally i want the system to stop charging. I know that there is always enough current to at least charge at 20A (in my situation), so it would be nice if it would be configurable.

arpiecodes commented 1 year ago

@daninden That sounds a bit unsafe to me (completely ignoring max currents already set in the configuration). The system should always stop charging if the application supplying the API data stops doing so as then the load balancing feature is essentially 'flying blind'. In such case I think it'd be better to change the mode to Normal and use OverrideCurrent option to dynamically set it from an external system like HA.

daninden commented 1 year ago

well the latter won''t work if communication is down :).. but I agree, in most situations that is probably not safe. As long as it restarts charging once communication is up again it shouldn't be a big problem I guess. I don't want to have to manually reset it....

arpiecodes commented 1 year ago

@daninden yes, it should in theory start charging again if the API comes back providing new values. You can test this easily of course. Personally I have not tested it thoroughly yet but it should in theory work that way when in Smart mode.

daninden commented 1 year ago

i checked the code... it does reset CT_NOCOMM only when timeout is set to 10. This is not done from the API i think, so currently it will not start charging again.

arpiecodes commented 1 year ago

@daninden Yes, the current code in the master branch does not contain anything to reset the counter nor trigger the failsafe protection. I added that timeout counter to my pull only recently; https://github.com/serkri/SmartEVSE-3/pull/69/commits/67e2e950fb1ef43e764e080cb40e77cedab18205.

daninden commented 1 year ago

something like this should be sufficient I think:

        if (MainsMeter == EM_API) {
            if (!(ErrorFlags & CT_NOCOMM) && phasesLastUpdate < (time(NULL) - 10))
            {
                ErrorFlags |= CT_NOCOMM;
                if (State == STATE_C) setState(STATE_C1);                       // tell EV to stop charging
                else setState(STATE_B1);                                        // when we are not charging switch to State B1
            }
            else if (ErrorFlags & CT_NOCOMM)
            {
                ErrorFlags &= ~CT_NOCOMM; 
            }
        }
arpiecodes commented 1 year ago

That's the way I initially tried to fix it, but that caused some weird behaviour as the timeout would keep on resetting to 10 in the main code, clearing the NO COMMS error while it was added again by the EM_API code, over and over. Much alike what you shared now. Eventually settled on simply using the timeout value for EM_API as well.

daninden commented 1 year ago

could also opt to use the timeout variant only if MainsMeter != EM_API

if ((ErrorFlags & CT_NOCOMM) && MainsMeter != EM_API && timeout == 10) ErrorFlags &= ~CT_NOCOMM;          // Clear communication error, if present
arpiecodes commented 1 year ago

Actually tried that as well, but then you had to do it on multiple places because the Modbus communication code also sets and resets it at random places in the code. I figured it was best/easiest to simply re-use the same mechanism already in place for exactly this situation (used for both Sensorbox and Modbus Mains energy meters). And it works.

But maybe the maintainers can find another way to fix it properly/find your solution better. I guess it's just a matter of preference.

daninden commented 1 year ago

yeah.. i understand.. the code is a bit all over the place, so it's hard to make a 'good' fix. Lets see if there are any maintainers alive :)

arpiecodes commented 1 year ago

Exactly how I'm feeling about the code. I guess a huge refactor round would do wonders. My OCD meter goes off the charts sometimes. :D

daninden commented 1 year ago

i think i have an easier fix, created a PR.

74

Just using the existing mechanism, and setting timeout = 10 in the API call

Tested it, seems to work.

dingo35 commented 1 year ago

74 handled this issue and is committed.