Can't get stdio_set_chars_available_callback to work with USB

[HakanL]

I have a project where I want to get a callback for when I receive characters from the stdio USB virtual serial port. It works fine if I call getchar in the main loop, but in my main solution I can't easily do that so I was happy to find stdio_set_chars_available_callback, but I can't get it to work. The callback is called (I toggle an LED to show that my code is called), but I never receive the character in the callback method. Here's the callback method:

void received_char_callback(void* param)
{
    gpio_put(LED_PIN_INDICATOR, !gpio_get(LED_PIN_INDICATOR));

    int key = getchar_timeout_us(0); // get any pending key press but don't wait
    if (key > 0)
    {
        // We never get here
        receivedChar = (char)key;
    }
}

And I set it up like this:

stdio_set_chars_available_callback(received_char_callback, NULL);

And here's the simple repo: https://github.com/DMXCore/Pico-Examples/tree/main/UsbStdio

Am I doing anything wrong? I'm on Pico SDK 1.5.1. Surely this is supported by USB, not just UART, correct?

[peterharperuk]

Yes, it's supposed to work. received_char_callback is definitely getting called?

[HakanL]

Yes, it's definitely called, the LED is off until I send a character, then for every character (without reading it) the callback is called (the LED is flipped).

[HakanL]

Someone else had an issue with this as well, seems that it was never resolved for him: https://stackoverflow.com/questions/76482416/read-strings-from-usb-cdc-how-to-use-stdio-set-chars-available-callback https://forums.raspberrypi.com/viewtopic.php?t=352549

Searching for code using stdio_set_chars_available_callback returns almost nothing (which doesn't mean much of course, but still)

[peterharperuk]

I ran you code and it worked?

--==Init==-- Start! Last char: a Last char: a Last char: a Last char: b Last char: b Last char: d Last char: d Last char: e Last char: g Last char: i Last char: k Last char: m Last char: n Last char: p Last char: q Last char: r Last char: t Last char: u Last char: v Last char: w Last char: w Last char: y Last char: z Last char: z Last char: z Last char: z

[HakanL]

Hmmm, that's odd. What could the differences be?

[peterharperuk]

Ah - sorry, your program still had the uart enabled and that confused me. think I can repro the problem although I'm not getting any callbacks

[HakanL]

Ah, my bad, I had accidentally removed pico_enable_stdio_uart(UsbStdio 0) from the CMakeLists.txt file. Just pushed that back in so the repo is complete. Hmm, I'm definitely getting callbacks, so that's odd.

[HakanL]

My camera skills aren't amazing, but here's my set up, Pico with a simple serial terminal. As I type the LED flips, but it never prints the 'Last char' part.

https://github.com/raspberrypi/pico-sdk/assets/407941/d3133149-7f5b-42ae-aa52-d6923bffd8d0

[peterharperuk]

ok - I am getting callbacks. But the callback happens in the irq. I think you'll have a problem reading a character in an irq as we try and claim a mutex. I would modify your code to use an async worker.

[HakanL]

Ah I see, but isn't it odd that it worked for you before (I assume over UART)? Or at least it would need additional documentation and a user would probably assume that it should be possible to implement in the way I did.

[HakanL]

FWIW I added an async worker and then my code is working fine. Hopefully this issue will help other developers experiencing the same issue. Obviously it would be nice if it was possible to read the characters directly in the callback, just like for UART, so the behavior isn't inconsistent, but I can understand if it's not possible due to the difference between USB and UART.

[progvault]

Looks like the issue is indeed with the mutex in stdio_usb_in_chars() in stdio_usb.c. Does seem kind of odd to put a character available callback in the SDK and not be able to read characters in the callback. I wonder what the intention was?

Also, the logic around the mutex code was sort of confusing. After commenting out the mutex code inside stdio_usb_in_chars() in stdio_usb.c, as shown below, the example works correctly.

stdio_usb.c

int stdio_usb_in_chars(char *buf, int length) {
    // note we perform this check outside the lock, to try and prevent possible deadlock conditions
    // with printf in IRQs (which we will escape through timeouts elsewhere, but that would be less graceful).
    //
    // these are just checks of state, so we can call them while not holding the lock.
    // they may be wrong, but only if we are in the middle of a tud_task call, in which case at worst
    // we will mistakenly think we have data available when we do not (we will check again), or
    // tud_task will complete running and we will check the right values the next time.
    //
    int rc = PICO_ERROR_NO_DATA;
    if (stdio_usb_connected() && tud_cdc_available()) {
        //if (!mutex_try_enter_block_until(&stdio_usb_mutex, make_timeout_time_ms(PICO_STDIO_DEADLOCK_TIMEOUT_MS))) {
        //    return PICO_ERROR_NO_DATA; // would deadlock otherwise
        //}
        if (stdio_usb_connected() && tud_cdc_available()) {
            int count = (int) tud_cdc_read(buf, (uint32_t) length);
            rc = count ? count : PICO_ERROR_NO_DATA;
        } else {
            // because our mutex use may starve out the background task, run tud_task here (we own the mutex)
            tud_task();
        }
        //mutex_exit(&stdio_usb_mutex);
    }
    return rc;
}

main.cpp

#include <stdio.h>
#include "pico/stdio.h"
#include "pico/stdlib.h"

#define MAX_LEN 32
const uint LED_PIN_INDICATOR = 25;

void chars_available_callback(void* param)
{
    // toggle led
    gpio_xor_mask(1u << PICO_DEFAULT_LED_PIN);   

    // get any pending key presses
    volatile signed char *c = (signed char *) param;
    while ((*c = getchar_timeout_us(0)) > 0)
        c++;
    *c = '\0';
}

int main()
{
    stdio_init_all();

    // There isn't a strict rule that you must always call sleep_ms()
    // after stdio_init_all(). However, in some cases, it can be a helpful
    // precautionary measure to ensure that the UART has properly 
    // initialized and is ready to transmit data without any issues.
    sleep_ms(2000);

    printf("--==Init==--\n");

    // Init all inputs and outputs
    gpio_init(LED_PIN_INDICATOR);

    gpio_set_dir(LED_PIN_INDICATOR, GPIO_OUT);

    // Set initial state
    gpio_put(LED_PIN_INDICATOR, 0);

    static volatile signed char receivedChar[MAX_LEN] = {0};
    stdio_set_chars_available_callback(&chars_available_callback, (void*) receivedChar);

    // Send to console
    printf("Start!\n");

    while (1) {
        tight_loop_contents();
        if (!receivedChar[0]) continue;

        printf("String: %s", receivedChar);
        receivedChar[0] = '\0';
    }
}

[arg08]

Yes, this is a really evil bug in the existing implementation of stdio_set_chars_available_callback() for USB. Yet another report of it on the forums yesterday, triggered by the pico-examples pico-examples/pico_w/wifi/iperf/picow_iperf.c which provides an example that doesn't currently work.

In terms of fixing this properly, the characters can only ever become available as a result of calling tud_task(). tud_task must be called with the mutex claimed (to satisfy TinyUSB reentrancy requirements), and the tud_cdc_rx_cb() will therefore always be called with the mutex claimed. So there really isn't any point in using the tud_cdc_rx_cb() mechanism - the stdio code can just check for chars available after calling tud_task()

This patch fixes it:

diff --git a/src/rp2_common/pico_stdio_usb/stdio_usb.c b/src/rp2_common/pico_stdio_usb/stdio_usb.c
index 0dc8d6d..b1f297c 100644
--- a/src/rp2_common/pico_stdio_usb/stdio_usb.c
+++ b/src/rp2_common/pico_stdio_usb/stdio_usb.c
@@ -56,8 +56,11 @@ static int64_t timer_task(__unused alarm_id_t id, __unused void *user_data) {

 static void low_priority_worker_irq(void) {
     if (mutex_try_enter(&stdio_usb_mutex, NULL)) {
+               uint32_t chars_avail;
         tud_task();
+               chars_avail = tud_cdc_available();
         mutex_exit(&stdio_usb_mutex);
+               if (chars_avail)  chars_available_callback(chars_available_param);
     } else {
         // if the mutex is already owned, then we are in non IRQ code in this file.
         //
@@ -147,12 +150,6 @@ int stdio_usb_in_chars(char *buf, int length) {
 }

 #if PICO_STDIO_USB_SUPPORT_CHARS_AVAILABLE_CALLBACK
-void tud_cdc_rx_cb(__unused uint8_t itf) {
-    if (chars_available_callback) {
-        usbd_defer_func(chars_available_callback, chars_available_param, false);
-    }
-}
-
 void stdio_usb_set_chars_available_callback(void (*fn)(void*), void *param) {
     chars_available_callback = fn;
     chars_available_param = param;

The above patch doesn't try to fix the other calls to tud_task() that are made in a rather kludgy way in the _in_chars/_out_chars blocking functions. IMO the _in_chars one doesn't want fixing (you really don't want a chars available callback when you are already inside a blocking read chars call), the _out_chars one could potentially deserve the same fix - check for chars available then call the user function with the mutex dropped - but again debatable if you actually want this in the middle of a blocking output call, specially for the normal use for this in printf() etc. So I wouldn't do any more.

[kilograham]

merged into develop