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

AOD fixup #2106

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

AOD fixup #2106

wants to merge 5 commits into from

Conversation

mark9064
Copy link
Contributor

  • Removed ROUNDED_DIV macro which doesn't make sense to rely on here (it's from the NRF SDK).
    • Would an namespace {} function be better than the inline one? Any ideas?
  • Motion is now updated when in AOD (as watchfaces display steps and this should update if in AOD).
  • Idle refresh rate increased from 2Hz to 8Hz. This reduces visible display strobing in AOD particularly on apps with a brighter background such as Twos

Before merging this the power impact of the 8Hz idle commit needs to be tested; if it uses a lot more energy then it might not be worth the improved visuals. I don't have a power profiler so I'd really appreciate if someone could compare AOD power usage with that commit applied directly to main compared to current main (other commits in this branch like motion updates could affect results)

Copy link

github-actions bot commented Aug 23, 2024

Build size and comparison to main:

Section Size Difference
text 374200B -32B
data 948B 0B
bss 63480B 0B

@mark9064
Copy link
Contributor Author

This PR now depends on #2109

Looking at AOD again, re-using Idle in DisplayApp and Sleeping in SystemTask wasn't a great idea as these states now have 2 different meanings depending on whether AOD is enabled. Also, if something changes the AOD enablement state while in AOD, all hell breaks loose (currently this isn't possible, but an auto wake/sleep pr like is currently proposed would cause problems here) and generally a brittle design like this is something to avoid.

So this PR will also add a new AODSleeping state to SystemTask and an AOD state to DisplayApp. But adding those states depends on the state refactor, as the waking up state gets in the way among other things

@mark9064 mark9064 marked this pull request as ready for review September 22, 2024 15:18
@mark9064
Copy link
Contributor Author

mark9064 commented Sep 22, 2024

Code changes are now ready to review

The power of 8Hz idle should still be checked. Based off how battery life is for me it is worse but not by much, and noticeable flickering is resolved

@JF002 you suggested a state diagram or similar in #2109 which I didn't have a chance to finish before merging. This PR adds a few states anyway so perhaps that was for the best! Anyway, what format do you think would work best? ASCII diagram in the code somewhere (if so where)? SVG state diagram? I'm not really sure how to present it

Copy link
Contributor

@NeroBurner NeroBurner left a comment

Choose a reason for hiding this comment

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

I don't have a voltage meter. But the code changes are nice. More explicit AOD states is good in my opinion

@NeroBurner NeroBurner added the enhancement Enhancement to an existing app/feature label Sep 28, 2024
@NeroBurner NeroBurner added this to the 1.15.0 milestone Sep 28, 2024
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants