Schedule next standby on AutoStandby plugin init

[seb-maillard]

This PR attempts to fix #12814.

Following this suggestion and that one, I tested the fix vs. standard plugin settings and the plugin appears to work as expected, even with a never-opened-before complex EPUB. To be clear, this is not an attempt to fix the other issues reported for this plugin, c.f. #9493.

Current approach

Based off of this suggestion, schedule the next standby when the plugin is initialized, either:

1. When KOReader starts
   • To prevent the device from instantly going into standby
2. During a view transition
   • The standby scheduled by the input event leading to the transition has been unscheduled in order to avoid going into standby during the transition
   • Therefore, re-schedule the next standby

Since calling AutoStandby:onInputEvent() on init would actually likely result in filtering out the call (because the view transition happens right after pressing the screen), I isolated this section in a separate function which is now also called in AutoStandby:init().

Undesirable side-effect of the current approach

Since the standby previously scheduled has just been unscheduled by AutoStandby:onCloseWidget() and another one is scheduled pretty much right away by AutoStandby:init(), this results in increasing the standby delay because of the multiplier effect.

Important note regarding the current approach

The approach is to schedule the next standby on init because it has been unscheduled in AutoStandby:onCloseWidget(). It seems the original purpose of unscheduling the standby is to "prevent standby during a view transition". However, with the proposed fix, when opening a never-opened-before complex EPUB and/or in case the plugin is set to its aggressive mode (e.g. battery below the Always standby if battery below threshold), AutoStandby:allow() will actually be invoked on init, i.e. during the transition.
I've tested this use-case (e.g. set Always standby if battery below to 100 and open/close a book) with the patch. And it works fine, i.e. the device instantly goes into standby but only after the view transition is fully over.

As far as I understand, it's because the role of the plugin is not to put the device into standby, it's actually to prevent the device from going into standby. Indeed, when forbidden == false (i.e. Allow auto-standby is unchecked), the device always goes into standby instantly after every event. But only after everything this event has triggered has been fully executed.
If I understand correctly, the plugin has been created precisely to prevent the device from going into standby right away, so that some automatic events scheduled in the future (such as auto page turning, frontlight dim, etc.) will be able to be triggered before the device goes into standby.

If the above is correct, a better approach would probably be to simply not unschedule the next standby in AutoStandby:onCloseWidget() anymore, since there's no risk for the device to go into standby during the view transition. This would have the benefit to avoid the undesirable side-effect mentioned in the previous section.
However, to fix the other bug mentioned in #12814 (device goes into standby instantly when KOReader starts), it would still be necessary to invoke AutoStandby:scheduleNextStandby() on init. But only if the plugin is initialized for the first time, e.g. probably inside this block.

I'd be quite interested to further discuss this point.

Disclaimer

Not only this is my first experience with KOReader, its code-base and the PocketBook InkPad platform, this is also my first experience with Lua. I struggled quite a bit with variable visibility and function arguments and I'd really appreciate some extra guidance. In particular:

1. I tried the following signature for the function I created: AutoStandby:scheduleNextStandby(t, config)
   • Indeed, these variables are already available in AutoStandby:onInputEvent() and config is also already available in AutoStandby:init(), so I only would've needed to add os.time() in that case
   • However, I was unable to figure the right syntax to pass those arguments to the function when calling it from both functions
2. Similarly, I couldn't keep the references to self in the code I extracted from AutoStandby:onInputEvent()
   • Apparently, self couldn't be recognized when AutoStandby:scheduleNextStandby() was invoked from AutoStandby:onInputEvent() (e.g. attempt to index local 'self' (a nil value) ), event though it would be fine when invoked from AutoStandby:init()
   • I found it quite weird since self was actually recognized in AutoStandby:onInputEvent()

CC:

• @NiLuJe, @pazos and @Frenzie as you've been previously involved in Enable auto-suspend for PocketBook InkPad 3 #12791
• @rjd22 since you're also working on PB standby issues in the context of Time-dependent functions don't work properly #9493

---

This change is 

[NiLuJe]

