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

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

Merged
merged 13 commits into from
Jun 21, 2024
Merged

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

merged 13 commits into from
Jun 21, 2024

Conversation

gsurkov
Copy link
Member

@gsurkov gsurkov commented Jun 21, 2024

What's new

  • Add a signal API for threads
  • Integrate signals into the Loader and EventLoop subsystems
  • Add loader close CLI subcommand
  • Show the application name when loader info is executed (FL-3513)

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)

  • PR has description of feature/bug or link to Confluence/Jira task
  • Description contains actions to verify feature/bugfix
  • I've built this code, uploaded it to the device and verified feature/bugfix

Copy link

github-actions bot commented Jun 21, 2024

Compiled f7 firmware for commit 7bd18a91:

@gsurkov gsurkov marked this pull request as ready for review June 21, 2024 14:42
@CookiePLMonster
Copy link
Contributor

CookiePLMonster commented Jun 21, 2024

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.

Co-authored-by: Silent <[email protected]>
@gsurkov
Copy link
Member Author

gsurkov commented Jun 21, 2024

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
Copy link
Contributor

CookiePLMonster commented Jun 21, 2024

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++;

@skotopes skotopes merged commit 4cf9886 into dev Jun 21, 2024
11 checks passed
@skotopes skotopes deleted the gsurkov/3847_thread_signals branch June 21, 2024 20:44
@gsurkov
Copy link
Member Author

gsurkov commented Jun 21, 2024

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

@CookiePLMonster
Copy link
Contributor

CookiePLMonster commented Jun 21, 2024

@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.

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.

4 participants