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

Allow power manager to disable charging #2257

Merged
merged 1 commit into from
Dec 22, 2020

Conversation

eberseth
Copy link
Contributor

@eberseth eberseth commented Dec 19, 2020

Problem

The power manager may override user requests to disable battery charging. While the user may set the charge state on the PMIC, the power manager may enable charge state silently.

Solution

Add a flag for non-volatile power management configuration that ensures that the power manager honor user requests to disable charging. Additionally create get methods for the configuration so that the user can query the current configuration.

Steps to Test

Which unit/integration/application tests are applicable to this code change? (At minimum a test of some kind should be provided)

Example App

SerialLogHandler logHandler;

void setup() {
    // Apply a custom power configuration
    SystemPowerConfiguration conf;

    conf.feature(SystemPowerFeature::DISABLE_CHARGING);

    int res = System.setPowerConfiguration(conf);
    Log.info("setPowerConfiguration=%d", res);
    // returns SYSTEM_ERROR_NONE (0) in case of success

    // Settings are persisted, you normally wouldn't do this on every startup.
}

void loop() {
    auto config = System.getPowerConfiguration();
    Log.info("Current PMIC settings:");
    Log.info("PMIC detection: %s", cfg.isFeatureSet(SystemPowerFeature::PMIC_DETECTION) ? "true" : "false");
    Log.info("Use VIN with USB: %s", cfg.isFeatureSet(SystemPowerFeature::USE_VIN_SETTINGS_WITH_USB_HOST) ? "true" : "false");
    Log.info("Power manager disabled: %s", cfg.isFeatureSet(SystemPowerFeature::DISABLE) ? "true" : "false");
    Log.info("Charging disabled: %s", cfg.isFeatureSet(SystemPowerFeature::DISABLE_CHARGING) ? "true" : "false");
    Log.info("Input voltage: %u mV", cfg.powerSourceMinVoltage())
    Log.info("Input current: %u mA", cfg.powerSourceMaxCurrent());
    Log.info("Charge voltage: %u mV", cfg.batteryChargeVoltage());
    Log.info("Charge current: %u mA", cfg.batteryChargeCurrent());
}

References

N/A


Completeness

  • User is totes amazing for contributing!
  • Contributor has signed CLA (Info here)
  • Problem and Solution clearly stated
  • Run unit/integration/application tests on device
  • Added documentation
  • Added to CHANGELOG.md after merging (add links to docs and issues)

wiring/inc/spark_wiring_system.h Outdated Show resolved Hide resolved
wiring/src/spark_wiring_power.cpp Show resolved Hide resolved
system/inc/system_dynalib.h Outdated Show resolved Hide resolved
system/src/system_power_manager.cpp Show resolved Hide resolved
system/src/system_power_manager.cpp Outdated Show resolved Hide resolved
wiring/inc/spark_wiring_power.h Show resolved Hide resolved
@monkbroc monkbroc removed their request for review December 21, 2020 16:28
@monkbroc
Copy link
Member

I'll let Guo-Hui and Andrey review. This is beyond my knowledge to meaningfully review.

Copy link
Member

@avtolstoy avtolstoy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mainly due to dynalib change and ABI problem with getConfig().

system/src/system_power_manager.cpp Show resolved Hide resolved
system/src/system_power_manager.cpp Outdated Show resolved Hide resolved
system/src/system_power_manager.cpp Outdated Show resolved Hide resolved
system/src/system_power_manager.cpp Outdated Show resolved Hide resolved
system/src/system_power_manager.cpp Outdated Show resolved Hide resolved
wiring/inc/spark_wiring_power.h Show resolved Hide resolved
wiring/inc/spark_wiring_system.h Outdated Show resolved Hide resolved
wiring/inc/spark_wiring_system_power.h Outdated Show resolved Hide resolved
wiring/src/spark_wiring_power.cpp Show resolved Hide resolved
assertEqual(cfg.getPowerSourceMinVoltage(), particle::power::DEFAULT_INPUT_VOLTAGE_LIMIT);
assertEqual(cfg.getBatteryChargeCurrent(), particle::power::DEFAULT_CHARGE_CURRENT);
assertEqual(cfg.getBatteryChargeVoltage(), particle::power::DEFAULT_TERMINATION_VOLTAGE);

assertEqual(System.batteryState(), (int)BATTERY_STATE_UNKNOWN);
assertEqual(System.powerSource(), (int)POWER_SOURCE_UNKNOWN);
// Restore default power management configuration
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: (can be a separate PR) let's add three tests 1) disable/enable charging in runtime and verifying configuration and actual PMIC state 2) disable/enable charging, resetting and making sure that the necessary charging state is set on boot 3) disable/enable chargin, triggering PMIC watchdog and making sure that the necessary charging state is achieved after watchdog timer expires.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'm still testing some of this code so I didn't add the tests here yet.

@avtolstoy avtolstoy added this to the 3.0.0 milestone Dec 21, 2020
@eberseth eberseth force-pushed the feature/ch66660/power-manager branch from 3f17480 to 62444ee Compare December 22, 2020 04:09
system/inc/system_dynalib.h Show resolved Hide resolved
system/src/system_power_manager.cpp Outdated Show resolved Hide resolved
system/src/system_power_manager.cpp Show resolved Hide resolved
wiring/inc/spark_wiring_system.h Outdated Show resolved Hide resolved
@eberseth eberseth force-pushed the feature/ch66660/power-manager branch from 62444ee to e74d98e Compare December 22, 2020 16:16
@eberseth eberseth merged commit dbf4773 into develop Dec 22, 2020
@eberseth eberseth deleted the feature/ch66660/power-manager branch December 22, 2020 17:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants