PiSupply / PiJuice

Resources for PiJuice HAT for Raspberry Pi - use your Pi Anywhere
https://uk.pi-supply.com/collections/pijuice/products/pijuice-portable-power-raspberry-pi
GNU General Public License v3.0
438 stars 104 forks source link

Weird behaviour when using "no battery power on" and "power off" event under some circunstances #596

Open ccapublic opened 3 years ago

ccapublic commented 3 years ago

pijuice_sys.zip

Hi all !

The company I am working for is planning to implement PiJuice in some of its products. Both our R&D & Industrialization teams have faced some issues (v1.4) that we'd like to submit.

After running many test scenarios, we’ve found several blocking issues. We implemented some workarounds (not really clean fixes), but « forking » the service code is clearly not the good way to go.

You’ll find below a short description of the issues we faced and the way we did workaround them. We would be more than happy that our observations are confirmed, and hopefully that clean fixes are implemented in a later release.

I am conscient this post shall probably have been split. Hopefully this can be done later on if our observations are confirmed.

Issue #1

Description : • If system tasks are enabled and power off event is associated (in our case to standard SYS_FUNC_HALT_POW_OFF script). • If power gets lost during boot (that is after PiJuice powers raspi on and before service actually starts) • Power loss is not handled properly : raspi remains ON until battery is exhaused or reaches « low charge » level (if set) • Activating « low charge » as a workaround is not an option for us : system MUST halt/power off as soon as possible upon power loss.

Quick Fix :

• Modification of pijuice-sys.py • At service startup, if power off event is set and power is actually off, piggily and immediately trigger a halt + power off

Fix #2 (inserted early at service startup)

if configData.get('system_task', {}).get('enabled') and noPowEn:
    weHavePower = _CheckPower()
    if not weHavePower :
       pijuice.power.SetSystemPowerSwitch(0)
       pijuice.power.SetPowerOff(60)
       _SystemHalt('no_power')
       while dopoll:
            time.sleep(1)

Fix #2 (part of piggy fix for "won't detect power loss if it occured during boot")

def _CheckPower(): usbPowerGood = True ioPowerGood = True result = pijuice.status.GetStatus() if result['error'] == 'NO_ERROR': status = result.get('data', {}) if( status.get('powerInput', 0) != "PRESENT" ): usbPowerGood = False if( status['powerInput5vIo'] != "PRESENT" ): ioPowerGood = False

weHavePower = usbPowerGood or ioPowerGood

return weHavePower

Issue #2

Description : • If « no battery turn on » option is active • If battery is actually missing (or almost dead, ie << 2.8V - 2.7V in the fix) • If PiJuice service is configured to check battery charge or voltage • When power is turned on, PiJuice properly starts raspi… • …but then service runs the event script associated to minChg or minBatVolt (which usually triggers a system shutdown) • This makes the « no battery turn on » option unusable in conjonction with battery monitoring system tasks (systems turns on then off…)

Quick Fix : • Modification of pijuice-sys.py • In the main loop (called every 5s) • If battery looks absent or dead AND « no battery turn on » is set, ignore any battery check

Fix # 1 (in main loop)

    _UpdateBatteryStatus()
                if ( not optNoBatPowerOn or not batMissingOrDead ):
                    if minChgEn:
                        _EvalCharge()
                    if minBatVolEn:
                        _EvalBatVoltage()
                    if noPowEn:
                        _EvalPowerInputs(status)

Fix #1 (If 'boot on no battery' option is active and battery is missing or dead, do not shutdown right after boot... Obviously...)

def _UpdateBatteryStatus(): global optNoBatPowerOn global batStatus global batVoltage global batMissingOrDead

optNoBatPowerOn   = True
batStatus         = ""
batVoltage        = 4200
batMissingOrDead  = False

result = pijuice.config.GetPowerInputsConfig()
if result['error'] == 'NO_ERROR':
    pow_config = result['data']
    optNoBatPowerOn = pow_config['no_battery_turn_on']

result = pijuice.status.GetStatus()
if result['error'] == 'NO_ERROR':
    status = result.get('data', {})
    batStatus = status['battery']

result = pijuice.status.GetBatteryVoltage()
if result['error'] == 'NO_ERROR':
    batVoltage = float( result.get('data', {}) )

batMissingOrDead = batVoltage < 2700 or (batStatus == "NOT_PRESENT")

Suggestion #1

Description : • Unable to automate configuration of « STM32 FW parameters » • Namely « no battery power on » and battery profiles. • It looks like json file contains only system level parameters, not the options above

Quick Fix : • Find out how pijuice-cli pushes those parameters • Create our own script based on it • Piggy + risky if inner API gets modified (BTW latest release seems to have broken this script)

Suggestion : • Introduce « STM32 parameters » into json file • Have service apply those parameters upon startup if different from current config • --- OR --- • Have pijuice-cli accept a conf file as parameter. If passed, silently apply the parameters then gently exit. We would call this method whenever config has to be applied (minimum a first boot)

tvoverbeek commented 3 years ago

@ccapublic Regarding Suggestion #1 (setting STM32 FW parameters) have a look at the recently added https://github.com/PiSupply/PiJuice/tree/master/Software/Source/Utilities. It addresses loading and dumping these parameters.

