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.

Sign up for GitHub

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

Jump to bottom

Fixes abnormal battery charged behavior and adds a test for battery state deducing. #2290

Merged
merged 10 commits into from
Apr 7, 2021
Merged

Fixes abnormal battery charged behavior and adds a test for battery state deducing. #2290

merged 10 commits into from
Apr 7, 2021

Conversation

XuGuohui
Copy link
Member

@XuGuohui XuGuohui commented Feb 26, 2021 •
edited
Loading

Add a manual test application for validating battery state. This is a supplement of #2272.

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)

avtolstoy
avtolstoy requested changes Feb 26, 2021
View reviewed changes
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.

This is a good candidate to have as a 'test application', however using the test framework here doesn't bring any benefits and will not allow us to easily migrate this test suite to be run in an automated manner.

Is there any way we can figure out several bite-sized test cases that we can run in a semi-automated manner similar to wiring/power_management with appropriate assertions in place, so that we don't have to eyeball the behavior of the device?

@XuGuohui
Copy link
Member Author

I did take some time thinking how we can automate the test. My thinking was that the test application prompts tester to set up the hardware, for example, "Please plug the battery and USB", then the application would automatically enable/disable charging and make some assertion according to the current hardware setup. After the test case runs for a period, then prompt tester to set up the hardware in another manner and continue the next test case, and so on. The assertions would be that the power source and battery state should be consistent under certain hardware setup. How does the plan sound?

@avtolstoy
Copy link
Member

@XuGuohui Yes, that sounds much better:

  1. We have bite-sized test cases
  2. The parts that cannot be automated as of right now require a human operator to perform a specific action
  3. We have proper validation (assertions) that with specific conditions set by human operator a particular test case passes
  4. We'll be able to more easily refactor the tests to be performed in an automated manner by just replacing the parts that as of right now require human intervention

@eberseth
Copy link
Contributor

I had a similar feeling of wanting it to setup and verify something instead of display results.

@XuGuohui XuGuohui requested a review from avtolstoy March 1, 2021 16:53
@XuGuohui XuGuohui force-pushed the test/system_power_long_running/ch73611 branch from a39d97b to 2e9f63f Compare April 1, 2021 17:27
@XuGuohui XuGuohui changed the title [test]: add test for battery state deducing. Fixes abnormal battery charged behavior and adds a test for battery state deducing. Apr 1, 2021
@XuGuohui XuGuohui requested a review from avtolstoy April 1, 2021 17:31
avtolstoy
avtolstoy approved these changes Apr 5, 2021
View reviewed changes
system/src/system_power_manager.h Outdated 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
user/tests/wiring/power_management_long_running/power.cpp Show resolved Hide resolved
user/tests/wiring/power_management_long_running/power.cpp
return (System.batteryState()!= BATTERY_STATE_UNKNOWN) && (System.powerSource() == POWER_SOURCE_USB_HOST);
}, 10000));
PMIC power;
assertTrue(power.isChargingEnabled());
Copy link
Member

Choose a reason for hiding this comment

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

This failed for me a number of with battery at around 4.02V.

Copy link
Member Author

Choose a reason for hiding this comment

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

This fails only if the SystemPowerFeature::DISABLE_CHARGING is true or the battery is disconnected. Otherwise, the PMIC might have tweaked its register for some reason.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's probably that on startup we get continuous charged event even if when we raise the recharge threshold to be 300mV

user/tests/wiring/power_management_long_running/power.cpp Outdated Show resolved Hide resolved
@avtolstoy avtolstoy added this to the 3.0.0 milestone Apr 6, 2021
@avtolstoy avtolstoy force-pushed the test/system_power_long_running/ch73611 branch from cda6155 to 602eee5 Compare April 7, 2021 05:23
@avtolstoy avtolstoy merged commit 15ec689 into develop Apr 7, 2021
@avtolstoy avtolstoy deleted the test/system_power_long_running/ch73611 branch April 7, 2021 05:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eberseth eberseth eberseth approved these changes

@avtolstoy avtolstoy avtolstoy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
3.0.0
Development

Successfully merging this pull request may close these issues.
3 participants
@XuGuohui @avtolstoy @eberseth