GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Always on display by KaffeinatedKat · Pull Request #1869 · InfiniTimeOrg/InfiniTime

Conversation

@KaffeinatedKat

@KaffeinatedKat KaffeinatedKat commented Sep 30, 2023

Copy link
Copy Markdown
Contributor

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

playlogo, hexisXz, LinuxinaBit, jl-23929, and headblockhead reacted with heart emoji
All reactions
  • ❤️ 5 reactions
@github-actions

github-actions Bot commented Sep 30, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Build size and comparison to main:

Section Size Difference
text 378724B 1516B
data 948B 8B
bss 63504B -52B
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JF002

JF002 commented Sep 30, 2023

Copy link
Copy Markdown
Collaborator

This will probably reduce the battery life to just a few hours. The LCD uses between 10 and 30mA when it's ON, which is quite high compared to the current power usage of InfiniTime (less than 1mA).

I can't think of any way to reduce power usage of the display when it's on... Lowering the brightness will effectively reduce the power usage, but probably not enough to maintain a reasonable battery life. This is caused by the LCD technology of the display.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KaffeinatedKat

KaffeinatedKat commented Sep 30, 2023

Copy link
Copy Markdown
Contributor Author

is there anyway to reduce the brightness lower than the lowest setting right now? Currently on the lowest brightness setting it's gone ~15 hours and it's at ~35%, which was better than I was expecting

Saarsk and egormanga reacted with thumbs up emoji
All reactions
  • 👍 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JF002

JF002 commented Sep 30, 2023

Copy link
Copy Markdown
Collaborator

You could drive the brightness using PWM. There might already be a PR for that.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064

mark9064 commented Oct 1, 2023

Copy link
Copy Markdown
Member

You could drive the brightness using PWM. There might already be a PR for that.

Indeed there is, see #575

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KaffeinatedKat

KaffeinatedKat commented Oct 1, 2023

Copy link
Copy Markdown
Contributor Author

I've changed it so that the "always on" setting prevents the display from turning off, but the watch still goes into the sleep mode, and just sets it to a low brightness with pwm. Now the display will light up when any of the wakeup actions happen, then go dim again after the display timeout.

My only issue is the watch is in sleep mode and the watchface does not update while sleeping. Because the display is still on the watchface is just frozen till woken up.
I've been poking around the code and I cannot find where I can make the display update while sleeping

A video of the watchface not updating while asleep:
https://drive.google.com/file/d/1pNt-cbVS1s_8tmAK6B8ZmDR_WU164OI8/view

Saarsk reacted with thumbs up emoji LinuxinaBit and Saarsk reacted with heart emoji
All reactions
  • 👍 1 reaction
  • ❤️ 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064

mark9064 commented Oct 1, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

A common pattern you'll spot all across InfiniTime is using queue read timeout to implement periodic tasks. Many tasks have an event queue that they read from forever, and when fetching an event from a queue you can specify a timeout, which is how long to wait if nothing arrives. After this timeout, the queue fetch function returns that no items were fetched from the queue as nothing was available in the time specified. So if you have a loop which does:

  1. get event with timeout 1 minute
  2. process event if one exists
  3. execute x task

then x task runs at least every minute.

This is exactly how it's implemented for the display, with x task in this case being refreshing display contents. So change

    case States::Idle:
      queueTimeout = portMAX_DELAY;
      break;

to

    case States::Idle:
      if (settingsController.GetAlwaysOnDisplay()) {
        queueTimeout = lv_task_handler(); // returns time until LVGL tasks need running
      } else {
        queueTimeout = portMAX_DELAY;
      }
      break;

and you're good :)

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064

mark9064 commented Oct 1, 2023

Copy link
Copy Markdown
Member

Seems to work well with the above change. I haven't been running it long enough to test battery yet though. One suggestion would be to disable always on if InfiniTime is set to sleep mode

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KaffeinatedKat

KaffeinatedKat commented Oct 2, 2023

Copy link
Copy Markdown
Contributor Author

Seems to work well with the above change. I haven't been running it long enough to test battery yet though. One suggestion would be to disable always on if InfiniTime is set to sleep mode

