Emulationstation 2.9.1rp / 2.10.0rp-dev Event Scripting Incomplete - Issues

Lolonois

While jrassa (github alias) did the heavy lifting with bootstrapping the scripting support for ES, his currently accepted PR 551 has some flaws:

screensaver-stop and screensaver-start events are not implemented as a Github user notes
In ES 2.9.1RP and 2.10.0RP-DEV the sleep mode is entered immediately after the screensaver timeout, when the screensaver is either "dim" or "black".
In ES 2.9.1RP and 2.10.0RP-DEV the sleep mode is never entered if the screensaver is either "slideshow" or "video".
Last but not least, there is no option in the UI to set the timeout for the sleep mode (usually larger than the screensaver timeout)

On the other hand: My PR 560 which fixes all four issues is sitting at github for more than a year!

I have no issue with having strict reviews on my PR but I also expect this on the PR of other users, which may have had prevented the first three flaws above.

Epilog: Since we all want to get better I provide the code in question which is responsible for the noted behaviour. See explaination below.

   259          unsigned int screensaverTime = (unsigned int)Settings::getInstance()->getInt("ScreenSaverTime");
   260          if(mTimeSinceLastInput >= screensaverTime && screensaverTime != 0)
   261                  startScreenSaver();
   262  
   263          // Always call the screensaver render function regardless of whether the screensaver is active
   264          // or not because it may perform a fade on transition
   265          renderScreenSaver();
   266  
   267          if(!mRenderScreenSaver && mInfoPopup)
   268          {
   269                  mInfoPopup->render(transform);
   270          }
   271  
   272          if(mTimeSinceLastInput >= screensaverTime && screensaverTime != 0)
   273          {
   274                  if (!isProcessing() && mAllowSleep && (!mScreenSaver || mScreenSaver->allowSleep()))
   275                  {
   276                          // go to sleep
   277                          if (mSleeping == false) {
   278                                  mSleeping = true;
   279                                  onSleep();
   280                          }
   281                  }
   282          }

Line 260: Test screensaver timeout and start screensaver if timeout reached, so far so good.
Line 272: Test screensaver timeout (again!) and check if sleeping is allowed: mScreenSaver is never NULL (see SystemScreenSaver::SystemScreenSaver()) and mScreenSaver->allowSleep() returns false if either "video" or "slideshow" is set, in all other cases true (e.g. "dim", "black") - see SystemScreenSaver.cpp.

HTH

PS: Looping in @pjft and @BuZz to get this addressed.

jdrassa

@Lolonois

I have to say that it is a bit disingenious to state that my PR has flaws because it was missing the functionality that your PR is looking to add.

screensaver-stop and screensaver-start events are not implemented as a Github user notes

The screensaver events were never intended to be a part of that PR. The plan all along was for you to build that functionality on top of my PR. The note you linked to on github referenced a draft of the Scripting documentation that you had edited to add the screensaver events.

screensaver-stop and screensaver-start events are not implemented as a Github user notes

In ES 2.9.1RP and 2.10.0RP-DEV the sleep mode is entered immediately after the screensaver timeout, when the screensaver is either "dim" or "black".

In ES 2.9.1RP and 2.10.0RP-DEV the sleep mode is never entered if the screensaver is either "slideshow" or "video".

Last but not least, there is no option in the UI to set the timeout for the sleep mode (usually larger than the screensaver timeout)

The functionality you describe here has nothing to do with my Scripting PR and is the intended behavior that was implemented as part of the Power Saver PR. You have proposed a change to allow more control over this behavior, but that doesn't mean the existing functionality is flawed.

Now, lets talk about why your PR wasn't merged. I think the overall answer is that it got lost in the shuffle, but I would like to elaborate on it from my personal perspective. You initially submitted a different PR that you later closed in favor of the current PR. On your initial PR, I had expressed some concern over a change you had made to the execution of the scripts that I felt warranted discussion. This was never addressed on the original PR or the current PR. Honestly, after experiencing a lot of friction previously with grid view PR, I just didn't have the will to press the issue and figured I would leave it to others and see what they think. From there, the only additional feedback was to cleanup the whitespace. By the time that was done I think it had just fallen off of everyones radar.

All that being said, I will find the time to give it another look and I'm sure we can get it merged soon.

Lolonois

I had to rock the boat mildly to get some attention to this.

By no means I wanted to disrespect you (@jdrassa), or your work and energy invested, I remember the discussion that
the PRs should be applied as pair.

What I was aiming at that without the second part your first part is not fully functional and most and foremost why it
took so long to review: I understand that without automated tests one is bound to manual tests to verify every path my
PR is introducing and thanks for your hint that my PR just got forgotten (which can easily happen since this is for all
of us a spare time project).

I just got nervous since some guy from the original mk_arcade_joystick_rpi module noted in a comment of an issue, that
the project is moved to gitlab, after at least two years of doing so and while issues and PRs are piling up at the
unknowingly abandoned github project. You can imagine that with this "communication" I won't contribute to recalbox
ever more. Luckily mitu forked this project and integrated it into Retropie. :)

Long story short: I wanted to avoid that our effort ends up in a half baked solution and to accept my part to make it complete
and last but not least to provide value to the Retropie users, as it has been verified independently to be working.

Cheers

Lolonois

Thanks all for merging my PR. :-)

@hex as suggested on GH I 'tag' you, I learned you initiated PR #172.
Maybe you can share your test results of the Power Saving on Emulationstation v2.9.2+ in a new thread?

Hex

@Lolonois The PR has some users adding their metrics for the PS feature. Since then there have been more Pis released and every Pi that's more powerful than the previous one will use less cpu % while ideling.

The main concept of is is that is allows switching from a vsynced rendering to an event driven rendering. It makes no sense to keep rendering the same screen if no changes are present. Thus saving cpu cycles and increasing the battery life. on PiZero there are significant savings (40-60% based on theme). I implemented this feature cos my handheld build was overheating while doing nothing but ES. Animations are not a priority on a battery life focused build and Instant PS mode almost doubled my standby time. ie time spend browsing games on ES.

Please refer to the PR and https://retropie.org.uk/forum/topic/11304/testers-needed-power-saver-features-pr-172 where we have had extensive discussions and testing for the same

RussellB

@hex I know this thread is a bit old but I want to see if there is interest.

I've added three new events to support integration with the Pixelcade LCD Marquee:

game-select
system-select
screensaver-select

How would I go about incorporating these into the main branch?

RussellB