samsta / BatteryController

GNU General Public License v3.0
0 stars 0 forks source link

Limit currents if measurements fall between warning and critical limits #4

Open samsta opened 3 years ago

samsta commented 3 years ago

Both charging and discharging currents need to be limited when measurements are between warning and critical limits.

Unlimited current is 20A.

Constants: max & min overall battery voltage Variables: max & min currents. These will change based on variables coming from the battery. Cell voltage: If the highest cell goes above warn_volt_max we reduce max charge current, until we go to zero charge current just below crit_volt_max. If the lowest cell goes below warn_volt_min we reduce max discharge current, until we go to zero discharge current just above crit_volt_min. Temperature At high & low temperature we reduce charge and discharge current.

I'd suggest calculating the current limits based on the cell voltages and temperatures individually and then use the worst case (i.e. minimum). Use linear interpolation between 20A@warn and 0A@critical.

The functionality to shut off the contactor is already put in place, so this is just about the limits. Limits need to be exposed via methods that can be queried by the Inverter. The Inverter needs to publish them as part of the tick that's already happening every 5 seconds.

Values to begin with for the Leaf are these (below). All of the critical & none of the warning values are already in the Nissan Monitor.

CRIT_VOLT_MAX = 4.15 CRIT_VOLT_MIN = 3.00 CRIT_VOLT_DELTA = 1.00 CRIT_TEMP_MAX = 50 CRIT_TEMP_MIN = 2

WARN_VOLT_MAX = 4.1 WARN_VOLT_MIN = 3.3 WARN_VOLT_DELTA = 0.1 WARN_TEMP_MAX = 40 WARN_TEMP_MIN = 5

NiallDarwin commented 3 years ago

Query on style: I edited your comment rather than commented on it. Is that acceptable?

I'd suggest calculating the current limits based on the cell voltages and temperatures individually and then use the worst case (i.e. minimum). Use linear interpolation between 20A@warn and 0A@critical.

Agreed.

I'd also limiting based on Temp in this way. The gap between CRIT_TEMP_MIN & WARN_TEMP_MIN is only 1. What is the resolution from the temp sensors as reported on CAN? This seems too narrow a band & could cause 'hunting'.

samsta commented 3 years ago

Since it's not really a comment you've edited but rather the Issue I've raised, editing is the right thing to do.

The resolution is currently only 1degC. We could interpolate the table for the ADC in https://www.mynissanleaf.com/viewtopic.php?f=44&t=11676 if we need to get a higher resolution.

Because the band is as wide as the resolution, we'll essentially have 3 states: 20A if above WARN_TEMP_MIN, 10A at WARN_TEMP_MIN, and 0A (contactor opens) at CRIT_TEMP_MIN. At the 5 second update rate the potential jumping between limits doesn't worry me too much. But there could be a bit of chatter on the contactor if we poll the temperatures often and they go back and forth between CRIT and WARN

samsta commented 3 years ago

Further to that, going from WARN to CRIT with a change of 1degC in itself is perhaps a bit questionable. Might be good to start throttling a little earlier (i.e. higher temp), or open the contactor a little later (i.e. lower temp).

NiallDarwin commented 3 years ago

From your MNL (MyNissanLeaf) link, "It's kinda noisy though so better than 1/4 degree C isn't likely." so there's not much to be gained from interpolating.

It's the low end of the scale that's stupid-tight. I suggest WARN_TEMP_MIN = 5. That gives us 3C between throttling start and shutdown so that's room to have some throttling rather than going straight into shutdown.

2 Style notes: 1/ I have changed Big Goal 1 to suit. I'm 99% sure you didn't need me to tell you this but bear with me, I'm still learning (& liking) this collaboration tool. 2/ I have two branches of question arising from this. They are below but should I raise them as a new 'issues'? A/ In certain Leaf Batteries there are heaters which can be switched on at low temp. In other manufacturer's batteries there are coolant lines which can be used to heat or cool the battery as required. Should there be any allowance for this in the code? B/ Lithium batteries can safely discharge to about -5C but safely charge only above 0C. I don't believe we need to allow for this asymmetry, I don't envisage them being deployed in such extreme environments. However I know your approach is to build everything in from the ground so I'm keen to hear your thoughts.

samsta commented 3 years ago

It's the low end of the scale that's stupid-tight. I suggest WARN_TEMP_MIN = 5. That gives us 3C between throttling start and shutdown so that's room to have some throttling rather than going straight into shutdown.

Sounds good

1/ I have changed Big Goal 1 to suit. I'm 99% sure you didn't need me to tell you this

Don't quite know what you mean? What exactly did you change?

2/ I have two branches of question arising from this. They are below but should I raise them as a new 'issues'?

Yeah, issues are a good way of noting possible future improvements. We can use the labels to make it clear which things are absolutely necessary and which are nice-to-haves.

A/ In certain Leaf Batteries there are heaters which can be switched on at low temp. In other manufacturer's batteries there are coolant lines which can be used to heat or cool the battery as required. Should there be any allowance for this in the code?

I'd say we add these things as the need arises. Lots of figuring out to do, which probably requires getting our hands on such batteries

B/ Lithium batteries can safely discharge to about -5C but safely charge only above 0C. I don't believe we need to allow for this asymmetry, I don't envisage them being deployed in such extreme environments. However I know your approach is to build everything in from the ground so I'm keen to hear your thoughts.

No, the approach is not to build everything in from the ground, but make a design that is easy to extend and bolt extra things on without having to worry that it all goes up in flames.

The inverter already asks for charge and discharge currents separately, so it's easy to have different temperature limits. The harder part might be the opening of the contactor because the point at which we'll force it open it will depend on whether we're charging or discharging. AFAICT the only way we can figure out right now whether we're charging or discharging is by looking at the direction of the current. But once the contactor is forced open because we tried charging at 0C or below, how would we know when the inverter wants to start discharging again if there's no current flowing in either direction because the contactor is open? I'm sure the inverter tells us if it wants to charge or discharge in one of the variables, but I believe we haven't figure that out yet.

I'd say for now its probably safest to just stick to the symmetric limits.

NiallDarwin commented 3 years ago

1/ I have changed Big Goal 1 to suit. I'm 99% sure you didn't need me to tell you this

Don't quite know what you mean? What exactly did you change?

I actually changed CRIT_TEMP_MIN but what I meant to change was WARN_TEMP_MIN. I have now corrected this mistake.

2/ I have two branches of question arising from this. They are below but should I raise them as a new 'issues'?

Yeah, issues are a good way of noting possible future improvements. We can use the labels to make it clear which things are absolutely necessary and which are nice-to-haves.

Will do

AFAICT the only way we can figure out right now whether we're charging or discharging is by looking at the direction of the current. But once the contactor is forced open because we tried charging at 0C or below, how would we know when the inverter wants to start discharging again if there's no current flowing in either direction because the contactor is open? I'm sure the inverter tells us if it wants to charge or discharge in one of the variables, but I believe we haven't figure that out yet.

I'd say for now its probably safest to just stick to the symmetric limits.

Agreed, let's stick to symmetrical limits

samsta commented 3 years ago

I have just noticed that the LBC tells us MaxPower and MaxChargePower in the 0x1dc message. Perhaps we should take that into account as well. My only question would be how we translate that into max currents - based on the nominal voltage (360V) or the actual voltage?

NiallDarwin commented 3 years ago

Actual Voltage if possible

samsta commented 3 years ago

I'd suggest calculating the current limits based on the cell voltages and temperatures individually and then use the worst case (i.e. minimum)

Hmm. Not so sure anymore about that, after some further thinking. Surely, if both a temperature and voltage limit is violated, the current needs to be reduced even further?

So let's assume the maximum cell voltage is at a point where the current should be reduced to 0.5 x nominal current, and an then the temperature goes above the limit too, I'd say the current should be reduced even more. So if we have limit factors that are at 1 when good, somewhere between 1 and 0 when between warning level and critical level, and 0 when at or beyond critical level, and the voltage-based factor is fv and the temperature-based factor is ft, then I'd suggest we use fv x ft for the limit rather than min(fv, ft). Whatcha think, @NiallDarwin ?

samsta commented 3 years ago

If we also bring the voltage-delta based factor fd into the equation, we'd then use fv fd ft rather than min(fv, fd, ft)

NiallDarwin commented 3 years ago

I 'd suggest calculating the current limits based on the cell voltages and temperatures individually and then use the worst case (i.e. minimum)

Hmm. Not so sure anymore about that, after some further thinking. Surely, if both a temperature and voltage limit is violated, the current needs to be reduced even further?

Yes. I wasn't specific enough to communicate my meaning correctly.

When limiting based on voltage it is the extreme voltage (highest if high, lowest if low) that we should use to determine the limit. As opposed to the overall pack voltage. Same process for temperature.

I'd suggest we use fv x ft for the limit rather than min(fv, ft). Whatcha think, @NiallDarwin ?

Yes. If we are in a limiting scenario for both temperature & voltage yes, I agree with your suggestion.

If we also bring the voltage-delta based factor fd into the equation, we'd then use fv fd ft rather than min(fv, fd, ft)

I think no. If we are using cell voltage rather than pack voltage we are already capturing the worst case so there's no need to account for delta. I don't see how it helps. All the other cells are by definition in a less critical state & so should be able to cope with whatever the extreme cell demands.

samsta commented 3 years ago

But you do want to limit the currents when the delta is high without the cell voltage limits being violated? If the answer is yes, I'd suggest min(fv, fd) x ft to avoid surprises.

NiallDarwin commented 3 years ago

But you do want to limit the currents when the delta is high without the cell voltage limits being violated? If the answer is yes, I'd suggest min(fv, fd) x ft to avoid surprises.

I don't think I do.

NiallDarwin commented 3 years ago

On Pi I have changed: critical high voltage from 4.15 to 4.1, warn high voltage from 4.1 to 4.05 and voltage limit resolution from 0.001 to 0.1. This is to try and remove the tendency of the inverter to hit those limits

samsta commented 3 years ago

That's unlikely to work. There's no 0.1V step from 4.05V to 4.1V, so you'd need a 0.01V resolution, at least. Even if it were to work, I'm doubtful that it would address the issue that there's no headroom between the current limit dropping to 0 and the contactor opening

On January 25, 2021 at 12:29 PM, Niall Darwin notifications@github.com wrote:

On Pi I have changed: critical high voltage from 4.15 to 4.1, warn high voltage from 4.1 to 4.05 and voltage limit resolution from 0.001 to 0.1. This is to try and remove the tendency of the inverter to hit those limits

β€” You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe.

NiallDarwin commented 3 years ago

You make some good points there 😜

On Mon, 25 Jan 2021, 12:36 PM sam nobs, notifications@github.com wrote:

That's unlikely to work. There's no 0.1V step from 4.05V to 4.1V, so you'd need a 0.01V resolution, at least. Even if it were to work, I'm doubtful that it would address the issue that there's no headroom between the current limit dropping to 0 and the contactor opening

On January 25, 2021 at 12:29 PM, Niall Darwin notifications@github.com wrote:

On Pi I have changed: critical high voltage from 4.15 to 4.1, warn high voltage from 4.1 to 4.05 and voltage limit resolution from 0.001 to 0.1. This is to try and remove the tendency of the inverter to hit those limits

β€” You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe.

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/samsta/BatteryController/issues/4#issuecomment-766460664, or unsubscribe https://github.com/notifications/unsubscribe-auth/AMIBEIYXQLCU4NEAIUIWASDS3SVHZANCNFSM4SMNSIRQ .

NiallDarwin commented 3 years ago

critical high voltage from 4.15 to 4.1, warn high voltage from 4.1 to 4.05 and voltage limit resolution from 0.001 to 0.1.

I have now reversed these changes on the pi