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

Implement automatic firmware reload and show error stacktrace #969

Merged
merged 1 commit into from
Jun 8, 2024

Conversation

xs5871
Copy link
Collaborator

@xs5871 xs5871 commented May 19, 2024

Trigger a supervisor reload on any exception, restarting the firmware; unless it's a keyboard interrupt in which case we explicitly want to drop into the REPL.
Replace the sparse debug error message with a stacktrace.

Copy link
Member

@regicidalplutophage regicidalplutophage left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe reload = True should be an object in KMKKeyboard so that we can toggle this behavior in userspace?

@xs5871
Copy link
Collaborator Author

xs5871 commented May 20, 2024

It started out that way and then I removed it. Couldn't think of a reason to ever disable this in "normal" use; I always rather have the keyboard reboot than manually unplug-replug it.
Only when debugging crashes would I maybe want to stop the autoreload occasionally, and then I have the serial console open anyways and can just look at the terminal history or hit ctrl-c (or disable reloading in code).

@regicidalplutophage
Copy link
Member

Well, ideally errors shouldn't happen, I'd argue if they are that's barely a normal use

@xs5871
Copy link
Collaborator Author

xs5871 commented May 23, 2024

Any sufficiently complex software has bugs, encountering them is part of the UX, as undesireable as that may be. That is beside the point though.

Can you give a realistic example where turning of the reload is an improvement to the user experience, discounting scenarios that require the serial console?

@claycooper
Copy link
Member

The only issue I can think of with auto-restarting is that a crash caused by an obscure, rare bug would get masked and attributed to an unrelated symptom however that's more of an item to note when troubleshooting or assisting someone and not a blocker to this PR.

I'll approve this PR and leave it up to @xs5871 to merge depending on the conversation's outcome with @regicidalplutophage.

@xs5871
Copy link
Collaborator Author

xs5871 commented May 26, 2024

I wouldn't say it "masks" issues, it automates the recovery. Crashes should still be very noticable.
Our recommendation for figuring out any kind of unexpected behavior already is: check debug output. Doesn't really impact diagnostic procedure at all, in my opinion. Even with the current crash and drop to REPL you'd only know something died by checking the logs.

@regicidalplutophage
Copy link
Member

What I'm the most afraid of is it "masking" some issue created in userspace by a user. And even as a developer it would be helpful if it can be disabled easily. If something goes wrong it's more often than not an easy fix and we want to fix it right away and for that execution has to be stopped.

If we were to provide some file with "sane defaults" to enable after a keyboard does everything we want to (like boot options) enabling autoreload can be a part of it.

@xs5871
Copy link
Collaborator Author

xs5871 commented Jun 8, 2024

Here's an idea: we could couple the reload to the debug flag, which is already automagically enabled and disabled depending on an active connection to the serial console.

@regicidalplutophage
Copy link
Member

Yeah, that would be perfect

@xs5871 xs5871 force-pushed the enhancement-error-handling branch from 999517f to 61b6eba Compare June 8, 2024 20:17
Trigger a supervisor reload on any exception, restarting the firmware; unless
it's a keyboard interrupt in which case we explicitly want to drop into the
REPL.
Replace the sparse debug error message with a stacktrace.
@xs5871 xs5871 merged commit e79414e into main Jun 8, 2024
2 checks passed
@xs5871 xs5871 deleted the enhancement-error-handling branch June 8, 2024 20:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants