InfiniTimeOrg / InfiniTime

Firmware for Pinetime smartwatch written in C++ and based on FreeRTOS
GNU General Public License v3.0
2.64k stars 904 forks source link

Always on display #1869

Open KaffeinatedKat opened 9 months ago

KaffeinatedKat commented 9 months ago

I have added an option in the "Display timeout" for an always on display. Not sure how useful this is, or how good the battery life is (will be testing this and will post a comment with how long the battery lasted).

Any suggestions on how I could possibly increase the battery with the display always on would be great.

image

KaffeinatedKat commented 7 months ago

Took a few tries to hit it as I was blind, but switching off and back on again (just with power button) after that brought the display to life (though with inverted colours). Curious to hear if this hack works for you

This does bring back the display for me, colors do not always invert though

mark9064 commented 6 months ago

OK I managed to work around rewriting most of InfiniTime's handling of time. It's still not an ideal implementation (as the Minutes() Seconds() etc methods of the datetimecontroller are not called atomically in blocks allowing the time to be updated between calls by another task) but the time jitter issue should be resolved.

Still no clue on the display blanking with the analogue face :( I've tried a few things such as waiting for SPI transactions to fully finish before doing anything else but no luck yet. Continuing to look into it

PWM is the next target

Sorry it's been so slow :sweat_smile:

KaffeinatedKat commented 5 months ago

I merged the new 1.14 changes, and I was hoping that the new changes to how the analog face is drawn would fix the display blank, but unfortunately it did not. Still stuck with no second hand with the aod. @mark9064 Looking forward to seeing your recent changes on your branch, i'd love to help clean it up if you wanted to open a pull request, got a lot of time on my hands

mark9064 commented 5 months ago

I've been thinking a lot about our usage of PWM, which adds a non-negotiable 150uA consumption. We should be able to replace this with a timer and PPI to the GPIOTE which essentially emulates PWM for us without having the power hungry PWM clock. This should get us down to 5uA. We can also stop the timer when modulation isn't needed, but as a more general approach this should allow efficient backlight modulation

Gonna have a shot at implementing this soon. I'll try get the 2Hz backlight changes up

mark9064 commented 5 months ago

I've got this working. Been working on this every day, absolute nightmare as the previous SPI erratum 58 workaround was silently overwriting the events - but I've fixed that now so we're good to go. Haven't had a chance to evaluate power usage yet. The refactor only uses TIMER/PPI when needed and only ever controls one pin, so hopefully current consumption should be reduced. It also only re-initialises the channels when switching output pin so calling setBrightness() every frame to create a smooth brightness fade should be performant.

Will push it when I have a chance tomorrow. After that we have just the analogue issue to resolve (confirmed not to be fixed by these changes) and this branch should be good to go after a rebase (to get the commits into a sensible order!)

Edit: There are still occasional frame pacing issues for 2Hz, but I think this is an artefact of the panel rather than anything else. Still open to any ideas regarding this one, but it's not severe so I think it can be ignored for now

KaffeinatedKat commented 5 months ago

That's great to hear, can't wait to see these changes with a hopefully even better battery life.

Some info on the analog display blank: I've been messing with the analog face and i've found that it's not the second hand that causes the issue.

If I remove all the if statements from WatchFaceAnalog::UpdateClock() so the hands are redrawn every time it's called, then remove the part that does the second hand, the issues still occurs.

Currently looking for a way to disable the watchface from redrawing the hands while changing sleep states, i'll let you know if I get anywhere

mark9064 commented 5 months ago

Is there any chance you could rebase this branch onto main rather than merge main in? Currently it's a little tricky for me to review the work done on the branch because there are 70 commits from main sandwiched between what I want to add and what we did on the branch earlier

I think github filters them out when rendering but I can't seem to find an easy way to get Git to do that normally

KaffeinatedKat commented 5 months ago

i've added you as a collaborator to the repo, i'm not super well versed in git and i'm not sure how to accomplish this

mark9064 commented 5 months ago

Oh thanks! That'll work. I'll get it rebased, but I should explain what I actually mean first. So when you merge main into this branch, you fundamentally take all the commits on main and append them to this branch. But when you rebase, you take all the commits on the branch, and then append them to a fresh copy the new up to date main branch. So this way you don't have any merge commits in the history, you have only the new commits you added.

To achieve this you run git rebase <branchname>. So for main that's git rebase main (make sure to update your local main branch first!). But now you have a problem: the branch on the remote and the local branch you've just created from the rebase now have different versions of history. On one of them, the commits you wrote were added at some point in the past, and on the rebased one they were added right on the tip of the main branch. Since you want to keep the history where the commits were added to the fresh main, you have to force push and overwrite what's on the remote. This can cause problems for other contributors as they won't be able to git pull due to the diverging histories on the branch (they will have to reset their branches, and copy back over changes they've made which is super painful).

So common practice is only to rebase a branch if you're the only one working on it, because it causes huge problems for others with these diverging histories. The best way to avoid all these problems though? Just don't add new changes from main into the branch being worked on - only merge things in if needed (if you want to try the PR branch as if it were on the latest main, create a local copy of the PR branch and rebase that local copy onto main). This keeps the history clean, and when the PR is merged into the main branch Git automatically figures out that the new commits need to go on the end of main.

--

Now I've written all that, I've realised that rebasing it will break your stuff so I'll just work around it locally, it's not too big of a deal actually. Hey maybe you got something out of this essay at least!

KaffeinatedKat commented 5 months ago

thanks for the explanation, I assumed keeping it up to date with main was important but when explained like that it makes sense why you would wait to merge with the main branch assuming there is no major changes to adjust for

mark9064 commented 5 months ago

PR is up! https://github.com/KaffeinatedKat/InfiniTime/pull/3

mark9064 commented 5 months ago

Fixed the above two issues, minor changes so hopefully you don't mind me pushing directly (I'll always PR for actual work so you can review)

Edit: Just fixed formatting for the other files as well. I've also now set up the pre-commit hook on my machine

KaffeinatedKat commented 5 months ago

The analog display blank issue is mysterious, and I honestly think the best solution here is to just keep the second hand disabled while the aod is on (as it is now). This issue only happens when waking up.

I've tried adding in delays, stopping the watchface from updating while waking up, making sure the hands aren't being drawn outside of the screen, removing the call to wakup to spiNorFlash (i'm just throwing shit at the wall and seeing if anything sticks at this point).

Unless anyone has any ideas on why this issue is happening, removing the second hand appears to be the best solution

mark9064 commented 5 months ago

There's certainly some deeper display issue here. At the moment I'm reading through all the SPI code to try and get an idea of the exact issue. I suspect it's behind the display invert problem, and also behind the bug where the button text on the timer has a ~10% chance of becoming a bit corrupted when the timer wakes the watch from AOD (set the timer to a few seconds and use lower to sleep to get fast repeats)

I can trigger display inversion without even needing AOD on the analogue face (just using sleep notifications mode to disable it and spamming the button). I've only just discovered this today - I need to test if this happens on the current main

mark9064 commented 5 months ago

Normal analogue face bug not reproducible on main so for sure there are some SPI race conditions or some overtightened timings - not sure which. Continuing to investigate

KaffeinatedKat commented 5 months ago

I think that https://github.com/InfiniTimeOrg/InfiniTime/issues/1984#issuecomment-1919625745 this gives us a little bit more insight on what is happening here. The analog issue only happens when turning the screen on, and this issue happens when turning the screen on. Both of these actions draw to the screen. I think the issue might lie with drawing the screen and also modifying the state of the screen at the same time, causing some kind of race condition and bits are written to the wrong place, so the display freaks out and either turns off or inverts the colors

This whole SPI thing flies right over my head, but i'm going to attempt to learn how it all works so we can hopefully figure out this nasty bug a bit faster. Let me know if you have any more updates on it

mark9064 commented 5 months ago

Yep, I've arrived at the same conclusion!

My current theory is that the display switches from panel data write mode to command mode too early and interprets the image data as commands somehow. Not sure how close I am though. I think it corroborates with the analogue face issues as well, as the analogue face creates the largest screen paints regularly (repainting the second hand requires changing a good portion of the screen)

FintasticMan commented 5 months ago

Thank you people for your hard work in implementing this very useful feature and squashing bugs! I'm sorry I've not been very involved with the PR. When this is ready for review, could you organise the commits into standalone changes, as right now it's quite a lot to try to review in one go 😅. I'm looking forward to using the always on display!

mark9064 commented 5 months ago

Yeah no worries, we are still trying to get the last display bugs ironed out then after that it can be rebased into something more sensible. Right now it's definitely a bit chaotic

mark9064 commented 5 months ago

Oh, another issue I just remembered. If always on is changed during sleep notification mode it will enter aod as opposed to turning the display off as usual

KaffeinatedKat commented 5 months ago

the always on setting is now correctly displayed as enabled while in notification sleep, and changing it while in notification sleep works correctly

mark9064 commented 4 months ago

I think I might've fixed the display issue. Fix is currently very hacky and needs a refactor, also not 100% sure I've got everything sorted here either (perhaps I just have good/bad luck with triggering the issue right now). Will be a while until I can work on it again as I'm still catching up with things after FOSDEM and I'm also away this weekend too. But hopefully soon...

mark9064 commented 4 months ago

the always on setting is now correctly displayed as enabled while in notification sleep, and changing it while in notification sleep works correctly

I might've messed up a local merge but this isn't working quite right for me. If I switch on always on display while in sleep mode, always on will activate in sleep until I toggle notifications to normal and back to sleep again

KaffeinatedKat commented 4 months ago

Best way to check for sure is to just run the aod with the analog face for a day or 2. If the issue is still there it'll eventually trigger

-------- Original Message -------- On 2/8/24 17:52, mark9064 wrote:

I think I might've fixed the display issue. Fix is currently very hacky and needs a refactor, also not 100% sure I've got everything sorted here either (perhaps I just have good/bad luck with triggering the issue right now). Will be a while until I can work on it again as I'm still catching up with things after FOSDEM and I'm also away this weekend too. But hopefully soon...

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

mark9064 commented 4 months ago

Pretty confident the fix is good now. Should also clear up the random display inverts people get. Going to PR this in hopefully tomorrow after some code cleanup

KaffeinatedKat commented 4 months ago

That's awesome to hear, can't wait to have that fixed. What was the cause of the issue? Very interested to know

-------- Original Message -------- On 2/10/24 17:50, mark9064 wrote:

Pretty confident the fix is good now. Should also clear up the random display inverts people get. Going to PR this in hopefully tomorrow after some code cleanup

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.Message ID: @.***>

mark9064 commented 4 months ago

The problem was that the LCD can be switched to command mode (from data) while the previous SPI transaction was still ongoing, so the pixel data got interpreted as commands. I didn't even know the pin to switch between command mode and data mode existed before investigating this!

KaffeinatedKat commented 4 months ago

the always on setting is now correctly displayed as enabled while in notification sleep, and changing it while in notification sleep works correctly

I might've messed up a local merge but this isn't working quite right for me. If I switch on always on display while in sleep mode, always on will activate in sleep until I toggle notifications to normal and back to sleep again

I have actually fixed this now. The previous fix changed the UI toggle to be displayed properly, and disabling aod while in notification sleep. You can now enable the aod setting while in notification sleep and the display will turn off as expected till you leave notification sleep

mark9064 commented 4 months ago

OK, with https://github.com/KaffeinatedKat/InfiniTime/pull/4 this PR is ready functionally It won't build right now because it needs the removal of GFX which just got merged into main

I've managed to entirely remove task notifications from InfiniTime at the RTOS level, as SPI was previously the only user. This should save a few bytes of memory for every task that is created. The pre/post transaction hook system isn't lovely but I think it's the best solution we can do here. If someone with more C++ knowledge knows a fancier way to do it please enlighten me to your ways!!

Hopefully https://github.com/KaffeinatedKat/InfiniTime/pull/4 looks OK. If so, I'll start rebasing this branch onto main so it can be reviewed properly

mark9064 commented 4 months ago

Quick list of other issues this happens to resolve

KaffeinatedKat commented 4 months ago

awesome stuff man, good job figuring that out and fixing it. PR looks good to me, you can go ahead and rebase everything so this can be reviewed and hopefully merged

SteveAmor commented 4 months ago

@mark9064 @KaffeinatedKat would it be possible to break out the code for the SPI fix into a separate PR, therefore creating a small, focused pull requests that fulfills a single purpose? This will help with tracking.

KaffeinatedKat commented 4 months ago

@mark9064, would it be possible to break out the code for the SPI fix into a separate PR, therefore creating a small, focused pull requests that fulfills a single purpose? This will help with tracking.

This is probably a good idea, these SPI problems are a problem outside of this PR, this just exposed the issue enough that it needed to be resolved

SteveAmor commented 4 months ago

@mark9064, would it be possible to break out the code for the SPI fix into a separate PR, therefore creating a small, focused pull requests that fulfills a single purpose? This will help with tracking.

This is probably a good idea, these SPI problems are a problem outside of this PR, this just exposed the issue enough that it needed to be resolved

Absolutely fantastic work here 😀

mark9064 commented 4 months ago

Rebasing is in progress, on to main now and down to 32 commits. When it's in shape I'll repush it, and then I think we can discuss what changes we might want to break out

JF002 commented 4 months ago

I haven't had the opportunity to have a closer look at this PR, but I'm definitely interested in all the work that have been done here!

Splitting the PR into multiple PRs will probably help with the reviews since we'll be able to focus on a single topic with less code involved. Do you think it would be possible to isolate a few of the points mentionned in https://github.com/InfiniTimeOrg/InfiniTime/pull/1869#issuecomment-1937937984 into smaller PRs?

If this is too much work, maybe wait for me to have a closer look at this PR so we can find the best way to continue the review. I would not want you to do the extra work for nothing. I can't promise anything (I'm still recovering from a flu, it's taking more time than expected...), but I'll try to provide more feedback soon.

Another question : do you have any idea of the battery life with the always-on mode enabled?

mark9064 commented 4 months ago

Yeah, don't worry about reviewing this yet. I don't have a whole lot of free time right now so it'll take me a few days to reorder/combine/split things. I'll drop another message here when that's done.

After that we can discuss if we want to split parts out into other PRs (I think we will want to split a few)

Battery life is a bit more than 2 days with the watch in sleep mode for ~7h/day

mark9064 commented 4 months ago

Suggested PRs

  1. Continuous time updates (https://github.com/InfiniTimeOrg/InfiniTime/pull/1869/commits/7e178ccaa70c2e8b9b22c115527cfecb588b4430)
  2. Erratum 58 improvements (https://github.com/InfiniTimeOrg/InfiniTime/pull/1869/commits/0792c3dd8fc831ec190762e1c7a21368da9c9021)
  3. LCD delay improvements (https://github.com/InfiniTimeOrg/InfiniTime/pull/1869/commits/2438927b97d4b584f99d2090888ba382cc85d89a, https://github.com/InfiniTimeOrg/InfiniTime/pull/1869/commits/fb8eb479b29811c007f46cbad5e91a1962825cbc)
  4. SPI transaction hooks (https://github.com/InfiniTimeOrg/InfiniTime/pull/1869/commits/e5968882a717ad829546a21ef7dbb41cec053431, https://github.com/InfiniTimeOrg/InfiniTime/pull/1869/commits/c1b6ae1f0af93245c804e8fbcf35cc984d9b289b, https://github.com/InfiniTimeOrg/InfiniTime/pull/1869/commits/77327b4da89a18ab4d772d51575b2064e624decf)
  5. This PR (rest of the commits)

The commits above are all in order with respect to this PR

Rebase patch notes:

Original branch before rebase here if anyone would like to compare

Hopefully review is much easier now. The series of commits is certainly much more readable to me at least!

JF002 commented 4 months ago

I've just setup my devkit with my NRF PPKII (power measurement tool).

image

The PPK is connected between the watch and the battery, so it measures the whole power usage of the watch.

To get a baseline, I first measured the average current of the watch running InfiniTime 1.14

Now, using this branch without any change (on 7e178ccaa70c2e8b9b22c115527cfecb588b4430):

Here's the capture from the always mode : image

More zoomed in: image

The pattern repeats every 500ms : 130ms @ 1.92mA and then 370ms @ 4.02mA

Let me know what other measurements would be useful :)

KaffeinatedKat commented 4 months ago

Is there any noticible difference between running the display at 8 colors vs full color while in AlwaysOn?

-------- Original Message -------- On 2/17/24 12:49, JF wrote:

I've just setup my devkit with my NRF PPKII (power measurement tool).

image.png (view on web)

The PPK is connected between the watch and the battery, so it measures the whole power usage of the watch.

To get a baseline, I first measured the average current of the watch running InfiniTime 1.14

  • Max brightness : 23mA
  • Medium brightness : 13.5mA
  • Low brightness : 8mA
  • IDLE (BLE enabled, slow advertising (sleeping for more than 30s)) : 420µA

Now, using this branch without any change (on 7e178cc):

  • Max brightness : 26mA
  • Medium brightness : 13mA
  • Low brightness : 7.75
  • Always on mode : 3.5mA

Here's the capture from the always mode : image.png (view on web)

More zoomed in: image.png (view on web)

The pattern repeats every 500ms : 130ms @ 1.92mA and then 370ms @ 4.02mA

Let me know what other measurements would be useful :)

— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you were mentioned.Message ID: @.***>

JF002 commented 4 months ago

@KaffeinatedKat

Is there any noticible difference between running the display at 8 colors vs full color while in AlwaysOn?

How can I check this?

mark9064 commented 4 months ago

I've got a set of test cases I can hopefully send to you in an hour or so

mark9064 commented 4 months ago

aod_testing.md Sorry for the delay. All the branch names refer to the ones on my repository, not this branch here. I spotted a rebase mistake meaning that the previous testing you did might not quite be accurate

https://github.com/mark9064/InfiniTime/branches

TLDR The tests cover

JF002 commented 4 months ago

Here are my measurements! For each branch/ref, I

Test case Ref Result
InfiniTime 1.14 ecf2f56 ON : 7.48mA - OFF : 397µA
Test case 1 main: 6ab512a 10.57mA
Test case 1 aod: f84b619 10.58mA
Test case 2 aod tip ON : 7.42mA - AOD : 3.45mA - OFF : 398µA
Test case 3 aod tip Digital : 3.45mA - Analog : 3.84mA - PTS : 3.45mA - Terminal : 3.49mA - Infineat : 3.45mA - G7110 : 3.45mA
Test case 4 4028304 ON : 7.43mA - OFF : 398µA
Test case 4 ac19f99 ON : 7.00mA - OFF : 398µA
Test case 5 0dd662a ON : 7.51mA - AOD : 3.89mA
Test case 5 81d25d2 ON : 7.49mA - AOD : 3.5mA
Test case 6 ON : 7.14mA - AOD : 7.11mA
Test case 7 ON : 7.30mA - AOD : 6.27mA
Test case 8 fd92ff3 Digital ON : 7.38mA - Digital OFF : 3.64mA - Analog ON : 7.80mA - Analog OFF : 4.05mA

Test case 6 and 7 show strange behaviors (and I double checked them) : AOD power usage is higher than when the display is simply ON.

Let me know if you want me to do more measurements :)


On another note, I took the time to have a look at the changes from this branch. I think it would really help my review (and the reviews from other Core developers and contributors) if some of those changes were applied in dedicated PR, especially those that touch the low level drivers (SPI, LCD) and that are not directly linked to this PR (DateTime component, Weather component).

I know that splitting into multiple PR will require a bit of work, but this will allow the reviews to focus on smaller and simpler topics.

Thanks!

mark9064 commented 4 months ago

Thank you so much for all of the measurements! The data looks great

Case 6 particularly is very odd (7 is probably strange for the same reason). I'll need to design some follow up tests. Not sure when I'll be able to get some test cases going but hopefully by the weekend.

The other measurements are very interesting though

Overall it looks like the biggest power savings are related to refresh rate. But to say much more the backlight by itself definitely needs to be tested. Having full colours while in always on seems plausible though power wise provided we can manage the refresh another way

RE splitting into multiple PRs: we already discussed this in the pinetime-dev channel but for anyone else following along here I put forward some ideas in this comment https://github.com/InfiniTimeOrg/InfiniTime/pull/1869#issuecomment-1949583369

mark9064 commented 4 months ago

Ended up being simpler than I anticipated

Case #9
- Goal: test backlight power usage (always on spi/flash context)
- Ref: aod-power-case9: 7ad0bab, 0e256dc, 61405ec, 80812f8
- Conditions: AOD on
- Test
    - Allow device to enter AOD
    - Screen should go blank (may glow from backlight being on in some tests)
    - Record for 10s
JF002 commented 3 months ago

Here are the result for test case #9 (sorry again for the delay!)

Test case Ref Result
9 7ad0bab ON : 7.48mA - OFF (AOD) : 403µA
9 0e265dc1 ON : 7.47mA - OFF (AOD) : 970µA
9 61405ec ON : 7.37mA - OFF (AOD) : 1.69mA
9 80812f8 ON : 6.71mA - OFF (AOD) : 2.25mA

Feel free to split this PR into smaller ones (like you mentioned in https://github.com/InfiniTimeOrg/InfiniTime/pull/1869#issuecomment-1949583369) so that we can we review each parts of the code impacted by this PR :)

mark9064 commented 3 months ago

Thanks for the extra test! The last one looks a bit strange, there are no changes to ON behaviour so the 6.71 is unexpected - not sure what happened there? If that's reproducible, it would be a very interesting result

Aside from that, the numbers are very useful:

It looks like the PPI backlight has more overhead than expected :(

I will think about what tests could be useful here. I suspect efficiency at lower duties is terrible, and 99% duty is probably pretty good. If 50% efficiency is the best we can get it's not the end of the world (doing better than PWM I think, would need to check the datasheet though), but it would be nice to do better. According to the datasheet overhead should be low (I expect 80+% to be possible)

I'll try get the PRs up soon, life is so busy right now!

mark9064 commented 3 months ago

I've started splitting the PRs now, only had time to do one today but I'll get around to the others when I'm free

mark9064 commented 3 months ago

OK, the four sub-PRs are now split out. It looks like I need to make PRs for InfiniSim for 3 of them as well, not sure how long this will take as I've never worked on it before. But the PRs themselves here are all ready for review :)