evcc-io / evcc

Sonne tanken ☀️🚘
https://evcc.io
MIT License
3.32k stars 606 forks source link

Mismatched current with MaxCurrentMillis() function #10396

Closed Sauttets closed 11 months ago

Sauttets commented 11 months ago

The MaxCurrentMillis function takes in a float64 which can have up to 17 digits after the decimal point. The function that checks if the expected amperage is equal to the received amperage does not round the float64 but compares it directly :

if lp.chargeCurrent != current {
    if lp.guardGracePeriodElapsed() {
        lp.log.WARN.Printf("charger logic error: current mismatch (got %.3gA, expected %.3gA)", current, lp.chargeCurrent)
    }
        lp.chargeCurrent = current
    lp.bus.Publish(evChargeCurrent, lp.chargeCurrent)
}

this results in some interesting warnings:

[lp-1  ] WARN 2023/10/19 13:42:44 charger logic error: current mismatch (got 15A, expected 15A)
[lp-1  ] WARN 2023/10/19 13:42:45 charger logic error: current mismatch (got 15A, expected 15A)
[lp-1  ] WARN 2023/10/19 13:42:47 charger logic error: current mismatch (got 14.9A, expected 14.9A)
[lp-1  ] WARN 2023/10/19 13:42:51 charger logic error: current mismatch (got 15A, expected 15A)
[lp-1  ] WARN 2023/10/19 13:42:53 charger logic error: current mismatch (got 14.9A, expected 14.9A)

To avoid this issue the value should either be rounded, or accept a small deviation.

Side note: Most PWM-Function-Units on microcontrollers have 256 steps which can be adjusted from 0-100%. According to SAE J1772 (or IEC 61581) each percent of the PWM is equal to 0.6A. Thus the smallest possible adjustment most PWM-Controllers can do is: 100%÷256×0,6A = 0.234A

andig commented 11 months ago

Thats true. But hoe many digits is „small“? Seems we should always truncate.

andig commented 11 months ago

Thanks for adding the definition of „small“. The main purpose of this api is to prevent devices not receiving/accepting the current command and continue charging high power in a low power scenario. For that purpose, 1A precision should be enough for detection. Since we cannot know the controllers precision and I would still like to maintain finer control for e.g. Wallbe or OpenWB Pro I‘d stick to that for time being.

andig commented 11 months ago

There‘s another downside to this. While we would not raise the error, we‘d still re-set the current in every control cycle. We need to be careful how to handle regular PV mode current adjustments in this case.

/cc @MarkusGH

premultiply commented 11 months ago

Is there really a downside of resending a current value on each cycle?

MarkusGH commented 11 months ago

Since we cannot know the controllers precision and I would still like to maintain finer control for e.g. Wallbe or OpenWB Pro I‘d stick to that for time being.

Ho about adding a Granularity value to ChargerEx?

(pseudocode)

if Abs(lp.chargeCurrent - current) > (1 /charger.Granularity) {

andig commented 11 months ago

Is there really a downside of resending a current value on each cycle?

Easee cloud api…

MarkusGH commented 11 months ago

Is there really a downside of resending a current value on each cycle? Easee cloud api…

Special cases like that could be handled in the charger code. If a rate limit is needed for specific devices, loadpoint.go is IMO not the place for that.

Sauttets commented 11 months ago

I currently round the value to 5 digits after the decimal point, which is a 10μA accuracy. I don't think any car can change the current this exact.

Edit:

if math.Round(lp.chargeCurrent*100000)/100000 != math.Round(current*100000)/100000  

i am sure there is a cleaner way to do this but this works fine or:

tolerance := 1.01
if lp.chargeCurrent*tolerance < current || current < lp.chargeCurrent/tolerance
premultiply commented 11 months ago

I've never seen anything less than 1 mA.

MarkusGH commented 11 months ago

i am sure there is a cleaner way to do this

Yes (IMO) :-)

Test for:

Abs(lp.chargeCurrent - current) > tolerance

Tolerance is an absolute value here.