Add call to tud_task in getchar() #918

2022-07-13T14:07:56-05:00

kilograham commented

2022-07-13 14:07:56 -05:00

(Migrated from github.com)

Depending on USB timing, the low priority may never call tud_task() during `getchar()
Also fixed a 1.4.0 regression which may cause tud_task() to not be called after an IRQ if the mutex wasn't available (previously we tried again automatically ever 1ms)

* Depending on USB timing, the low priority may never call `tud_task()` during `getchar() * Also fixed a 1.4.0 regression which may cause `tud_task()` to not be called after an IRQ if the mutex wasn't available (previously we tried again automatically ever 1ms)

liamfraser (Migrated from github.com) reviewed

2022-07-14 04:38:01 -05:00

kilograham commented

2022-07-14 11:40:33 -05:00

(Migrated from github.com)

ha, as I looked at this again, i realize it should be canceling the existing alarm.. i will add a fix

lurch (Migrated from github.com) reviewed

2022-07-21 05:35:45 -05:00

src/rp2_common/pico_stdio_usb/stdio_usb.c

So AFAICT you're setting one_shot_timer_crit_sec to NULL, here, and then elsewhere you're doing critical_section_is_initialized(&one_shot_timer_crit_sec). But given that critical_section_is_initialized checks crit_sec->spin_lock != 0, won't that lead to a null-pointer dereference? 🤷‍♂️

So AFAICT you're setting `one_shot_timer_crit_sec` to NULL, here, and then elsewhere you're doing `critical_section_is_initialized(&one_shot_timer_crit_sec)`. But given that `critical_section_is_initialized` checks `crit_sec->spin_lock != 0`, won't that lead to a null-pointer dereference? :man_shrugging:

kilograham (Migrated from github.com) reviewed

2022-07-24 09:54:34 -05:00

src/rp2_common/pico_stdio_usb/stdio_usb.c

setting the contents of one_shot_timer_crit_sec to zero.

liamfraser (Migrated from github.com) reviewed

2022-07-27 09:49:47 -05:00

liamfraser (Migrated from github.com) left a comment

No description provided.

lgtm but maybe get @Wren6991 to review also before merging

Wren6991 (Migrated from github.com) reviewed

2022-08-08 03:51:06 -05:00

src/rp2_common/pico_stdio_usb/stdio_usb.c

				`@ -25,6 +25,11 @@ static uint8_t stdio_usb_core_num;`

if this fails, you have initialized stdio_usb on the wrong core

Which core is this supposed to be? The one taking the IRQs for the default timer pool? Maybe it's worth making that constraint explicit in the doc comment for stdio_usb_init, especially if the outcome of this assertion is that you won't be getting any stdout.

Might it also be worth failing earlier, at init time, if the USB IRQ is not shared and the init is called on a different core than the default timer IRQ is serviced on?

> if this fails, you have initialized stdio_usb on the wrong core Which core is this supposed to be? The one taking the IRQs for the default timer pool? Maybe it's worth making that constraint explicit in the doc comment for `stdio_usb_init`, especially if the outcome of this assertion is that you won't be getting any stdout. Might it also be worth failing earlier, at init time, if the USB IRQ is not shared and the init is called on a different core than the default timer IRQ is serviced on?

kilograham (Migrated from github.com) reviewed

2022-08-08 09:40:43 -05:00

src/rp2_common/pico_stdio_usb/stdio_usb.c

				`@ -25,6 +25,11 @@ static uint8_t stdio_usb_core_num;`

I have moved the assertion (it remains an assertion)... i added a return code to stdio_init_all() as it wasn't even previously passed thru.

Note, i also notice that stdio_usb.c won't even compile if PICO_TIME_DEFAULT_ALARM_POOL_DISABLED=1 though no one has apparently needed this. It might be worth adding the ability to initialize stdio_usb with your own alarm pool instead, but that is for another day

I have moved the assertion (it remains an assertion)... i added a return code to `stdio_init_all()` as it wasn't even previously passed thru. Note, i also notice that `stdio_usb.c` won't even compile if `PICO_TIME_DEFAULT_ALARM_POOL_DISABLED=1` though no one has apparently needed this. It might be worth adding the ability to initialize `stdio_usb` with your own alarm pool instead, but that is for another day