Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Kobo] Restart KOReader after a crash #5328

Merged
merged 23 commits into from Sep 7, 2019

Conversation

@NiLuJe
Copy link
Member

commented Sep 6, 2019

Because getting booted back to Nickel is mildly annoying ;).

(Keeps track of crashes to avoid getting stuck in boot loops).

NiLuJe added 6 commits Sep 6, 2019
Restart KOReader after a crash.
Kobo only, because it's possibly the platform where getting booted out
of KOReader is the most annoying.

Keep track of crashes, to be able to give up after a while, in order to
avoid boot loops.

Depends on koreader/koreader-base#966
Bump base
We need OpenType support in FBInk
(koreader/koreader-base#966)
@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

Looks something like that in action:

gsod

With KFMon's replacement boot progress bar on top:

gsod2

NiLuJe added 3 commits Sep 6, 2019
Batch both prints in a single flashing refresh
Should make one-off crashes more obvious.

(And a crash-loop slightly less annoying than if we were to only flag
the gray clear as flashing).
Slightly more generc approach to the "pause on crash" issue
By keeping track of the last crash's timestamps, and only pausing if
there's been more than 5s between crashes.

(The idea being not to pause more than once in a crash loop).
@poire-z

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

Not so fond of the idea, but may be that's because I'm a dev - while users may like it.
I use KSM, and when it crashes, I get back to KSM with some lower light level (that's my grey screen of death :), and I can just hit "start koreader".

Additionally, depending on what I suspect caused the crash (if I'm testing stuff), I do not want to restart koreader. Possibly because then I would just be waiting 15s before being able to get back to usb, and it would fill crash.log with multiple instances of the crash.
And it could possibly overwrite/rotate the settings.reader.lua / book's metadata.lua.old and the previous OK one would be lost (while no-restart would not trash the settings.reader.lua.old or metadata.lua.old and you'll be able to recover them, in a state before some setting change that could be the cause of the crash).
Why 5 restarts ? If the first one crashes too, there's probably no reason for the 2nd, 3rd, 4th and 5th to work better :)

I'll probably patch that out for my own safer usage (may be you could make that an option ? in developpers options?).

(But may be I'd like a 1-time restart, with an excerpt of the crash log stacktrace on your grey screen :) so I know what next step to take: restart or go fix that over usb :)

@Frenzie

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

@poire-z I think the 5 restarts might be meant to be accumulative over the course of weeks. If so, it might be better to store the date and see if the last two restarts occurred within the last 10 seconds or so.

@Frenzie Frenzie added this to the 2019.10 milestone Sep 6, 2019

@Frenzie

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

PS 2019.10 so that 2019.09 can go out this weekend. ;-)

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

@Frenzie: That's exactly what this does ^^ (except with a 5s grace period instead of 10 ;p). Plus, any "expected" exit (i.e., a KOReader restart via the menu) will reset the counter.

@poire-z: The idea behind the 5 (well, in practice, 6) crash limit is to avoid getting stuck in a loop in the unlikely event we crash on startup.
In which case, the delay only gets applied to the first of those crashes, because, yeah, that'd be annoying otherwise ;). (Which means a full crash loop ends up tallying neatly at right below the 5s grace period).

The vast majority of crashes do NOT happen on startup, though, and in 99% of thoses cases, you will probably end up restarting KOReader straight away ;).

That said, I do get the point about wanting to just die on crash as usual, I'll look into it ;).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

@poire-z: And I was indeed thinking about how best to mangle a relevant bit of the tail of the crash.log somewhere around the bottom of that grey screen ;).

@Frenzie

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

@NiLuJe My point is more that if you're running the same session for several months you could rack up a total of 5 crashes, each of which might've occurred weeks apart.

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

@NiLuJe: Oh, right, I see. Yeah, that makes sense ;).

@Frenzie Frenzie added the enhancement label Sep 6, 2019

NiLuJe added 2 commits Sep 6, 2019
@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

Okay, log printing is in. Caught an FBInk bug in the process, yay :D.

(Hi, #5323 ;p)

crashy

# U+1F4A3, the hard way, because we can't use \u or \U escape sequences...
# shellcheck disable=SC2039
./fbink -q -b -O -m -t regular=./fonts/freefont/FreeSerif.ttf,px=${bombHeight},top=${bombMargin} $'\xf0\x9f\x92\xa3'
# And then print the tail end of the log on the bottom of the screen...
crashLog="$(tail -n 25 crash.log)"

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 6, 2019

Member

Fancy, although if it's only for 2 seconds you might have to be a pretty fast reader. ;-)

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Sep 6, 2019

Author Member

Oh, yeah, good point, delay should be higher now that it does this ;p.

This comment has been minimized.

Copy link
@poire-z

poire-z Sep 6, 2019

Contributor

Yep :) Ideally, I'd like some button dialog on that, that waits for my choice: Exit | Restart, so we get the time to read it and decide. Can fbink do that ? :)

(After a crash, I usually read the crash.log with a KSM script in KSM own clumsy text viewer, or launch Koreader and view it in TextEditor - so that fbink display would avoid that need.)

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Sep 6, 2019

Author Member

It's... tricky because of the various crappy input protocol handling on Kobos.

What might be doable is a single button, saying "Tap screen to <restart|exit> KOReader", the context (i.e., restart or exit) being the current position in the crash counter.
In which case we only need to listen for the first EV_KEY (or whatever) event that makes sense, and a tap anywhere would do the trick.

This comment has been minimized.

Copy link
@poire-z

poire-z Sep 6, 2019

Contributor

If I'm able to just hack that counter to make my tap be always too many already, and make it always exit, I'm super fine with that :)

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Sep 6, 2019

Author Member

I went with a "tap to continue" on the first crash in a series, which should work well enough in practice ;).

Kept a timeout in place in case all hell breaks loose and touch is broken ;).

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Sep 6, 2019

Author Member

@poire-z: You raise an interesting conundrum: I can add an option in the Dev menu, but what it actually does is up for grabs:

I was originally thinking of simply completely reverting back to the previous behavior.
But I could also (and, in fact, it's probably going to be tiny bit easier) make it always print the log, wait for tap & exit.

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Sep 6, 2019

Author Member

(essentially, check that dev setting, if it's on, always enforce the counter to 1, and at the end, just break).

This comment has been minimized.

Copy link
@poire-z

poire-z Sep 6, 2019

Contributor

superfinewithme :)

NiLuJe added 5 commits Sep 6, 2019
Manually post-processes tabs into four spaces
Because otherwise FBInk tries to print a TAB glyph ;p.
Tweak crash accounting a bit
To be more amenable to extremely long-running sessions

(i.e., we reset to the first crash based on the delat between the last
two crashes).

Also, pause for longer on a the first crash, now that we display a log
excerpt.

And, make the crash limit actually 5, not 6.
Instead of a hard-coded sleep, wait for a touch event
With a fallback timeout, just in case.
@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

Okay, dev option is in!

Final? iteration looks like this:

crashy

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

There are a few other fun icons available, FWIW.

I settled with the bomb because it made me laugh, and (somehow, despite me barely ever having used a classic Mac) reminded me of the old Mac OS system error popup...

In the same vein, there's a "Fire" icon, which could be a nod to HCF/Halt and Catch Fire.

Or less nerdy stuff like skull & bones or something ;p.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

Just looked at some page listing some emoticons where I found yours... and there's not many alternatives.
This one is nice in full massive black, except that it expresses that a bomb will trigger - while the crash has already happened :)

@Frenzie

This comment has been minimized.

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Sep 6, 2019

There's also a "kapow" like explosion outline, but getting the two to align was tricky, so I gave up ;p.

@Frenzie Frenzie modified the milestones: 2019.10, 2019.09 Sep 6, 2019

# Cue a lemming's faceplant sound effect!

echo "!!!!" >>crash.log 2>&1
echo "Hu oh, something went awry... (Crash n°${CRASH_COUNT}: $(date +'%x @ %X'))" >>crash.log 2>&1

This comment has been minimized.

Copy link
@Frenzie

Frenzie Sep 6, 2019

Member

Btw, is this on purpose or a typo? ;-P

This comment has been minimized.

Copy link
@NiLuJe

NiLuJe Sep 6, 2019

Author Member

Which part? ^^

@Frenzie
Frenzie approved these changes Sep 6, 2019
@pazos

This comment has been minimized.

Copy link
Contributor

commented Sep 6, 2019

if I had a Kobo I would try to crash it just to see that screen. ❤️

NiLuJe added 6 commits Sep 6, 2019
Bump base
(Pickup the missing rtc stuff)
Words are hard!
(Huh. Uh. Not Hu! :D).
Tweak margin & sizes computations, so that they have a better chance of
behaving as expected on a wider range of devices.
Really make shfmt happy
That div vs. mul distinction looks fishy...

@NiLuJe NiLuJe merged commit a3acc66 into koreader:master Sep 7, 2019

1 check passed

ci/circleci: build Your tests passed on CircleCI!
Details
@Frenzie

This comment has been minimized.

Copy link
Member

commented Sep 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.