KMKfw / kmk_firmware

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

Should Modules and Extensions exceptions propagate? #428

Open morrijr opened 2 years ago

morrijr commented 2 years ago

(Out of the discussion on #423)

An exception raised by these should fail the keyboard? If so, which?

Extension: def enable(self, keyboard): def disable(self, keyboard): def on_runtime_enable(self, keyboard): def on_runtime_disable(self, keyboard): def during_bootup(self, keyboard): def before_matrix_scan(self, keyboard): def after_matrix_scan(self, keyboard): def before_hid_send(self, keyboard): def after_hid_send(self, keyboard): def on_powersave_enable(self, keyboard): def on_powersave_disable(self, keyboard):

Module: def during_bootup(self, keyboard): def before_matrix_scan(self, keyboard): def after_matrix_scan(self, keyboard): def process_key(self, keyboard, key, is_pressed, int_coord): def before_hid_send(self, keyboard): def after_hid_send(self, keyboard): def on_powersave_enable(self, keyboard): def on_powersave_disable(self, keyboard):

kdb424 commented 2 years ago

Extensions should be considered critical. If during boot, I think we may not want to post if there is going to be a fault "randomly" for the user. Modules are sandboxed to not care if it functions or not, and is not critical. Good example would be RGB is not needed to work the keyboard, and can live in a sandbox. If it crashes, your keyboard won't be taken with it, so silent errors should be considered as ok.

morrijr commented 2 years ago

To confirm my understanding;

kdb424 commented 2 years ago

At least in terms of booting modules, for sure. In terms of other things, especially power save, I don't know if it should. Modules should be left self contained, failing without care though yes. I'd like some input from others on this as well.

morrijr commented 2 years ago

Further to this, should the default implementation raise a "Not Implemented" or just do nothing?

xs5871 commented 2 years ago

Isn't it the other way around? Extensions only get the sandbox, modules have full access. I'd also deduce that from the names "extension" -> something peripheral, non-critical, and "module" -> tightly coupled to the core.

xs5871 commented 2 years ago

On this discussion specifically: I'd prefer the board to be in "as much of an operational state as possible". That means catching all exceptions, but showing them while debugging. Modules and extensions should gracefully fail or bail out, but never crash the firmware. That's something I'm a bit annoyed with, that current debugging doesn't show stack traces. I also started on better logging on seperate branch.

morrijr commented 2 years ago

Perhaps a 'fail early when debugging'?

Things can't always fail gracefully. That's what exceptions were intended for (going back to the original reasons C++ had exceptions added to it).

Downside of keyboards. Limited feedback capability :)

If the keyboard should crash as little as possible, should the default implementation of the extension/module methods raise an exception. That seems counter intuitive to me. Means I must implement everything to make sure it doesn't throw (this isn't obvious at the moment because all the exceptions are caught and discarded).

morrijr commented 2 years ago

Isn't it the other way around? Extensions only get the sandbox, modules have full access. I'd also deduce that from the names "extension" -> something peripheral, non-critical, and "module" -> tightly coupled to the core.

That's the way around I would have expected things too, but I'm new to this project and wouldn't like to judge!

kdb424 commented 2 years ago

Means I must implement everything to make sure it doesn't throw This was at least the original intent I think when I designed this system. That said, people have pushed it further than I did, clearly, so I'm open for other's opinions on this. Python is typically designed for "ask for forgiveness, not permission" type of language.