flipperdevices / flipperzero-firmware

Flipper Zero firmware source code
https://flipperzero.one
GNU General Public License v3.0
11.69k stars 2.59k forks source link

[FL-3847, FL-3513] Thread Signals #3730

Closed gsurkov closed 1 week ago

gsurkov commented 1 week ago

What's new

Verification

  1. Start any EventLoop-based application, for example: loader open NFC
  2. Run the loader info command. The application name should be printed.
  3. Run the loader close command. The application should exit and the corresponding message should be printed.
  4. Start any application that does not use the EventLoop subsystem.
  5. Repeat step 3. This time, an error message should be printed.

Checklist (For Reviewer)

github-actions[bot] commented 1 week ago

Compiled f7 firmware for commit 7bd18a91:

CookiePLMonster commented 1 week ago

Can this be expanded to send arbitrary (user) signals from CLI to the application? Everything in this PR appears to be ready for that, except for loader_cli.

An ability to send arbitrary signals via e.g. loader signal 101 [optional_args] could come in handy for debugging, especially without a developer board. For example, one could use those signals to toggle debug messages on/off without the need to employ awkward button combinations, to skip to a specific part of the application, or to toggle a developer mode.

EDIT: One more remark - when I first saw the PR, I thought those signals are going to execute in the context of the thread a signal is sent to. It might be worth mentioning in the docs that this is not the case, and the signaled thread may run concurrently with the signal function.

Realistically I don't see a way to ensure that this callback runs in the correct context, but for the end user it's easy if they just serialize signals via a queue.

gsurkov commented 1 week ago

An ability to send arbitrary signals via e.g. loader signal 101 [optional_args] could come in handy

done.

I thought those signals are going to execute in the context of the thread a signal is sent to.

No, this is just a settable callback, the recipient is responsible for queueing the request, if necessary, on their side (this highly depends on the concrete implementation of the handling mechanism).

CookiePLMonster commented 1 week ago

done.

Great! Just one nitpick - if instead of %p the argument became a pointer to the first non-whitespace character after the signal number (or NULL if that character happens to be \0), the arguments would become arbitrary. It'd then be up to the app to treat it as a string, parse as integer etc.

You should be able to easily do it with strtoul (as it gives a pointer to the first character after the parsed integer) + while (isspace(*ch)) ch++;

gsurkov commented 1 week ago

@CookiePLMonster nice idea, we'll see how to best implement it a bit later.

CookiePLMonster commented 1 week ago

@CookiePLMonster nice idea, we'll see how to best implement it a bit later.

Sounds good! As I mentioned, strtoul (instead of sscanf) will do most of the job for you since it provides an output variable pointing at the character the function finished parsing at. From there it's just the matter of scanning for the first non-whitespace char, which will be either the user-supplied string or \0. The revised approach shouldn't be any more complex than the current sscanf.