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

Power optimization - Increase SystemTaskPeriod. #1757

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

JF002
Copy link
Collaborator

@JF002 JF002 commented May 7, 2023

Increase the timeout on the message queue in SystemTask. This reduces the power consumption by 60-70µs in sleep mode.

Increase the timeout on the message queue in SystemTask. This reduces the power consumption by 60-70µs in sleep mode.
@JF002 JF002 added the enhancement Enhancement to an existing app/feature label May 7, 2023
@github-actions
Copy link

github-actions bot commented May 7, 2023

Build size and comparison to main:

Section Size Difference
text 407708B 48B
data 940B 0B
bss 54136B 0B

@JF002 JF002 mentioned this pull request May 7, 2023
@mark9064
Copy link
Contributor

mark9064 commented May 8, 2023

Super cool to see all the power optimisations inbound!
Unfortunately this PR means that the time is only updated every 4s when the watch is on, which is a problem for watch faces with seconds on them. It also means chimes can arrive 4s late, though I suppose this is less of a problem (it's still nice to have them arrive right on the hour though)
Edit: Also breaks motion based wake as the motion data is only updated every 4s too.
Maybe it could be changed to update more often when the display is on?

@FintasticMan
Copy link
Member

No motion-based wake algorithm will work with this massively increased period. Perhaps it could be set to 4 seconds if it's sleeping and there are no motion wake algorithms enabled?

@JF002
Copy link
Collaborator Author

JF002 commented May 8, 2023

Yes, you're both right, this optimization is a bit too aggressive.

Perhaps it could be set to 4 seconds if it's sleeping and there are no motion wake algorithms enabled?

That's probably the way to go !

Improve the timeout on the queue : by default, the timeout is 100ms, and it's extended to 4s in sleep mode, when no motion based wake up option is enabled.
@JF002 JF002 force-pushed the power-optimization-increase-system-task-period branch from 525f1a2 to 491d3ae Compare May 8, 2023 19:24
src/systemtask/SystemTask.cpp Outdated Show resolved Hide resolved
@Riksu9000
Copy link
Contributor

There are many things that rely on the SystemTask loop period, not just motion. Have you considered all of them so they don't cause issues? Ideally all periodic tasks should be moved away from the message handling loop, so the period can be set to infinite.

In your measurement, did you have motion wake up methods enabled, i.e. did UpdateMotion() return early? If not, I'd like to see the power consumption measured again with two different periods, without the automatic period switching, but motion disabled in both tests by disabling motion wake up methods.

@JF002
Copy link
Collaborator Author

JF002 commented May 18, 2023

There are many things that rely on the SystemTask loop period, not just motion. Have you considered all of them so they don't cause issues? Ideally all periodic tasks should be moved away from the message handling loop, so the period can be set to infinite.

The period cannot be set to Infinite, it must actually be shorter than the period of the watchdog. That's exactly why I chose to run all background tasks in SystemTask : if SystemTask does not run correctly, we cannot guarantee the good state of the system, so we let the watchdog reset the board.

In your measurement, did you have motion wake up methods enabled, i.e. did UpdateMotion() return early? If not, I'd like to see the power consumption measured again with two different periods, without the automatic period switching, but motion disabled in both tests by disabling motion wake up methods.

I've just done the measurements :

  • Wake on wrist / shake wake enabled : period set to 100ms : 890µA.
  • Wake on wrist / shake wake disabled : period set to 4s : 780µA

Note that motion algorithms and low level driver could probably be improved by using the hardware FIFO and the "motion/no-motion" interrupt pin to reduce the power usage.

Increase the SystemTask period also when the notification mode is set to Sleep (as it also disables the motion-based wake options).
@Riksu9000
Copy link
Contributor

I went through the tasks that are run in the loop and the noteworthy observations are that the system time may only be updated once every four seconds, which could have unintended consequences, and BleDiscoveryTimer is not working as intended. There's a comment that says service discovery is deferred for 3 seconds, but this doesn't seem to be true, and changing the period will impact it further. It should be re-evaluated and probably use a proper timer.

I've just done the measurements :

I would like to see the power consumption difference between the different periods set statically, with motion disabled in both tests. The behavior when motion is enabled shouldn't be affected by this PR, so we're only interested in seeing the difference when motion is disabled. Or was this how you measured the 60-70µA difference?

@JF002
Copy link
Collaborator Author

JF002 commented May 18, 2023

I would like to see the power consumption difference between the different periods set statically, with motion disabled in both tests.

  • Period set to 100ms : 850µA
  • Period set to 4000ms : 770µA

I went through the tasks that are run in the loop and the noteworthy observations [...]

This are valid concerns. I'll check them and see if I can solve those issue in this PR or if they'll need more work.

@JF002 JF002 added the needs more work This PR needs more work label May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhancement to an existing app/feature needs more work This PR needs more work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants