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

Output Channels -- deadlock and generally broken #97

Open
henrygab opened this issue Sep 21, 2024 · 0 comments
Open

Output Channels -- deadlock and generally broken #97

henrygab opened this issue Sep 21, 2024 · 0 comments
Labels
bug Something isn't working

Comments

@henrygab
Copy link
Collaborator

henrygab commented Sep 21, 2024

Executive Summary

The current buffered IO, if used from Core1, is fundamentally broken, with both deadlocks and terminal IO corruption possible / likely.


Background

Most things run on Core0. Most output (terminal, statusbar, binmode, and debug channels) are generated from Core0. At the same time, there's nothing restricting Core1 from outputting to those channels. The current printf(), putc(), etc. using a blocking function to put the characters into a queue (backed by a fixed buffer), tx_fifo. The queue provides atomic insertions, and as used by the stdio-like functions, will block when the queue is full.

Core1's main loop checks for and drains the tx_fifo queue, sending the buffer over USB (and/or a hardware UART). It also does the same for the other channels (binmode, status bar, and maybe soon a debug output channel).


Deadlock

This one's easy to understand. If the queue is almost full (e.g., 5 open slots for a character), and Core1 calls printf("Hello, world!\n");, then it will deadlock. Specifically, it will add up to five characters (H, e, l, l, o), and then the next character will block due to the queue being full. Since Core1's main loop is now blocked, it never drains that queue, resulting in a deadlock.


Generally Broken

If you recall, the output channels use a queue, which gives atomic insertion of a byte. However, Core0 output to at least the main channel (e.g., terminal output) often includes VT100 sequences to add color, position the cursor onscreen, and more. Those VT100 sequences must be output atomically ... if another thread added to the tx_fifo queue in the midst of the VT100 sequence, the terminal screen would have ... undefined behavior. Generally, this is not an issue because Core0 is not running multiple threads, and it's well-known to not perform blocking IO in an ISR ... so nothing of note is interrupting Core0's execution.

However, if Core1 ever directly (or calls a function which) outputs to the tx_fifo channel, the terminal's VT100 sequences are highly likely to become corrupted. For example, Core1 might be calling printf("ABCDEFG"), while Core0 was calling printf("abcdefg"). The tx_fifo queue might therefore contain: ABabcdCDEFefGg ... or any other interleaved version of the output.


Potential Fixes

Fixing this, while not a huge problem is NOT a tiny change.

  • Optionally, explicitly disable output from Core1 that's destined for Terminal?
  • Optionally, explicitly disable output from Core1 that's destined for BinMode?

Otherwise, include terminal output queue in the below general solution:

  • Each core to have distinct buffers for output channel purposes, such as an array (e.g., tx_fifo[2], dbg_fifo[2]).
  • Modify output channel functions (e.g., printf(), etc. for terminal, macros for debug, etc.) to either:
    • detect at entry which core they are executing on, and passing the appropriate queue (e.g., tx_fifo_0 for Core0, tx_fifo_1 for Core1) to wrapped version of the function, --OR--
    • use CPUID SIO register to index into the corresponding per-CPU queue (get_core_num() from pico/platform.h)
  • Do not use blocking functions to add to the fifo queues. Instead, loop doing:
    • try_add() to add to the queue
    • return if addition to the queue was successful
    • if it failed and on Core1, immediately service / drain the queue

N.B. - CPUID/get_core_num() is a special SIO (single-cycle IO) register, so using it is particularly efficient for processor-specific tasks.


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant