Battery protection

[arne182]

If your car battery voltage is lower than 0% switch off charging to stop the Eon from damaging the 12v car battery. Leaving you with a car that can not start but has a fully charged eon ;-)

Choose one of the templates below:

Fingerprint

This pull requests adds a fingerprint for <Make - Model - Year - Trim>.

This is an explorer link to a drive with the stock system enabled: ...

Car support

This pull requests adds support for <Make - Model - Year - Trim>.

This is an explorer link to a drive with the stock system enabled: ...
This is an explorer link to a drive with openpilot system enabled: ...

Feature

This pull requests adds feature X

Description

Explain what the feature does

Testing

Explain how the feature was tested. Either by the added unit tests, or what tests were performed while driving.

[ErichMoraga]

Assuming "10500" is 10.5V, I'd actually feel better if it was a higher value like 11000.

[arne182]

@ErichMoraga The voltage does fluctuate a bit. having it at 11000 will also trip it when the car is switched off. Especially if your battery is not fully charged.

I tested it at 11200 and that was not so nice to have the eon off but the car still able to start. But from the graph above I should only have had 10% or less in my auxilary battery. I would be all for a warning at 11500 but definitely cut the power only if really needed.

[arne182]

Maybe another question. The measured voltage is that through the comma power cable? I assume there must be some voltage loss through the cable that could be accounted for. How many cables are being used for the power transmition?

[ErichMoraga]

I'd think the measured voltage is whatever eventually hits the Panda. Observed voltage will vary around the car. That said, I will test this out with it set to 13V so it only charges the EON when my alternator/generator is running. In the Prius Prime, it actually doesn't always run when the car is on... something I monitor using various gauges. Anyway, I really prefer not to charge external devices when the car is off.

Edit: 13V went as expected, although after some thought, I decided 12V will work better for me in my fork (but will first continue to test internally).

[geohot]

I support this conservative threshold. Not sure if we need some smoothing on that input signal to prevent false positives.

[ErichMoraga]

For smoothing, one could use high/low hysteresis as seen in thermald.py for the temperature thresholds that trigger different fan speeds.

[arne182]

It is not hysterisis it is more a pulsing of current. That makes the voltage drop and rise. From my measurements between 0.5V and 1V we could also average the max and min values over the last second or two? But as it is now it is working. Any current draw that would cause the battery voltage to drop below the critical voltage will switch off the charging

[ErichMoraga]

Since were talking about refining the last sentence, how about simplifying it to...
"Start vehicle to resume charging."

[jyoung8607]

I support this conservative threshold. Not sure if we need some smoothing on that input signal to prevent false positives.

Yes. It's completely normal to have quick hits of higher battery load and thus lower Vbatt. For example, it'll happen directly after shutdown (although less so with battery surface charge) and definitely right after opening the car. Or, interactions with Telematics services will wake things up. Instantaneous battery state can't really be relied on.

As an example, if you come back to the car 10 minutes after parking it because you forgot something, unlocking it turns on a lot of external lights, and opening the door typically turns on a bunch of internal lights, the instrument cluster and infotainment system, blips the in-tank fuel pump to prime fuel pressure for starting, etc etc. It's totally normal to see a large Vbatt dip here, and for it to return to near-resting levels after you've locked things back up and the car's electronics have gone back to sleep.

EON can and should recover from these situations like this, and with the proper code, we could set a much higher and safer resting-state Vbatt cutoff while auto-recovering from temporary dips. Some pseudo-code:

VBATT_START_CHARGING = 12.0
VBATT_PAUSE_CHARGING = 11.8

if ignition or (Vbatt > VBATT_START_CHARGING for 60 seconds):
  enable charging if disabled
else if Vbatt < VBATT_PAUSE_CHARGING for 60 seconds:
  disable charging if enabled

I am not married to those particular VBATT_START_CHARGING and VBATT_PAUSE_CHARGING values, except to say we can set them relatively high with this design, and they just need enough spread between them to control hysteresis when EON adds up to ~11W (5V/2.2A, 12V/0.9A) of load.

[ErichMoraga]

@jyoung8607 We're on the same wavelength.

[rbiasini]

ok, let's implement a non-temporal hysteresis, while still being very conservative (at least for now). This is what I propose

diff --git a/selfdrive/boardd/boardd.cc b/selfdrive/boardd/boardd.cc
index 96d57b1da..fa8073985 100644
--- a/selfdrive/boardd/boardd.cc
+++ b/selfdrive/boardd/boardd.cc
@@ -62,6 +62,8 @@ bool loopback_can = false;
 cereal::HealthData::HwType hw_type = cereal::HealthData::HwType::UNKNOWN;
 bool is_pigeon = false;
 const uint32_t NO_IGNITION_CNT_MAX = 2 * 60 * 60 * 30;  // turn off charge after 30 hrs
+const uint32_t VBATT_START_CHARGING = 11000;
+const uint32_t VBATT_PAUSE_CHARGING = 10500;
 uint32_t no_ignition_cnt = 0;
 bool connected_once = false;
 bool ignition_last = false;
@@ -368,12 +370,20 @@ void can_health(PubSocket *publisher) {
   }
 
 #ifndef __x86_64__
-  if ((no_ignition_cnt > NO_IGNITION_CNT_MAX) && (health.usb_power_mode == (uint8_t)(cereal::HealthData::UsbPowerMode::CDP))) {
+  bool cdp_mode = health.usb_power_mode == (uint8_t)(cereal::HealthData::UsbPowerMode::CDP);
+  bool no_ignition_exp = no_ignition_cnt > NO_IGNITION_CNT_MAX;
+  if ((no_ignition_exp || (health.voltage <  VBATT_PAUSE_CHARGING)) && cdp_mode && !ignition) {
     printf("TURN OFF CHARGING!\n");
     pthread_mutex_lock(&usb_lock);
     libusb_control_transfer(dev_handle, 0xc0, 0xe6, (uint16_t)(cereal::HealthData::UsbPowerMode::CLIENT), 0, NULL, 0, TIMEOUT);
     pthread_mutex_unlock(&usb_lock);
   }
+  if (!no_ignition_exp && (health.voltage >  VBATT_START_CHARGING) && !cdp_mode) {
+     printf("TURN ON CHARGING!\n");
+     pthread_mutex_lock(&usb_lock);
+     libusb_control_transfer(dev_handle, 0xc0, 0xe6, (uint16_t)(cereal::HealthData::UsbPowerMode::CDP), 0, NULL, 0, TIMEOUT);
+     pthread_mutex_unlock(&usb_lock);
+  }
   // set power save state enabled when car is off and viceversa when it's on
   if (ignition && (health.power_save_enabled == 1)) {
     pthread_mutex_lock(&usb_lock);

@arne182 et al., what do you think? Due to conflicts, the diff might not be applied directly, but you get the sense.

[jyoung8607]

@arne182 et al., what do you think? Due to conflicts, the diff might not be applied directly, but you get the sense.

If I'm reading this correctly, the logic is sound. And it's okay if you want to skip the temporal component for now. But I don't think those proposed thresholds are good.

Panda/EON drawing ~12V/0.9A alone is too light a load to change Vbatt very much on an automotive-sized battery, so the OCV table @arne182 posted above is still reasonably applicable as a safety floor. Comparing your patch to the table, you can see 11.0 is near-dead and 10.5 is D-E-D dead.

I believe VBATT_PAUSE_CHARGING should be no lower than 11.8 and we might consider a touch higher. I had proposed VBATT_START_CHARGING = 12.0 and I still think that's okay but am willing to discuss. I have not tested to find a minimum spread between PAUSE and START for good hysteresis control but I think 0.2V isn't likely to cause trouble.

[jyoung8607]

I'll add two more comments in defense of my higher VBATT_PAUSE_CHARGING:

1. Normal (non-AGM) lead acid automotive batteries DO NOT like to be cycled deeply, 30% is about as low as you should go on any kind of regular basis.

2. When below 30-40% state of charge, lead acid batteries are subject to electrolyte freezing at winter temperatures commonly reached in the northern continental USA and other places openpilot is used.

[ErichMoraga]

@jyoung8607 and I are still on the same wavelength. I prefer 12V as the threshold, as a vehicle's health should be prioritized over the EON's, and is what I've been testing internally with the original, basic logic. That said, the driver will certainly see the alert often, which could be alarming to people.

I understand Comma's POV, trying to be conservative, but 10.5V is too low IMHO. I'm fine if that means we settle on values ~11.5V.

[arne182]

From my measurements with a 1v fluctuation it actually is at 11500. 12282-11334 measured in panda where battery is at a constant 12.18. So no need to worry there. There is a test script in the panda tests to get the health information. And a simple multimeter will tell you the actual voltage.

[arne182]

From personal test the code is working well. Is there anything else needed for this merge @rbiasini ?

[rbiasini]

@arne182 two comments. Other than that it's good to be merged.

[arne182]

Got another chance to test this and it is working perfectly.

[ErichMoraga]

Yes, I agree it works, although I'm not a fan of the thresholds. I experimented with them at the edge (12.4V and .5V hysteresis), for example, and could see the code in action, switching back/forth, so gave it a higher hysteresis value to prevent that.

BTW - 12.4V came from a battery mfg. recommendation to not let the battery discharge below 12.4V on a regular basis... which I agree with.

FYI - The day this PR was committed, I decided to make an LVD hardware solution (as seen in #hw-unofficial and #toyota-lexus) which works perfectly, since I wanted similar protection for anything using power from the CPv2. How did I make it so quickly? Used some off the shelf parts, knowledge of the new fake ethernet spec, and skill I acquired from ZSS case design.

[jyoung8607]

Yes, I agree it works, although I'm not a fan of the thresholds. I experimented with them at the edge (12.4V and .5V hysteresis), for example, and could see the code in action, switching back/forth, so gave it a higher hysteresis value to prevent that.

Arne observed a pretty big difference between VBatt measured with a multimeter at the battery and what Panda sees. Depending on where in the car it's hooked up, there could be enough small-gauge wire between the battery and Panda for meaningful voltage drop, so I think that's a valid observation.

As an example, Panda/EON at full rate charging will draw about 1A@12V, which according to an online calculator, could produce a drop of ~0.3V over 8-10 feet of 22AWG (0.35mm2) wire, and that's not including any extra crimps or connectors or fuses along the way. Couple that with additional slight voltage drop at the battery itself when it's placed under a bit of load, and perhaps 0.5V swings measured by Panda are plausible when charging load is applied or removed.

So I think we're all in raging agreement about the need for battery preservation, but ISTM the exact thresholds and on/off activity will need to be watched for how they perform in the real world on a variety of cars.

Other considerations:

• Earlier unconditional charging shutoff will help, I believe a 30 hour shutoff is already in the works
• Next NEOS with Revert "wifi: Scan for networks every time the supplicant state changes" android_frameworks_opt_net_wifi#1 already cuts sleep power usage by nearly a third, and there are further improvements that could be made in EON sleep efficiency
• If raw VBatt monitoring turns out unworkable, a simple Coulumb counter (e.g., integrate charge current over time and shut off charging after drawing 3-5Ah@12v, after a certain amount of confirmed vehicle runtime to verify battery was charged to start with) would be pretty robust and insensitive to bouncy VBatt.

[ErichMoraga]

Maybe we can have a "learner" to determine each car's unique voltage characteristics...

[ErichMoraga]

@rbiasini I'm sure you've already thought of this, but I'd like to minimize the chance of data corruption with how this PR affects the impending Comma Two release. I know you can differentiate between an EON and Comma Two, so one should incorporate a controlled, automatic shutdown for the Two when the low voltage threshold is hit.

[arne182]

Just got back from holidays and got a chance to see the alert in action. Worked exactly as expected with voltage varying with 0.969V. Thinking of adding a bigger aux battery to keep things running for longer

[ErichMoraga]

Bigger battery is a bandage.