KMKfw / kmk_firmware

Clackety Keyboards Powered by Python
https://kmkfw.zulipchat.com
Other
1.32k stars 458 forks source link

Implement automatic firmware reload and show error stacktrace #969

Closed xs5871 closed 3 weeks ago

xs5871 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

xs5871 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 3 weeks ago

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 commented 3 weeks ago

Yeah, that would be perfect