InkView doesn't care about whatever we're doing (i.e., if we allow standby [more accurately on PB, if we do not prevent standby], it should be aggressively trying to standby regardless of what we're doing), so I'm mildly surprised this works.

[pazos]

Hi, @sebastien-maillard!

Welcome to lua. It is a very easy and fun language. Quite powerful too!

Please move your AutoStandby:scheduleNextStandby before the AutoStandby:init method. Otherwise you won't be able to call it from there. The same with other functions.

Please consider a simpler name like AutoStandby:_scheduleNext. In lua everything is public, but the prefix _ is commonly used here to label methods that are used by other methods and are not part of the "public" interface.

Then please try again with self references. If you find other issues then please keep the PR updated with your reports :)

[seb-maillard]

Thanks both for the feedback!

InkView doesn't care about whatever we're doing (i.e., if we allow standby [more accurately on PB, if we do not prevent standby], it should be aggressively trying to standby regardless of what we're doing), so I'm mildly surprised this works.

I'm assuming you're referring to my point about not preventing standby during the view transition.

According to my tests, when I completely disable the AutoStandby plugin (i.e. let the OS handle put the device to standby), KOReader works fine when it comes to handling user interactions, e.g. navigation in file browser, menus, books, etc. For instance, when opening a never-opened-before complex EPUB (around 20s of loading time), the device will only go into standby after the book is fully loaded and the transition is over.
So my guess is the SDK will first fully execute all the code triggered by the interaction and only then put the device into standby.

This is actually how I use my device now (AutoStandby plugin disabled). I was initially planning on using the AutoStandby plugin to keep the device awake long enough to dim the light, but since it drains 20x more battery when idle and not in standby, it wasn't worth the trade-off for me.

Welcome to lua. It is a very easy and fun language. Quite powerful too!
Please consider a simpler name like AutoStandby:_scheduleNext. In lua everything is public, but the prefix _ is commonly used here to label methods that are used by other methods and are not part of the "public" interface.

Thanks for the tip and dully noted!

Please move your AutoStandby:scheduleNextStandby before the AutoStandby:init method. Otherwise you won't be able to call it from there. The same with other functions.

To be clear, the code I submitted does work as of right now, i.e. both AutoStandby:init() and AutoStandby:onInputEvent() are actually able to call AutoStandby:scheduleNextStandby(), even though it's declared below these two. In light of what you're saying, that shouldn't be possible so I'm a bit surprised it works as is.

Then please try again with self references. If you find other issues then please keep the PR updated with your reports :)

I followed your instructions, i.e.:

1. renamed the method to AutoStandby:_scheduleNext()
2. moved it above AutoStandby:init()
3. restored the usage of self within the method like it used to be in the original code of AutoStandby:onInputEvent().

However, I still run into the same issue as before, i.e. AutoStandby:_scheduleNext() works fine when called from AutoStandby:init(), but when it's called from AutoStandby:onInputEvent(), I get the following error in the log:

DEBUG AutoStandby:onInputevent() instance= table: 0xa2053960 
DEBUG AutoStandby: input too far in future, resetting adaptive standby delay from 5 to 1 
ERROR failed to call event handler onInputEvent plugins/autostandby.koplugin/main.lua:52: attempt to index local 'self' (a nil value) 

This happens on the following line:

    if not self:isAllowedByConfig() then

Basically, all the methods that could successfully be called on self in AutoStandby:onInputEvent() will trigger this error if called on self in AutoStandby:onInputEvent() when it's called from AutoStandby:onInputEvent().

I have to say I'm still quite confused with the usage of self, since for instance in the original AutoStandby:onInputEvent() method, variables such as settings and lastInput, which are declared in the same block, are not accessed the same way:

• self.settings
• AutoStandby.lastInput

Please note I added a commit for the first two points in this PR. But I pushed the commit for the third point (regarding self) in a separate branch since it now makes the plugin crash.

[NiLuJe]

Keep in mind Device:init calls setAutoStandby(true) on init (i.e., IV is allowed to standby). This will definitely break timers (hence AutoStandby ;)).

Also, the AutoStandby plugin is a special snowflake in that it explicitly only calls class methods (and potentially only as functions to make it clear) & attributes, IIRC, so the self usage (or lack thereof) is fairly odd ;).

[NiLuJe]

Keep in mind Device:init calls setAutoStandby(true) on init (i.e., IV is allowed to standby). This will definitely break timers (hence AutoStandby ;)).

