Skip to content

Espressif: Fix interrupts in UART workflow #6187

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

Merged
merged 2 commits into from
Mar 31, 2022

Conversation

prplz
Copy link

@prplz prplz commented Mar 23, 2022

Fixes uart workflow on espressif not being able to interrupt code and slowness at the "press any key" prompt.

While I was working on this I noticed a board with a neopixel status LED didnt suffer from the slowness due to incorect expansion of CIRCUITPY_STATUS_LED so I fixed that too.

@tannewt
Copy link
Member

tannewt commented Mar 23, 2022

Please explain why this fixes the UART issue. Ideally the UART code shouldn't need special cases for the debug UART.

@prplz
Copy link
Author

prplz commented Mar 25, 2022

First bug: with uart workflow (esp32-c3 boards mostly), pressing ctrl+c in the repl does nothing, while it should schedule a keyboard interrupt. In the usb cdc workflow this is done by setting a "wanted" char of ctrl+c and calling mp_sched_keyboard_interrupt when it's seen. Note the wanted char is only listened for on the first cdc interface (usb_cdc.console and not usb_cdc.data). Short of re-writing the uart code to use our own queues, the easiest way to do this same behaviour on uart was using an event listener task with the uart's pattern detection (uart_enable_pattern_det_baud_intr).

Second bug: like in #6046, the uart console respond slowly to the "press any key prompt". This is fixed by waking up the main task on the data event.

Third bug: I noticed while writing these fixes that a board with a neopixel status LED didn't suffer from the slow any key response bug. I tracked it down to an incorrect expansion of CIRCUITPY_STATUS_LED which was causing the loop to be constantly woken up, so I added parenthesis to make it expand correctly.

I put the event listener behind an ifdef because it is only used on boards that have a debug uart, and it should only act on the uart hosting the console.

@tannewt
Copy link
Member

tannewt commented Mar 25, 2022

Thank you for the explanation! Would you mind including a version of it before event_task to explain how it works there?

@prplz prplz force-pushed the espressif-uart-workflow-fixes branch from 90ac070 to 6860124 Compare March 25, 2022 05:20
@prplz
Copy link
Author

prplz commented Mar 25, 2022

I made the discussed changes: the task now starts for every uart and added some more comments. Let me know if theres anything else I should do.

@prplz prplz force-pushed the espressif-uart-workflow-fixes branch from 7aaae37 to f96cd73 Compare March 26, 2022 23:10
Copy link
Collaborator

@dhalbert dhalbert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, merging based on @tannewt's review.

@dhalbert dhalbert merged commit ec5c950 into adafruit:main Mar 31, 2022
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.

3 participants