Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate system event when low battery condition is detected #1154

Merged
merged 4 commits into from Dec 10, 2016

Conversation

@sergeuz
Copy link
Member

commented Oct 30, 2016

This PR adds low_battery system event, which is generated when low battery condition is detected. The event doesn't carry any data.

I don't have any battery, nor adjustable power supply unit to test this PR properly, so any help with the testing would be appreciated. Test application can be as simple as the following:

#include "application.h"

SYSTEM_MODE(MANUAL); // Electron will be offline!!!

SerialLogHandler logHandler(LOG_LEVEL_WARN, {
    { "app", LOG_LEVEL_ALL }
});

void lowBatteryHandler() {
    Log.warn("Low battery!");
}

void setup() {
	FuelGauge().setAlertThreshold(32); // MAX 32% (default 10%)
	System.on(low_battery, lowBatteryHandler);
}

void loop() {
	FuelGauge().quickStart();
	String stats = String(FuelGauge().getAlertThreshold()) + "(\%) "
		+ String(FuelGauge().getSoC()) + "(\%) "
		+ String(FuelGauge().getVCell()) + "(V)";
	Log.info("Battery stats: %s", stats.c_str());
	delay(1000);
}

According to current code, default threshold for low battery alarm is set to 10%.


Doneness:

  • Contributor has signed CLA
  • Problem and Solution clearly stated
  • Code peer reviewed
  • API tests compiled
  • Run unit/integration/application tests on device
  • Add documentation
  • Add to CHANGELOG.md after merging (add links to docs and issues)

Features

  • Added low_battery system event, which is generated when low battery condition is detected. This is when the battery falls below the SoC threshold (default 10%, max settable 32%). The event can only be generated again if the system goes from a non-charging to charing state after the event is generated. The event doesn't carry any data. [PR #1154]

@sergeuz sergeuz added this to the 0.7.x milestone Oct 30, 2016

@technobly technobly self-assigned this Oct 30, 2016

@technobly

This comment has been minimized.

Copy link
Member

commented Oct 30, 2016

I will test this when I get a chance. Thanks!

@technobly

This comment has been minimized.

Copy link
Member

commented Oct 31, 2016

I added some code to loop() to help facilitate a fast update on the the battery voltage and SoC readings, otherwise they take way too long to update by themselves due to the default filtering profile of the Fuel Gauge.

Test output:

0000020062 [app] INFO: Battery stats: 10(%) 14.656250(%) 3.720000(V)
0000021064 [app] INFO: Battery stats: 10(%) 14.398438(%) 3.710000(V)
0000022066 [app] INFO: Battery stats: 10(%) 14.398438(%) 3.700000(V)
0000023069 [app] INFO: Battery stats: 10(%) 10.257813(%) 3.693750(V)
0000024072 [app] WARN: Low battery!
0000024074 [app] INFO: Battery stats: 10(%) 8.582031(%) 3.690000(V)
0000025076 [app] INFO: Battery stats: 10(%) 7.964844(%) 3.678750(V)
0000026079 [app] INFO: Battery stats: 10(%) 6.613281(%) 3.670000(V)
0000027081 [app] INFO: Battery stats: 10(%) 5.742188(%) 3.658750(V)
0000028083 [app] INFO: Battery stats: 10(%) 6.613281(%) 3.676250(V)
0000029085 [app] INFO: Battery stats: 10(%) 8.582031(%) 3.690000(V)
0000030088 [app] INFO: Battery stats: 10(%) 11.550781(%) 3.700000(V)
0000031090 [app] INFO: Battery stats: 10(%) 10.515625(%) 3.700000(V)
0000032092 [app] INFO: Battery stats: 10(%) 10.515625(%) 3.700000(V)
0000033095 [app] INFO: Battery stats: 10(%) 11.292969(%) 3.693750(V)
0000034098 [app] WARN: Low battery!
0000034100 [app] INFO: Battery stats: 10(%) 9.480469(%) 3.690000(V)
0000035102 [app] INFO: Battery stats: 10(%) 9.480469(%) 3.690000(V)
0000036104 [app] INFO: Battery stats: 10(%) 12.328125(%) 3.700000(V)
0000037107 [app] INFO: Battery stats: 10(%) 11.550781(%) 3.700000(V)
0000038109 [app] INFO: Battery stats: 10(%) 10.257813(%) 3.690000(V)
0000039112 [app] WARN: Low battery!
0000039114 [app] INFO: Battery stats: 10(%) 8.582031(%) 3.690000(V)
0000040117 [app] INFO: Battery stats: 10(%) 9.222656(%) 3.688750(V)
0000041119 [app] INFO: Battery stats: 10(%) 8.582031(%) 3.688750(V)
0000042121 [app] INFO: Battery stats: 10(%) 9.480469(%) 3.690000(V)

You can see that the Low Batt alarm on the Fuel Gauge is being reset properly with SoC goes above the default 10% as well as triggering each time the SoC goes below 10%.

I also added some code to to setup() set the Low Battery Alarm threshold up to it's max 32%, comment that out to get the default 10% limit. Results:

0000018058 [app] INFO: Battery stats: 32(%) 37.582031(%) 3.788750(V)
0000019061 [app] WARN: Low battery!
0000019063 [app] INFO: Battery stats: 32(%) 31.691406(%) 3.780000(V)
0000020066 [app] INFO: Battery stats: 32(%) 28.253906(%) 3.770000(V)

I think we should add this complete code example to the .setAlertThreshold() section of Docs: https://docs.particle.io/reference/firmware/electron/#setalertthreshold-

.setAlertThreshold() should also have a description added:

threshold Sets the State of Charge (SoC) low battery alarm threshold in percentage from 0-32(%) as a byte. When the SoC threshold is reached, the alarm is automatically cleared and a System Event low_battery is generated. Something to keep in mind is that if the battery fluctuates around the set threshold, there can be a significant amount of low_battery events generated. It is up to user how this is handled in conjunction with the implemented re-charging method.

@technobly

This comment has been minimized.

Copy link
Member

commented Oct 31, 2016

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Oct 31, 2016

Right, I think we need to add some simple checks to ensure that low_battery is not generated too frequently

@technobly

This comment has been minimized.

Copy link
Member

commented Oct 31, 2016

One way might be to require the user to reset the Alert flag with FuelGauge().clearAlert(), but if they don't and there is a lot of PMIC interrupts, the PMIC interrupt handler will constantly be triggering the low_battery event as well. PMIC interrupts can happen very frequently when the battery tops off. The battery SoC shouldn't normally fluctuate that much if not calling .quickStart() often like I did in the loop(). It's typically heavily filtered so that alone will give if a decent amount of hysteresis. I guess the reason why I see it fluctuating here is mainly because .quickStart() is continuously coming up with slightly higher or lower values around the threshold. Saying all that, I feel like it's probably ok to leave it as is... but just add these notes to the documentation.

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Nov 6, 2016

Let's proceed with the discussion of this PR. Quick summary:

  • It would be nice to limit how frequently low_battery event is generated. Ideally, it should be generated only once and never generated again until battery has started to charge.
  • It would be nice to have battery_charged event. What about battery_charge_started, batter_charge_stopped events, or even a single event with battery status notified as a parameter?

@technobly Could you summarize your concerns on querying battery status in the firmware here?

@legoadk

This comment has been minimized.

Copy link

commented Nov 7, 2016

I'm sorry if this belongs elsewhere, but I'm curious; is this feature destined only for Electron firmware, or will it switch on when any MAX1704x fuel gauge is detected; for example, when a Photon is connected to a SparkFun Photon Battery Shield that contains a MAX1704x? It would seem silly to exclude that use-case from this feature (maybe I'm biased - my Photon project uses that shield!)

@technobly

This comment has been minimized.

Copy link
Member

commented Nov 29, 2016

@sergeuz here's what I think we could do to protect the system from memory issues, but not overcomplicate things:

  1. If we get a Low Battery interrupt, send the event, clear the Alert flag but do not allow sending another low battery system event until CHRG_STAT goes from not charging to some non-zero charging level. In line with what you originally said ;-)
  2. If low batt occurs while charging there is likely a heavy load applied, require CHRG_STAT to be zero first, then a non-zero state before allowing another low battery event (however if that happens and the load remains there is little chance that the capacity will go up). Otherwise if reset simply due to a charging state, it could also generate a lot of events if the load is just right.
  3. There is also a state where the battery can be heavily loaded as state of charge (SoC) is forced via .quickStart(), this should generate a low batt int/event, then the load removed when SoC is forced again via .quickStart() and the SoC can increase above the threshold, and finally force checked again under load to generate another low battery int/event. All of this can happen while the battery is not being charged. This is one reason simply polling for SoC changes won't give us the best response. Under this 3) case with 1) and 2) in place, it should still only generate one low battery event. And we don't really care that the SoC falsely indicated low and high and low again, but since it dropped below the threshold a low battery event is warranted. Users are the ones potentially forcing the quickStart() checks so the onus will be on them to understand these nuances (we can facilitate with documentation).
  4. It might be worth it to send a PMIC event that wraps up the STAT and possibly also ERROR bytes in one 16-bit pmic_status event. Since we would be reading these values anyway to parse the chrg_stat value. With this, users can determine charging status in an event driven way, but it is also not strictly enforced how the PMIC events are generated. You get them all and can apply a bit mask to look at values you are interested in. This can also generate a lot of system events though. More so than low batt, especially as the battery tops off. Because of this, it might be better to allow users to register a direct ISR handler instead of pushing these values through the system event handler. This is the reason we disabled those PMIC stat/error logs.

I think we should also keep this PR focused on low battery only as much as possible, so 4) would be left to another PR.

@technobly
Copy link
Member

left a comment

Let's add 1) and 2) from the comments and consider this PR finished.

@technobly technobly modified the milestones: 0.7.x, 0.6.1 Nov 29, 2016

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2016

To me it looks like in order to implement this we need to track battery's critical level (LOWBATT) and charging state (chrg_stat) independently. Does system_power_management_update() get called whenever any of these flags changes?

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2016

Added check for battery's charging status to system_power_management_update() function.

static bool wasCharging = false; // Whether the battery was charging last time when this function was called
const uint8_t status = power.getSystemStatus();
const bool charging = (status >> 4) & 0x03;
if (!charging && wasCharging) { // Check if charging has been stopped

This comment has been minimized.

Copy link
@technobly

technobly Dec 8, 2016

Member

Because it's more likely that a low battery event will be generated while the electron is not charging the battery, it seems like this line should be inverted to reset lowBattEventNotified when the system goes from not charging to charging.
if (charging && !wasCharging) {. Currently it would require two actions, a transition from !charging to charging, and then back to !charging.

Without this, it will not be possible to generate a low battery event while applying a heavy load under light/normal charging of the battery.

@sergeuz sergeuz force-pushed the feature/low_batt_event branch from a11615b to 43e1289 Dec 9, 2016

@sergeuz

This comment has been minimized.

Copy link
Member Author

commented Dec 9, 2016

Updated system_power_management_update() so that low_battery event can be generated again once the battery starts to charge. Rebased on top of current develop.

@technobly technobly merged commit 115f007 into develop Dec 10, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@technobly technobly deleted the feature/low_batt_event branch Dec 10, 2016

@technobly technobly removed their assignment Dec 10, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.