This is something I plan to implement, this feature would pair really nicely with #1461

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KaffeinatedKat

KaffeinatedKat commented Oct 2, 2023

Copy link
Copy Markdown
Contributor Author

A common pattern you'll spot all across InfiniTime is using queue read timeout to implement periodic tasks. Many tasks have an event queue that they read from forever, and when fetching an event from a queue you can specify a timeout, which is how long to wait if nothing arrives. After this timeout, the queue fetch function returns that no items were fetched from the queue as nothing was available in the time specified. So if you have a loop which does:

1. get event with timeout 1 minute

2. process event if one exists

3. execute x task

then x task runs at least every minute.

This is exactly how it's implemented for the display, with x task in this case being refreshing display contents. So change

    case States::Idle:
      queueTimeout = portMAX_DELAY;
      break;

to

    case States::Idle:
      if (settingsController.GetAlwaysOnDisplay()) {
        queueTimeout = lv_task_handler(); // returns time until LVGL tasks need running
      } else {
        queueTimeout = portMAX_DELAY;
      }
      break;

and you're good :)

I have applied this, and it doesn't work. Screen still doesn't update while sleeping and when I wake it up the colors invert, which is extremely weird. Any idea why?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KaffeinatedKat

KaffeinatedKat commented Oct 3, 2023

Copy link
Copy Markdown
Contributor Author

I have applied this, and it doesn't work. Screen still doesn't update while sleeping and when I wake it up the colors invert, which is extremely weird. Any idea why?

I have solved this issue, this change worked fine in the simulator but did not work on the watch. Figured out that by not putting the SPI to sleep, these changes work as intended

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064

mark9064 commented Oct 3, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Nice job, I'm running a patchset that disables SPI sleep and I suspected that might be difference - unfortunately I didn't have time to test and report back so sorry about that

One change I'd suggest: running the LVGL tasks full speed (ie max display refresh) is pretty power hungry, and I've found throttling it to 250ms saves power while keeping watchfaces with seconds up to date (as the display effectively refreshes at 4Hz). I'm getting ~30h battery life running the screen on constantly right now (This is a calculated number as I turn it off at night)
I also thought for correctness it might be good to pull up the screen running check. It also might be good not to reinitialise the SPI inside GoToRunning if it wasn't actually suspended

So I've been running this - feel free to integrate changes if you fancy

      if (settingsController.GetAlwaysOnDisplay()) {
        if (!currentScreen->IsRunning()) {
          LoadPreviousScreen();
        }
        int lvglWaitTime = lv_task_handler();
        // while in always on mode, throttle LVGL events to 4Hz
        queueTimeout = std::max(lvglWaitTime, 250);
      } else {
        queueTimeout = portMAX_DELAY;
      }
      break;

I've also noticed an issue where sometimes the brightness is on Low rather than Lowest while the watch is asleep - I haven't had time to debug that either but see if you spot it. For reference I have my normal brightness set to Low

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

KaffeinatedKat
KaffeinatedKat commented Oct 4, 2023
View reviewed changes
Comment thread src/components/settings/Settings.h Outdated
} else {
settings.alwaysOnDisplay.state = true;
}
}

@KaffeinatedKat KaffeinatedKat Oct 4, 2023

Copy link
Copy Markdown
Contributor Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

i'm unsure if this code belongs here, or if we should check the notification status before sleeping instead

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@escoand

escoand commented Oct 4, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown

Wouldn't be an update every minute enough for an always on display? It should increase the battery runtime further.

Off course with hiding the seconds hand...

egormanga reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064

mark9064 commented Oct 4, 2023

Copy link
Copy Markdown
Member

Wouldn't be an update every minute enough for an always on display? It should increase the battery runtime further.

Off course with hiding the seconds hand...

Yeah it's definitely something worth thinking about. On one hand it saves power, but on the other, it allows incorrect information to be onscreen for more than a minute like seconds on watchfaces with them, seconds on the timer app, bluetooth status, music status, steps, heart rate (if continuous/background measuring implemented) etc
Whether these are important enough for the power saving, I'm not sure. Maybe it's worth testing to see how big the power difference is?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KaffeinatedKat

KaffeinatedKat commented Oct 4, 2023

Copy link
Copy Markdown
Contributor Author

Wouldn't be an update every minute enough for an always on display? It should increase the battery runtime further.
Off course with hiding the seconds hand...

Yeah it's definitely something worth thinking about. On one hand it saves power, but on the other, it allows incorrect information to be onscreen for more than a minute like seconds on watchfaces with them, seconds on the timer app, bluetooth status, music status, steps, heart rate (if continuous/background measuring implemented) etc Whether these are important enough for the power saving, I'm not sure. Maybe it's worth testing to see how big the power difference is?

This might be worth it, we can set the update timer to 4Hz when apps like the timer and music are running, but set it back when not running these apps; perhaps an enum with apps that need a higher refresh timer. We can also disable the seconds on every watchface that has them when always on mode. We could also add a setting to disable this if someone wants seconds on. I think with the display the pinetime has, any power savings we can get is worth it

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@hexisXz

hexisXz commented Oct 12, 2023

Copy link
Copy Markdown

This will probably reduce the battery life to just a few hours. The LCD uses between 10 and 30mA when it's ON, which is quite high compared to the current power usage of InfiniTime (less than 1mA).

I can't think of any way to reduce power usage of the display when it's on... Lowering the brightness will effectively reduce the power usage, but probably not enough to maintain a reasonable battery life. This is caused by the LCD technology of the display.

I honestly think this would be pretty useful. You could just have a low quality version of the digital clock display on the screen with like medium brightness and just have everything else suspended in the background.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KaffeinatedKat

KaffeinatedKat commented Oct 14, 2023

Copy link
Copy Markdown
Contributor Author

This pr, for the most part, is complete. Only remaining issue is sometimes the display does not go into the lowest setting when going to sleep, and stays at the brightness setting. I have not been able to figure this out because it happens so infrequently.

@JF002, is there any way this feature would be considered for merging, or is the battery life reduction while enabled to high? Maybe there could be some kind of popup warning when enabled, to notify the user of the significant battery drain while enabled.

It lasts about a full day when enabled, and most smartwatches only last that long anyways. I feel like the people who genuinely find this useful would be willing to charge it everyday

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064

mark9064 commented Oct 14, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

The issues with the screen getting stuck in Low are due to the restorebrightness messages coming from the system task. (Edit: I can post some actual patches soon)

I think this PR (with the above issue fixed) is usable. But we should also test what the majority of the power consumption is when always on (I suspect it's the LCD) and if we can reduce it further (the LCD offers a power mode between sleep (display blank) and fully on with reduced colours etc.). Also might be possible to redrive the LCD at a lower refresh rate when in always on, but the datasheet for it on the Pine wiki is an absolute monster and I haven't had the time to fully explore it. As an aside it looks like the solution to the display invert issue might be in there too

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KaffeinatedKat

KaffeinatedKat commented Oct 14, 2023

Copy link
Copy Markdown
Contributor Author

I took a peek at the datasheet, and implementing switching to reduced colors while always on. The datasheet says this uses less power, but by how much I am unsure. I don't have a devkit, and can't measure power consumption.

This is also essentially free power reduction. Turns out, InfiniTime doesn't utility many colors at all, and while in reduced colors, you can hardly tell. The reduced brightness also helps hide it. The one app where you can really tell is 2048, and I doubt anybody will be letting it fall asleep on 2048 anyways.

I'm gunna keep poking around the datasheet to see if I can figure out the refresh rate thing, if that is possible it would probably help the power consumption by a sizeable margin

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064

mark9064 commented Oct 14, 2023

Copy link
Copy Markdown
Member

Oh yeah also forgot to mention, the settings version number should be bumped as a new member has been added

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064

mark9064 commented Oct 14, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

#1869 (comment)

The patch in question: mark9064@d43759f
Feel free to pull the commit into your branch if you're happy with it :)

Locally working well - to reproduce the issue on the original try disconnecting and reconnecting BT (the display should get stuck on after). With the patch it should stay in always on as expected

Reduced colours mode seems to be working, only just flashed it so haven't had time to see if there are power changes (I only have sealed). Some of the watchfaces suffer though

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KaffeinatedKat

KaffeinatedKat commented Oct 14, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Applied your patch, and it has fixed the issue on my device as well. Bumped the settings version, while I was at it

Also figured out how to modify the refresh rate, it now goes to ~4.8hz while always on, which is the lowest setting available. Hopefully this helps increase the battery life

If anyone with a devkit wants to test if the reduced colors actually saves a realistic amount of power that would be great. I feel like it would be worth reverting if it doesn't save enough power due to making some watchfaces look worse

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064

mark9064 commented Oct 14, 2023

Copy link
Copy Markdown
Member

+1 on the reduced colours thought

Can confirm refresh rate changes have applied properly, well done on figuring that out! I think it should actually be possible to inform LVGL of the exact refresh rate too and that way we shouldn't have to do the timer throttling hack? I'm not totally sure on this, I'd need to dig through the LVGL internals but it might be worth considering

KaffeinatedKat reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064

mark9064 commented Oct 14, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

I had a quick look: it seems that LV_DISP_DEF_REFR_PERIOD is used as a define to set the refresh period on all the LVGL tasks to 20ms. So I think what we'd have to do is go through all of these tasks (there are many inside InfiniTime and a couple in LVGL itself), and then use lv_task_set_period to set them to a longer execution period. Quite a bit of work, but looks possible

Edit: Damn this is looking pretty complicated. Some of the tasks should be suspended altogether (like the input reading task) while some should just be slowed. Not sure how feasible doing this is

Edit 2: The more I think about it, the less this seems like a good idea. Trying to bring all of the LGVL tasks together to change the intervals sounds like a bit of a nightmare organisation wise. Perhaps it would be better to properly understand exactly what lv_task_handler does and see if we should implement our own task handler that runs tasks with efficiency in mind rather than latency (ie batching task execution). What are your thoughts?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KaffeinatedKat

KaffeinatedKat commented Oct 15, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

Perhaps it would be better to properly understand exactly what lv_task_handler does and see if we should implement our own task handler that runs tasks with efficiency in mind rather than latency (ie batching task execution). What are your thoughts?

I think that this is a good idea, would this new task handler replace lv_task_handler all together, or only be used for the always on display?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064

mark9064 commented Oct 15, 2023 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Probably just for always on as I suppose we still want responsive inputs etc while on. But I guess it depends on how it's implemented? It might even not be needed at all - I'm not sure how the LVGL scheduler works

A quick list of things I've noticed so far:

  • Screen tearing is quite visible. Try with the analogue face (or timer app), notice how the second hand gets chopped in half occasionally. Also, the lcd pulses with the refresh rate (pretty tricky to spot but try looking at the top right). Maybe the LCD is being driven too slowly? I'm honestly no expert on what refresh rates the panel supports. I think I might try keeping 60Hz but using /8 divider on that and seeing if it does better - will report back. There also might be some VSYNC stuff we can do with the panel to fix this
  • Frame pacing is noticeably poor (visible with seconds) but honestly I think that's fine as a sacrifice if it gets battery life back
  • There are still some wake/sleep state bugs I need to investigate (display gets stuck on in some scenarios)
  • A background transition to the Sleep mode (eg with the auto sleep PR) causes everything to break

Battery life is looking promising though! Not sure if it's the reduced refresh or reduced colours but I'm seeing an improvement for sure

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KaffeinatedKat

KaffeinatedKat commented Oct 16, 2023

Copy link
Copy Markdown
Contributor Author

Screen tearing is quite visible

I've noticed the screen tearing, it's really bad if you run it at that refresh rate while awake. I don't personally run a watchface with seconds and so I haven't noticed it much, but it's for sure worth investigating for those who do. I'll do some tinkering with the refresh rate and see about VSYNC

A background transition to the Sleep mode (eg with the auto sleep PR) causes everything to break

I have encountered this, stopping the spi from sleeping altogether fixes the crashing, and then I just call "GoToRunning" to fix the screen state being wrong afterwards (see e6d60f1). We should find a way to fix this bug without the autosleep pr however, i'll keep you updated on that

There are still some wake/sleep state bugs I need to investigate (display gets stuck on in some scenarios)

I have not encountered any of these, if you could provide some scenarios that cause this, I can help debug it

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064

mark9064 commented Oct 16, 2023

Copy link
Copy Markdown
Member

Coming in on 48h runtime now with 20% still in the tank, very impressive! I'd say it's a lot more usable with those changes, awesome job

Another thought I had for power usage is driving the LCD at a slightly lower voltage. From what I can see the LCD driver allows choosing what drive voltage is used for the LCD, so maybe a slightly lower voltage could save some power? It also might cause the LCD to not turn pixels on/off properly so not sure if this is a viable route (and without a power profiler I'm not even sure if the panel draws much power or if it's mostly the driver chip). Also there appears to be about 10 driver voltages 😬 so yeah sounds fun

One other thing: some of the comments from the original PWM PR should probably be addressed. I think the points about not running the PWM engine when the screen is off or at one of the fixed brightness modes has merit, with PWM only being used between brightness settings. I think it also makes sense to PWM just one pin at a time, though I can see the formulas for calculating brightness being annoying as the brightness increase per duty increase is different for each pin
Ideally I think this should take the form of a unified brightness interface where it takes some target brightness value between 0-10000 (or whatever) and the brightness code calculates what pin configuration is needed along with whether PWM is required? I could have a crack at implementing this, what do you reckon?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KaffeinatedKat

KaffeinatedKat commented Oct 16, 2023

Copy link
Copy Markdown
Contributor Author

I can go ahead and mess with the voltage stuff, still messing around with the refresh stuff to reduce screen tearing, might as well mess with the voltage while i'm at it. Really glad to see how much potential there is for greater battery life with the display being on all the time, considering it's just a regular LCD screen

About the PWM stuff, I honestly have no idea how it works, but if you wanna take a crack at implementing the unified brightness interface, I would happy merge that into the branch. Sounds like it would be a really nice addition to improving the PWM implementation.

mark9064 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064
mark9064 force-pushed the aod branch from e3afe07 to c5aac9a Compare June 19, 2024 16:13
@mark9064

mark9064 commented Jun 19, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

I've just rebased to migrate this PR to batched display arguments as well, and to accept the RTC based implementation.

Thanks for the power testing again, you've been brilliant running so many tests over the course of this PR!
300µA takes AOD backlight PWM from 50% to 100% efficiency :)
I think that's as good we'll get

