RetroFlag / retroflag-picase

RetroFlag Pi-Case Safe Shutdown
MIT License
711 stars 227 forks source link

Safe Shutdown not working if emulationstation is not running #29

Open arelas opened 5 years ago

arelas commented 5 years ago

Safe shutdown works fine when inside emulation station, but if you are at the terminal, it says:

rc.local[469] emulationstation: no process found

then the script just hangs. I'll attempt to bypass this to see if it doesn't detect emulationstation, it just moves on.

crcerror commented 5 years ago

caused by && If killall fails everything else fails

felleg commented 5 years ago

Workaround before the official fix: sudo nano /opt/RetroFlag/SafeShutdown.py Change line 19 to: os.system("sudo shutdown -h now") Change line 23 to: os.system("sudo reboot")

Note: These changes will be overwritten if you re-run istall.sh and the SafeShutdown.py file is not already present in the retroflag-picase folder (the one from where you run the install code).

crcerror commented 5 years ago

@felleg Not a good idea, this will break safe shutdown.

Better use this: https://github.com/crcerror/retroflag-picase

felleg commented 5 years ago

@crcerror Oh, I assumed that doing sudo killall emulationstation was not "safe" to begin with. Do you suggest that the metadata information is safe when stopping ES with killall? In that case, then I agree, the script you linked is the better method.

Tolaris commented 5 years ago

There is a much simpler fix: https://github.com/RetroFlag/retroflag-picase/pull/36

Short version: in the os.system() calls in SafeShutdown.py, replace "&&" with ";"

keilmillerjr commented 5 years ago

sudo shutdown -h The halt should shutdown all running processes. I don’t understand why emulation station needs to be killed? I am using raspbian lite and removed the emulation station bit with no negative effects regardless if I have terminal, attractmode, or mame running. But I am no pro.

Tolaris commented 5 years ago

@keilmillerjr That should work fine! My suggestion was "simpler" because it only changed a few characters from upstream, as compared to @crcerror 's fix which involved whole new scripts.

I suspect that either Retroflag doesn't know how shutdown works (kill and reap all processes), or they just wanted to be sure that emulationstation shut down before any timeouts. I doubt it matters and I'll be playing with this too.

Note that "shutdown -h" without an argument defaults to "shutdown -h +1", which waits 1 minute to fully halt. Why not use "now"?

Stephenm64 commented 5 years ago

For people watching this conversation. One thing to note is that waiting for emulation station to fully exit and waiting for a period of time and/or issuing a sync command is best practice.

You can be effected by a long standing bug where gamelist.xml files may become corrupt or not save changes if a direct shutdown is issued.

On Mon, Nov 26, 2018, 2:37 AM Tyler J. Wagner <notifications@github.com wrote:

@keilmillerjr https://github.com/keilmillerjr That should work fine! My suggestion was "simpler" because it only changed a few characters from upstream, as compared to @crcerror https://github.com/crcerror 's fix which involved whole new scripts.

I suspect that either Retroflag doesn't know how shutdown works (kill and reap all processes), or they just wanted to be sure that emulationstation shut down before any timeouts. I doubt it matters and I'll be playing with this too.

Note that "shutdown -h" without an argument defaults to "shutdown -h +1", which waits 1 minute to fully halt. Why not use "now"?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/RetroFlag/retroflag-picase/issues/29#issuecomment-441593339, or mute the thread https://github.com/notifications/unsubscribe-auth/AAjUnf4ppHg4AB7CH7NyXPu1o7cZMla0ks5uy8RegaJpZM4WkDYL .

Tolaris commented 5 years ago

Yikes, thanks for that warning, Stephen! I'll try "sleep 1; sync;" instead.

keilmillerjr commented 5 years ago

@keilmillerjr That should work fine! My suggestion was "simpler" because it only changed a few characters from upstream, as compared to @crcerror 's fix which involved whole new scripts.

I suspect that either Retroflag doesn't know how shutdown works (kill and reap all processes), or they just wanted to be sure that emulationstation shut down before any timeouts. I doubt it matters and I'll be playing with this too.

Note that "shutdown -h" without an argument defaults to "shutdown -h +1", which waits 1 minute to fully halt. Why not use "now"?

I use the now parameter. I left it out in my original comment because I felt it wasn’t pertinent to the conversation.

crcerror commented 5 years ago

@keilmillerjr @Tolaris The invoking of scripts is needed to make the reset button behaviour work. You can terminate the current running emulator by pressing the reset button. This is a better behaviour than just to reboot the system imho. Moreover this is a proof of concept how python can handle shell outputs - I was a bit overwhelmed of the positive user feedback.

There is nothing against using shell scripts it does not steal hardware resources. It's just a one shot call by python interrupt.

Tolaris commented 5 years ago

@crcerror OK, that's a pretty fun script.

I had planned to implement something like your approach in pure python, mainly due to all the input validation challenges bash introduces. But you've already got some fun features (like restarting emulationstation in the "no emulator running" case), and working code wins. Thanks for writing this!

crcerror commented 5 years ago

@Tolaris You're welcome