That's potentially a separate issue, in that I would assume disabling AutoStandby leaves the system in a state that forcibly prevents standby, which isn't what's currently happening.

[seb-maillard]

Keep in mind Device:init calls setAutoStandby(true) on init (i.e., IV is allowed to standby).

I wasn't aware of that, thanks for pointing it out!

This will definitely break timers (hence AutoStandby ;)).

100%

Also, the AutoStandby plugin is a special snowflake in that it explicitly only calls class methods (and potentially only as functions to make it clear) & attributes, IIRC, so the self usage (or lack thereof) is fairly odd ;).

Dully noted!

That's potentially a separate issue, in that I would assume disabling AutoStandby leaves the system in a state that forcibly prevents standby, which isn't what's currently happening.

By disabling AutoStandby, I meant disabling the plugin. Not forbidding standby when the plugin is enabled. See the distinction I made in the doc at the end of the Meaning of standby section.

Thanks to this suggestion, I was able to solve both:

• the issue with references to self mentioned here
• the issue with passing t and config as parameters of AutoStandby:_scheduleNext() mentioned in the Disclaimer section of the PR description

See latest commits for reference.

As a side note, I'm still able to call AutoStandby:_scheduleNext() when it's declared below AutoStandby:onInputEvent(). The main advantage is that this dramatically reduces the diff for this PR compared to declaring it above AutoStandby:init(), as illustrated with this other branch.
However, I assume I should refrain from doing that since this seems to be against coding conventions.

[pazos]

Please use self:whateverFunction(), instead of self.whateverFunction() if the function itself need to access self.

Does that fixes the issue?

[Frenzie]

Note that self: whatever() is short for self.whatever(self, etc)

[seb-maillard]

Please use self:whateverFunction(), instead of self.whateverFunction() if the function itself need to access self.
Does that fixes the issue?

Yes, it's solved following this suggestion!

[pazos]

As a side note, I'm still able to call AutoStandby:_scheduleNext() when it's declared below AutoStandby:onInputEvent(). The main advantage is that this dramatically reduces the diff for this PR compared to declaring it above AutoStandby:init(), as illustrated with this other branch.
However, I assume I should refrain from doing that since this seems to be against coding conventions.

Nah. You can ignore my first message alltogether. I was thinking about circular dependencies on function calls or something like that.
That branch looks better.

But I'm struggling figuring out why the self function call doesn't work on init

[seb-maillard]

But I'm struggling figuring out why the self function call doesn't work on init

My bad, I didn't thought of changing this call too. It does work indeed!

[pazos]

Indeed that looks much better. Sorry about the false hints :p

[pazos]

Does _scheduleNext needs some kind of guard?

ie: when the autostandby is disabled we shouldn't schedule standby at all.

[seb-maillard]

Does _scheduleNext needs some kind of guard?

ie: when the autostandby is disabled we shouldn't schedule standby at all.

Are you referring to the case where standby is forbidden (i.e. forbidden == true)? In which case _scheduleNext already handles that case.

Or to the case where the plugin is disabled? In which case you seem to have the same opinion as expressed here. But that would be a separate issue I guess.

To be noted that the current situation allows both forbidding standby and allowing systematic standby, as I highlighted in the wiki. If disabling the plugin would have the same effect as forbidding standby, the option to allow standby without any delay should probably be implemented.

[pazos]

Sorry for the delay!

Are you referring to the case where standby is forbidden (i.e. forbidden == true)? In which case _scheduleNext already handles that case.

Yup, exactly that. Thank you!

[pazos]

That's potentially a separate issue, in that I would assume disabling AutoStandby leaves the system in a state that forcibly prevents standby, which isn't what's currently happening.

Yup. I assumed that too. IMHO it doesn't matter. Now we have A+ tier documentation in https://github.com/koreader/koreader/wiki/Auto-Standby

It should be a matter of link that in the user guide in troubleshooting. It aplies to pocketbook users. Many users won't be affected but those who rely on timers will (ie: autoscroll/page or notification/message timeouts). Those users will want to read the awesome wiki documentation and figure out a setting that work for them.

[NiLuJe]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @benoit-pierre)