All required PRs here are merged

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064
mark9064 force-pushed the aod branch from c5aac9a to ae0e893 Compare June 19, 2024 20:30
@mark9064

mark9064 commented Jun 19, 2024

Copy link
Copy Markdown
Member

Small rebase mistake (used old version of a comment)

There are no more technical enhancements I can think of - if people want to try this PR out and feedback now is the time :)

The commits should tell a reasonably coherent story of development. We might want to consider breaking out the brightness message refactor (fix brightness getting stuck high / flashlight brightness restore) but I think it's a simple enough change?

Either way, everything here is ready for review

JF002 reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@KaffeinatedKat

KaffeinatedKat commented Jun 19, 2024

Copy link
Copy Markdown
Contributor Author

awesome to see that this pr is finally ready for review

thanks so much to @mark9064, without your help with all the lower level issues this would not have been possible

mark9064 reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JF002

JF002 commented Jun 23, 2024

Copy link
Copy Markdown
Collaborator

Thank you very much for you work @mark9064 !

I flashed this branch on my PineTime so I can test it, and I'm pleasantly surprised by the result : the time is well readable in most situation : indoor and even outside if you live in Belgium right now (it's been raining for numerous weeks...).

The battery lasted for 2 days on a single charge (BLE enabled, wake up on double tap on the display), From June 19th to June 22nd :

Screenshot_20240623_104019_Gadgetbridge
Screenshot_20240623_104031_Gadgetbridge

While it's expected that the always-on display would reduce battery life, 2 full days is a reasonable trade-off

I'll do my best to review this PR ASAP !

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064

mark9064 commented Jun 23, 2024

Copy link
Copy Markdown
Member