ccapublic commented 3 years ago

@ccapublic Regarding Suggestion #1 (setting STM32 FW parameters) have a look at the recently added https://github.com/PiSupply/PiJuice/tree/master/Software/Source/Utilities.

Thanks @tvoverbeek. We'll be having a look.

I hope we'll also get feedback regarding the issues, as they are the main pb today.

tvoverbeek commented 3 years ago

For issue #1 there is a simpler solution if you can live with a 5 second delay before the shutdown. In line 35 of pijuice_sys.py: https://github.com/PiSupply/PiJuice/blob/a050850c1295d2c34198f1217103c94a7a44dce9/Software/Source/src/pijuice_sys.py#L35 change the initial value of noPowCnt to 0. The current logic only reacts on the no power event when the power has been detected present at least once.

For issue #2 I would use my own script running as a one-shot service before the pijuice service starts (requires modification of the pijuice service file to wait till your one shot service completes: requires ..., after ....) The script should then, when no battery present (your _UpdateBatteryStatus), disable the low charge and min bat voltage events. Then the pijuice service can run as is.

ccapublic commented 3 years ago

Thanks. I'll be forwarding your advices to the persons in charge. That being said, though more simple and/or cleaner, they still imply modifications to the service and to be honest I'm not fond of living with a modified service vs main branch. Hopefully our remarks make sense to most users and can be somehow fixed in the said main branch. Do you think this has a chance to happen ?Thanks again for taking time to help. Much appreciated.Cyril.

ccapublic commented 3 years ago

Hi all. Any chance a member of dev team confirms/infirms the phenomenon described above, and ideally if they can addressed in a future release ?

Thanks again !

shawaj commented 3 years ago

@mmilann any thoughts?

tvoverbeek commented 3 years ago

See my comment above. Maybe we could add a hook for a user script running before the service starts to handle issues as issue 2 (if no battery present, disable low charge and min bat voltage)

mmilann commented 3 years ago

@ccapublic suggestions looks reasonable and it is about way how to introduce in update. System task is made simple to read in order to be easily customised for specific application, you can decrease polling period from 5 to 1 in order to have faster response. Can you give more details about your use case, is it UPS, most of time running with power present or it is field application where Pi mostly runs on battery power?

As @tvoverbeek suggested it would be more cleaner to think about plugin logic with user scripts and update sys to support these situations. There can be few default user scripts included into package, and option to select/enable from gui.

ccapublic commented 3 years ago

Thanks for taking time to answer.

Ok, let me tell you more about the use case. The short answer is UPS. For now, we need to stay alive just enough time to properly shutdown our processes then haltpowoff the system (so ~1 minutr). Our main goal being "inform our cloud services and avoid SD card corruption". Of course we shall run again as soon as possible when power resumes (waiting 10s of seconds is ok, not minutes)

==>> This leads to one of the issues : Haltpowoff on no power is exactly what we need. The problem is not that it takes 1s, 5s or 10s to react, but simply that it does not work if power gets lost during boot. We do not think this is an intentional behaviour (else maybe you can explain the use case behind it ?), so why not "simply" fixing this ?

Back to the use case. As you probably noticed, battery is clearly "underused". However, because we nevertheless need it, it introduces painful constraints :

1- we need to keep its lifespan as long as possible to relieve maintenance constraints as much as possible. We plan to change profile so that it never gets charged above ~50 percent. As far as we were advised, this is better for chemistry and 50% is more than enough to cover our "once in a while 1 minute autonomy" needs. Thus the need to automate configuration of stm32 params. This is now possible on latest releases.

2- if for some reason the battery dies (failure, disrespect of maintenance guidelines, very long offline storage without removing battery...), we prefer taking the risk of losing "sd corruption protection" than never booting again or having someone go and press sw1. Here again there is a perfectly fitting option "power on even if no bat'. But right after booting the service will halt the system if a safety "power off on low.charge" event was set. We do not understand what is the interest of the "power on even if no bat" if the service subsequently shuts the system down because we set a safety low charge threshold (for the most occuring case where battery is NOT dead and shall be protected). Doesnt it sound reasonable to add the following as a builtin (optional ?) behaviour : "if we booted from absent/dead battery service shall ignore battery level checks" ?

We do not see any negative side effect of both suggestions, this is why we believe it may be worth implementing them "built in" (whatever the architecture and implementation choices). But maybe we missed an evidence so don't hesitate to "open our eyes" :D

Thanks for reading !

Cyril

mmilann commented 3 years ago

Thanks for explanation @ccapublic . Currently there are undergoing firmware/software updates, that will improve reliability of wake-up for UPS applications when power resumes. Issues with missing noPowerEvent and missing battery will be revised and addressed with system task updates. Functions for import/export firmware configurations, potentially could be made as new I2C command, if it is possible to read/write whole configuration memory block.

Yes UPS can benefit by setting lower regulation voltage in battery profile with pro-longed battery life.

ccapublic commented 3 years ago

Thank you @mmilann. We'll be living with our "dirty quick fixes" for now and will be happy to trash them once the improvements you are mentionning are implemented. Thanks again ! Cyril.