kiteretro / Circuit-Sword

GNU General Public License v3.0
134 stars 27 forks source link

Cleanly shut down EmulationStation if it is running #37

Closed jarettmillard closed 6 years ago

jarettmillard commented 6 years ago

This shuts down EmulationStation cleanly using ES' own shutdown mechanism. If ES is not running, it falls back to just running sudo shutdown -h now as it previously did.

Fixes #19

kiteretro commented 6 years ago

Looks great, what is the behaviour if something is launched from ES, and then you trigger shutdown (ie. running something in retroarch)? Is that treated as ES not running (interested in the expected behaviour)

kiteretro commented 6 years ago

Also, what is the file /tmp/es-shutdown? I assume this is generated by ES?

jarettmillard commented 6 years ago

If you have an emulator running in ES and you kill ES I assume the emulator just ends when the system shuts down. ES might do its own clean shutdown of a running emulator, but I'm not sure. I can test that out if you like.

/tmp/es-shutdown is a flag file that tells ES to halt the system after it exits. You can see how it works by looking at the ES wrapper script at /opt/retropie/supplementary/emulationstation/emulationstation.sh.

kiteretro commented 6 years ago

Ok nice, all sounds good. Do you think it would be a good idea to put in a sleep of like 10 secs or so, in case there is fault/crash with ES .. because otherwise the code would continue and close the python script down without calling a reboot at all..

So the logic can change slightly to always do the old reboot, but if ES is running it does the logic + a 10 sec sleep afterwards. This way whatever happens the shutdown command will always be sent. Do you agree?

jarettmillard commented 6 years ago

The only problem with that is that shutdown would pause for 10 seconds until the cs-osd service finished, wouldn't it? I initially tried to just send a SIGTERM to ES and call wait on the pid, but even that still resulted in ES not saving its files. Maybe I can wait for the wrapper script to finish instead. I'll experiment with it some.

kiteretro commented 6 years ago

I assumed that ES would then trigger the actual system shutdown? If so then the shutdown will trigger the cs-osd service to stop anyway. I'll have a go with your code this evening and see how it works as I haven't used these methods before. I'll report back with feedback :) Thank you for taking the time!

jarettmillard commented 6 years ago

I was thinking that since cs-osd is running as a service the system will wait for it to finish on its own at shutdown instead of forcefully killing it, but on second thought it might not work like that.

kiteretro commented 6 years ago

I most likely need to make a signal handler for the stop signal, at the moment I have a feeling it will just have an exception and exit, I only have a try/except on a KeyboardInterrupt :) I'll have a poke later

kiteretro commented 6 years ago

Could you take a look at the change I have made (in this merge) and let me know if that is ok? I have tested it on my system and it works as expected. After the os.kill on the ES pid is sent, ES does the shutdown immediately. The sleep allows for a fallback in case there is problem.. do you see any issues here? It could be extended to a larger number if you think the ES saving of meta data would take a while..

jarettmillard commented 6 years ago

You might want to put the flag back on the off chance that there are 2 instances of ES running. Other than that, if sleep isn't blocking shutdown that seems like a good solution to me.

kiteretro commented 6 years ago

Ok good call, I’ll make change tomorrow and merge it :) thanks for the help and contribution, it’s appreciated :)

On 28 Mar 2018, at 23:18, Jarett Millard notifications@github.com wrote:

You might want to put the flag back on the off chance that there are 2 instances of ES running. Other than that, if sleep isn't blocking shutdown that seems like a good solution to me.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

jarettmillard commented 6 years ago

No problem. Happy to help.

jarettmillard commented 6 years ago

Oh, I meant to do the check outside the loop and use the flag to sleep if you found any. That way you'd kill all the potential ES instances.

kiteretro commented 6 years ago

Ah I see your point, hopefully that's the last change ;)

jarettmillard commented 6 years ago

Looks good to me.

kiteretro commented 6 years ago

Tested and merged :)