And in real use the screen is off ~8h per night, so 3 days is possible (total runtime 48h as tested above - 16h on per day in real usage - 48/16 = 3)

One thing that's come to mind - should the brightness PWM bits belong in a driver rather than the controller?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

JF002
JF002 requested changes Jul 13, 2024
View reviewed changes

@JF002 JF002 left a comment

Copy link
Copy Markdown
Collaborator

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Thanks you for this very high quality work (in this PR and all previous PR that were needed for the always on mode)!

I have the feeling that all the low-level code added in BrightnessController should fit better in a driver class rather than in a Controller class (drivers encapsulates all the low-level interactions with the hardware, controllers add higher-level - applicative logic to those driver).
But since there's no brightness driver in InfiniTime right now, I guess we can do this in a future PR :)

I added a few non-critical comments. Would you like to have a look at them? The most important one consists in clarifying the always-on setting (state, enabled).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Comment thread src/components/settings/Settings.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/components/settings/Settings.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread src/components/brightness/BrightnessController.h Resolved

Uh oh!

There was an error while loading. Please reload this page.

KaffeinatedKat and others added 13 commits July 21, 2024 16:19
@KaffeinatedKat @mark9064
aod: integrate with display timeout
301e68b
@KaffeinatedKat @mark9064
aod: disable while in notification sleep
765f037
@KaffeinatedKat @mark9064
aod: switch to 8 colors when always on
d99651d
@KaffeinatedKat @mark9064
aod: lower refresh rate when always on
1876249
@mark9064
aod: fix brightness getting stuck high
4da3153
@mark9064
aod: fix flashlight brightness restore
6c2fcea
@KaffeinatedKat @mark9064
aod: lower voltage going to the display
259c171
@mark9064
aod: lower lcd voltage
40f98d5
@mark9064
aod: porch control: 2Hz idle + 75Hz on
8051ae8
@mark9064
aod: constant frequency idle frames
504fd05
@mark9064
aod: run LVGL task handler until all work finished
2710237
@mark9064
aod: avoid spinning DisplayApp under high LVGL load
2b1f194
@mark9064
aod: simplify AOD disablement based on notification status
cc1d4fe
@mark9064
mark9064 force-pushed the aod branch from 7f3720e to cc1d4fe Compare July 21, 2024 15:24
@mark9064

mark9064 commented Jul 21, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Cleaned up code style in a couple other places. I think all that's left to resolve now is the private class constants discussion

JF002 reacted with hooray emoji KaffeinatedKat reacted with heart emoji
All reactions
  • 🎉 1 reaction
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@JF002
JF002 merged commit 53dc9da into InfiniTimeOrg:main Aug 5, 2024
@JF002

JF002 commented Aug 5, 2024

Copy link
Copy Markdown
Collaborator

Thank you so much to everyone who contributed to this PR, and especially to @mark9064 who did a wonderful job and who was very patient during this very long review process (sorry about that!) 🥇

minacode, Itai-Nelken, and mark9064 reacted with hooray emoji
All reactions
  • 🎉 3 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@mark9064

mark9064 commented Aug 5, 2024

Copy link
Copy Markdown
Member

A massive thanks to you too for taking the time to test configurations and review/discuss even the most minor details (including for all the supporting PRs!). Also cheers to @KaffeinatedKat for graciously trusting me with contributing directly to this feature :)

JF002 reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

This was referenced Aug 18, 2024
Restore brightness level when closing the Flashlight application #2038
Closed
Fix brightness icon when swiping to close flashlight #1907
Closed
Brightness icon changes to flashlight's brightness when flashlight is closed by swipe #1751
Closed
@FintasticMan FintasticMan added this to the 1.15.0 milestone Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JF002 JF002 JF002 requested changes
@minacode minacode minacode left review comments
@NeroBurner NeroBurner NeroBurner left review comments
@mark9064 mark9064 mark9064 left review comments

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

new feature This thread is about a new feature

Projects

None yet

Milestone

1.15.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

10 participants

@KaffeinatedKat @JF002 @mark9064 @escoand @hexisXz @FintasticMan @SteveAmor @Saarsk @minacode @NeroBurner
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.