PM: Minor refactor to suspend/resume code flow #10426

NiLuJe · 2023-05-12T03:08:32Z

Make sure we only send Suspend/Resume events when we actually suspend/resume. This is done via the Device _beforeSuspend/_afterResume methods, and those were called by the input handlers, not the PM logic; which means they would fire, while the PM logic could actually take a smarter decision and not do what the event just sent implied ;).

(i.e., sleep with a cover -> suspend + actual suspend, OK; but if you then resume with a button -> input assumes resume, but PM will actually suspend again!).

Existing design issue made more apparent by #9448 ;).

Also fixes/generalizes a few corner-cases related to screen_saver_lock handling (e.g., don't allow USBMS during a lock).

And deal with the fallout of the main change to the Kobo frontlight ramp behavior ;).

This change is

NiLuJe · 2023-05-12T03:09:11Z

Draft because lightly tested and heavily sleep deprived ;p.

(I was supposed to test Android stuff tonight, not lose my mind >_<").

EDIT: Oh, and leftover debugging to be removed, too.

NiLuJe

Reviewed 7 of 17 files at r1, 13 of 15 files at r2, 2 of 2 files at r3, 11 of 11 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @NiLuJe)

API With the goal of deduplicating the slight madness involved here, and, most importantly, not calling them when we don't actually suspend or resume...

Makes more sense, ensures it deals with sleepcover events, too (lathough good luck tripping that ;p), and should dedupe a bunch of code.

event

And make sure we don't fire duplicate events (Might be tricky in practice, given that we're following system events, IIRC, and they should already be guarded properly).

And move things around so it's close to setEventHandlers

for us to be late, so, eh.

It effectively delays it until after the full refresh, and that's... a lot. Will have to see how the previous approach behaves on other, non awful devices. (Testing on an H2O right now)

Thankfully, newer, SMP devices can power through the resource contention. (i.e., on a Sage, it will smoothly ramp *during* the refresh. This is robably for a mix of reasons: because it's SMP, more powerful, and also because the display driver doesn't really give us great vsync fences, so we unblock earlier after the flashing refresh).

NiLuJe

Reviewed 6 of 6 files at r5, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @NiLuJe)

NiLuJe · 2023-05-16T18:34:46Z

Oohkay, me and my trusty fridge magnet haven't managed to break this on a few other devices, so this should be ready.

A bit more churn than I would have initially hoped, but the gist of it is to move the emission of Suspend/Resume Events from the input handlers to the actual PM logic, so as not to send spurious events when the logic does something more complex than what the input could entail (this usually involves sleepcover interactions ;)).

Frenzie

Looks okay, but it's quite large so the impact's a bit hard to gauge. If you think it's ready.

NiLuJe · 2023-05-18T20:40:42Z

It shouldn't be any worse, yeah ;o).

(Famous last words ^^).

Fixes a regression introduced in #10426, when suspending during night and resuming during daylight.

* afterResume had *two* different implementations, so the historical one that handled frontlight fixups no longer ran (regression since koreader#10426) * isFrontlightOn was completely broken, for a couple of reasons: * There was no is isFrontlightOnHW implementation, so when it ran, it mostly always thought the frontlight was on, because self.fl_intensity doesn't change on toggle off. * _decideFrontlightState was never called on Kindle, so isFrontlightOnHW was never really called, making isFrontlightOn completely useless. Call it in setIntensityHW's coda, as it ought to be. And properly document that. Generic *was* calling _decideFrontlightState is setIntensity, but *before* actually setting the frontlight, which makes no goddamn sense, so get rid of that, too. * Also fix frontlight toggle notifications (regression since koreader#10305) TL;DR: The PowerD API being a mess strikes again.

* afterResume had *two* different implementations, so the historical one that handled frontlight fixups no longer ran (regression since #10426) * isFrontlightOn was completely broken, for a couple of reasons: * There was no is isFrontlightOnHW implementation, so when it ran, it mostly always thought the frontlight was on, because self.fl_intensity doesn't change on toggle off. * _decideFrontlightState was never called on Kindle, so isFrontlightOnHW was never really called, making isFrontlightOn completely useless. Call it in setIntensityHW's coda, as it ought to be. And properly document that. Generic *was* calling _decideFrontlightState is setIntensity, but *before* actually setting the frontlight, which makes no goddamn sense, so get rid of that, too. * Also fix frontlight toggle notifications (regression since #10305) TL;DR: The PowerD API being a mess strikes again.

If we get the events, it means stuff happened, we can't just only honor it in the most common workflows ;). This effectively reverts a tiny bit of koreader#10426 (I was sort of expecting this to be problematic at the time, and I most likely hadn't tested it).

Fix #11164 and involves a drive-by fix: Kindle: Send Suspend/Resume event regardless of the screen saver state If we get the events, it means stuff happened, we can't just only honor it in the most common workflows ;). This effectively reverts a tiny bit of #10426 (I was sort of expecting this to be problematic at the time, and I most likely hadn't tested it).

NiLuJe mentioned this pull request May 12, 2023

Random "soft freeze" on Kobo Aura One #10357

Open

NiLuJe commented May 13, 2023

View reviewed changes

NiLuJe added 27 commits May 14, 2023 16:44

Good ol' pritnf debugging

114dc42

Start pluggin Device's _afterResume stuff inside PowerD's afterResume

ed1041c

API With the goal of deduplicating the slight madness involved here, and, most importantly, not calling them when we don't actually suspend or resume...

That ought to do it...

38b258e

Move the screen_saver_lock logic inside onPowerEvent

7daa59b

Makes more sense, ensures it deals with sleepcover events, too (lathough good luck tripping that ;p), and should dedupe a bunch of code.

Log the "power while "asleep" in screensaver mode w/ a sleepcover closed

186d8d9

event

Comment reflow

a62fd1d

Port Cervantes

6738b24

Don't suspend on USB events in screen saver lock

8452b28

Cervantes bis

518e02c

Port SDL

00245c1

Port PRSTUX

fcbb3cf

Restore input inhibition on SDL

e78cb33

Port Kindle

0b2a10e

And make sure we don't fire duplicate events (Might be tricky in practice, given that we're following system events, IIRC, and they should already be guarded properly).

Port PB

01e5c9e

Port rM

06bbc91

Oops, leftover from WIP

f863d34

I hate those.

8a2eb5b

Start working on a more generic UImanager ref approach...

86a6cde

There, should do the trick.

a97a5af

And move things around so it's close to setEventHandlers

Do the same for Input

204a287

Port PB

89942ec

Port Kindle

253db5a

Port Android

d6d6fa2

Port PRSTUX

0dcc0ec

Port SDL

e47a7e6

Missed one

e463abb

Log the delay when removing a task

bf2ffdd

Minor comment review

f6d485d

NiLuJe force-pushed the pm_shenanigans branch from 84ed1eb to f6d485d Compare May 14, 2023 14:46

NiLuJe added 7 commits May 14, 2023 17:02

Random experiment

dd6c089

Yeah, didn't really change anything, we're too late, but it makes sense

705b1ec

for us to be late, so, eh.

baguette de fromage

e5aa9ba

I should just go back to bed.

bf2b345

Well, this fixes the resume ramp, but I'm not a massive fan of it

63db318

It effectively delays it until after the full refresh, and that's... a lot. Will have to see how the previous approach behaves on other, non awful devices. (Testing on an H2O right now)

Zap debug

e4466d6

NiLuJe commented May 16, 2023

View reviewed changes

NiLuJe marked this pull request as ready for review May 16, 2023 18:33

NiLuJe requested review from Frenzie, pazos and poire-z as code owners May 16, 2023 18:33

NiLuJe added this to the 2023.05 milestone May 18, 2023

Frenzie approved these changes May 18, 2023

View reviewed changes

NiLuJe merged commit 7e98b9d into koreader:master May 18, 2023

zwim mentioned this pull request Jun 9, 2023

[AutoWarmth] Fix resume and frontlight issue #10557

Merged

Frenzie pushed a commit that referenced this pull request Jun 9, 2023

[plugin] AutoWarmth: Fix resume and frontlight issue (#10557)

1ce1fe1

Fixes a regression introduced in #10426, when suspending during night and resuming during daylight.

NiLuJe mentioned this pull request Nov 25, 2023

Frontlight is enabled on device wakeup #11142

Closed

NiLuJe mentioned this pull request Jul 31, 2024

Quick wake+sleep sequence can disable frontlight #12246

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

PM: Minor refactor to suspend/resume code flow #10426

PM: Minor refactor to suspend/resume code flow #10426

Uh oh!

NiLuJe commented May 12, 2023 •

edited

Loading

Uh oh!

NiLuJe commented May 12, 2023 •

edited

Loading

Uh oh!

NiLuJe left a comment

Uh oh!

NiLuJe left a comment

Uh oh!

NiLuJe commented May 16, 2023 •

edited

Loading

Uh oh!

Frenzie left a comment

Uh oh!

NiLuJe commented May 18, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

PM: Minor refactor to suspend/resume code flow #10426

PM: Minor refactor to suspend/resume code flow #10426

Uh oh!

Conversation

NiLuJe commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NiLuJe commented May 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NiLuJe left a comment

Choose a reason for hiding this comment

Uh oh!

NiLuJe left a comment

Choose a reason for hiding this comment

Uh oh!

NiLuJe commented May 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Frenzie left a comment

Choose a reason for hiding this comment

Uh oh!

NiLuJe commented May 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

NiLuJe commented May 12, 2023 •

edited

Loading

NiLuJe commented May 12, 2023 •

edited

Loading

NiLuJe commented May 16, 2023 •

edited

Loading

NiLuJe commented May 18, 2023 •

edited

Loading