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

Improve sleep time #648

Merged
merged 1 commit into from
May 8, 2022
Merged

Improve sleep time #648

merged 1 commit into from
May 8, 2022

Conversation

Riksu9000
Copy link
Contributor

@Riksu9000 Riksu9000 commented Sep 6, 2021

DisplayOn() and DisplayOff() are direct lcd commands, it's better to use only Sleep() and Wakeup().

The lcd is already controlled in SystemTask, so these aren't necessary in DisplayApp. SleepIn() puts the lcd into the lowest power mode so DisplayOff() doesn't do anything as far as I can tell.

EDIT: lcd can't be removed from DisplayApp, because DisplayAppRecovery needs it, and DisplayAppRecovery also has a lot of unused parameters. I'll look at this separately.

This eliminates the mysterious 500ms delay that exists in DisplayOff(). https://github.com/JF002/InfiniTime/blob/6f9f0e8b0e42a5526d47ca664534fb6b0ccb6ace/src/drivers/St7789.cpp#L119

@geekbozu
Copy link
Member

geekbozu commented Sep 9, 2021

I like these changes they definitely feel more appropriate.
Do we know why this was included to begin with or are we just assuming it was a previous choice that is now just Tech-Debt?

@@ -44,8 +44,7 @@ namespace Pinetime {
enum class States { Idle, Running };
enum class FullRefreshDirections { None, Up, Down, Left, Right, LeftAnim, RightAnim };

DisplayApp(Drivers::St7789& lcd,
Components::LittleVgl& lvgl,
DisplayApp(Components::LittleVgl& lvgl,

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lcd is actually used in DisplayAppRecovery, but it looks like DisplayApp and DisplayAppRecovery must use all the same parameters even if they aren't needed. I would like to improve this as well, but for now I'll just put lcd back in DisplayApp even though it isn't used, just to keep this PR simple.

@Riksu9000 Riksu9000 changed the title Remove lcd from DisplayApp/Improve sleep time Improve sleep time Sep 20, 2021
@hubmartin
Copy link
Contributor

Work fine. I've tested it for a few minutes together with my lcd wakeup optimizations and yours better button handler. I did not noticed any issues sleeping or when pressing button many times.

@lman0
Copy link

lman0 commented Oct 29, 2021

any news on reviewing this @geekbozu ?

@geekbozu
Copy link
Member

I want to do a comparison on battery draw given these changes still. Life has been busy, PR is in my todo list to review/data log :)

FintasticMan added a commit to FintasticMan/InfiniTime that referenced this pull request Dec 13, 2021
Squashed commit of the following:

commit d213ae7
Author: Riku Isokoski <[email protected]>
Date:   Mon Sep 20 11:38:33 2021 +0300

    Fix DisplayAppRecovery

commit de7cc1f
Merge: a825fcb 5855906
Author: Riku Isokoski <[email protected]>
Date:   Sat Sep 18 19:19:03 2021 +0300

    Merge branch 'develop' into del_dispapp_lcd

commit a825fcb
Author: Riku Isokoski <[email protected]>
Date:   Mon Sep 6 11:59:54 2021 +0300

    Remove lcd from DisplayApp
@Riksu9000 Riksu9000 requested a review from JF002 March 23, 2022 17:59
@Avamander Avamander added this to the 1.10.0 milestone Apr 18, 2022
@JF002
Copy link
Collaborator

JF002 commented Apr 24, 2022

The datasheet does not provide much details about DISPLAY ON and OFF modes, unfortunately, except that the display is in DISPLAY OFF mode after reset/sleep.
So the command DISPLAY ON is probably needed at startup and after sleep, but I don't know if we really need to send the command DISPLAY OFF before sleep.

IIRC, I wrote the display driver with the help of other community member at that time, and maybe by reading the sources of other similar driver found online. So I might have copied that displayon/off command just to be sure the display will work properly.

If there's no visible side effect of that command, let's remove it and spare 500ms each time the display goes to sleep :)

@JF002 JF002 merged commit 6dac0a6 into InfiniTimeOrg:develop May 8, 2022
@Riksu9000 Riksu9000 deleted the del_dispapp_lcd branch May 8, 2022 11:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants