8000 esp32/uart: Correctly init uart driver for repl. by andrewleech · Pull Request #8279 · micropython/micropython · GitHub
[go: up one dir, main page]
Skip to content

esp32/uart: Correctly init uart driver for repl. #8279

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

esp32/uart: Correctly init uart driver for repl. #8279

wants to merge 1 commit into from

Conversation

andrewleech
Copy link
Contributor

Initial attempt to replace the low level "raw" uart driver in uart0 (repl) with the standard high level idf uart driver.

There's some code cleanup (include files) still required, and possibly removal of the ringbuffer still in use for usb / dupterm?
Not sure if any of the other uart event queue types would be useful for anything (example code commented out currently).

Tested with the following test script:

import uos
import time
def run_test():
    start = time.ticks_us()
    loops = 5
    for _ in range(loops):

        uos.listdir('.')
        with open('micropython.map', 'r') as f:
            for line in f.readline():
                pass

    end = time.ticks_us()
    taken = time.ticks_diff(end, start) / 5
    print(f"taken: {taken/1000}us", )

with mpremote mount .

on master:

Local directory . is mounted at /remote
Connected to MicroPython at COM3
Use Ctrl-] to exit this shell
>
MicroPython v1.18-74-gfeeeb5ea3-dirty on 2022-02-09; ESP32 module with ESP32
Type "help()" for more information.
>>> import test;test.run_test()
taken: 1826.761us
>>> import test;test.run_test()
taken: 1824.004us

With this branch:

Local directory . is mounted at /remote
Connected to MicroPython at COM3
Use Ctrl-] to exit this shell
>
MicroPython v1.18-75-gc1bfd6616-dirty on 2022-02-09; ESP32 module with ESP32
Type "help()" for more information.
>>> import test;test.run_test()
taken: 1701.49us
>>> import test;test.run_test()
taken: 1704.198us

Ctrl-C seems to work from repl, eg

>>> import time
>>> while 1:
...     time.sleep(1)
...     print(1)
...
1
1
Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
KeyboardInterrupt:

For related information see #8255 and #8277

@andrewleech andrewleech mentioned this pull request Feb 10, 2022
Closed
dpgeorge
dpgeorge reviewed Feb 11, 2022
View reviewed changes
dpgeorge
dpgeorge reviewed Feb 11, 2022
View reviewed changes
ports/esp32/uart.c Outdated
uart_repl_set_pattern_detect(mp_interrupt_char);

//Create a task to handler UART event from ISR
xTaskCreate(uart_event_task, "uart_event_task", 2048, NULL, 12, NULL);
Copy link
Member

Choose a reason for hiding this comment

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

A shame that it needs to create a whole new task just to manage ctrl-C interrupts... is there another way to do this? Can we still use uart_isr_register()? How about uart_set_select_notif_callback()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the task does seem wasteful. I initially tried to follow the existing isr pattern, but the IDF high level driver itself is using the ISR for much of its behaviour, adding or own would need to replace that.
When I disabled their isr & enabled this one, I had to disable the tx buffer in uart_driver_install() as it's managed by their isr and it mostly worked. However any statement on repl that printed over a certain length ( ~132 characters iirc) it locks up.
Eg (by memory)

>>> [1]*10
[1, 1, 1, 1, 1, 1, 1, 1, 1, 1]
>>> [1]*100
[1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1, 1,

then nothing, couldn't type anything else, couldn't ctrl-c.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't seen uart_set_select_notif_callback() before though, I'll look into it.

@andrewleech
Copy link
Contributor Author

Maybe it would be better to skip their ISR and instead make an extended version of the micropython ringbuffer to support looking for the interrupt character. That code is duplicated a lot, it would probably be good to have a consolidated stdio buffer for both in and out with the interrupt char handling in just that one place?
The out buffer could have two interfaces for the write immediate vs write on connect like stm32 so that the startup message is buffered for when USB /uart is first connected?

@wsyfree wsyfree mentioned this pull request Feb 15, 2022
Open
@andrewleech
Copy link
Contributor Author

yep, uart_set_select_notif_callback() works! ugly wip commit just pushed now

@andrewleech
Copy link
Contributor Author

Hey @UnexpectedMaker would it be possible for you to test this branch to see if it fixes the uart repl bootloop issue you were talking about?

@andrewleech
Copy link
Contributor Author

yep, uart_set_select_notif_callback() works! ugly wip commit just pushed now

actually... I don't think it can work quite well enough - that callback always appears to be "a character behind" when it comes to interrupt char. In a while 1 loop on repl, I can hit ctrl-c but then need to hit an extra key to flush it through before it breaks out of the loop.

Looking at the IDF code (https://github.com/espressif/esp-idf/blob/2522277dfe51827575ea4724a1e54d1103aafd6f/components/driver/uart.c#L926) it looks like the interrupt handler reads the internal fifo, fires off this callback if configured, then pushes the data into the rx buffer.
So any data I try to read from buffer in the callback doesn't include whatever just triggered it - that's sitting in the static / private p_uart->rx_data_buf buffer I can't get access to.

Maybe replacing the ISR is the best option, though maybe extending it closer to the "full" version in IDF so it's compatible with the other high level api's I'm wanting to use here.

@dpgeorge
Copy link
Member

it looks like the interrupt handler reads the internal fifo, fires off this callback if configured, then pushes the data into the rx buffer.

What might work is to use mp_sched_schedule() in this callback to schedule another function which will check the UART buffer, and then process ctrl-C.

@andrewleech
Copy link
Contributor Author

What might work is to use mp_sched_schedule() in this callback to schedule another function which will check the UART buffer, and then process ctrl-C.

Yeah I'd kind of thought that, it felt like it was just adding even more layers of abstraction / background tasks to pump chars from one buffer to another - but yeah it's better than a separate freertos task for sure.

I can't just replicate their ISR and keep the high level api - far too many static types / variables / functions for that to work.

I can't help but think it's better to go mostly back to what's being used currently - a very thin / light driver, just one that's using some of their slightly heigher level init functions to hopefully make it behave properly without any chip-specific coded needed in the micropython interface. There's just so much code in their uart driver, mostly to support all the features / events / callbacks which wer're not using, like the semaphors and queues for their own buffer systems which we basically duplicate anyway with ringbuf.

@dpgeorge
Copy link
Member

I can't help but think it's better to go mostly back to what's being used currently - a very thin / light driver,

Yes that does sound good.... do we know why that approach doesn't work with newer IDF versions?

@andrewleech
Copy link
Contributor Author

Yes that does sound good.... do we know why that approach doesn't work with newer IDF versions?

no, but the existing driver never runs any of the uart "init" functions, it goes direct to a lower level chips specific uart_tx_one_char() function in mphalport which I suspect could be part of the problem

@andrewleech andrewleech changed the title esp32/uart: Use high level IDF uart driver for repl. esp32/uart: Correctly init uart driver for repl. Feb 24, 2022
@andrewleech
Copy link
Contributor Author

Well that's a much much smaller changeset, will need more info / hardware to test if it resolves the bootloop / newer IDF issue though.

@andrewleech andrewleech mentioned this pull request Feb 24, 2022
Closed
@andrewleech
Copy link
Contributor Author

Ah, but now it's back to an earlier issue I had - if mpy tries to tx too much data back to pc on repl it locks up

@andrewleech andrewleech force-pushed the esp32_uart_driver branch from debf9d1 to ad484ee Compare February 24, 2022 04:49 8000
@andrewleech
Copy link
Contributor Author

repl stdout now fixed, this should be ready for review @dpgeorge

DvdGiessen
DvdGiessen reviewed Feb 24, 2022
View reviewed changes
ports/esp32/uart.c Outdated
break;
}
str += ret;
mp_hal_delay_ms(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

uart_repl_write is called by mp_hal_stdout_tx_strn, which may release and re-acquire the GIL if we are echoing more than 20 characters.

Thus, it is not safe to call mp_hall_delay_ms here, because that function also releases and re-acquires the GIL, thus causing deadlock when mp_hal_stdout_tx_strn tries to acquire the GIL.

Fixed the issue locally by replacing this with ulTaskNotifyTake(pdFALSE, 1); instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's a great catch thanks, I'll update it tomorrow.

@andrewleech andrewleech force-pushed the esp32_uart_driver branch 3 times, most recently from 1591b9e to 4ec8210 Compare February 25, 2022 00:17
dpgeorge
dpgeorge reviewed Feb 25, 2022
View reviewed changes
ports/esp32/mphalport.c Outdated
for (uint32_t i = 0; i < len; ++i) {
uart_tx_one_char(str[i]);
}
uart_repl_write(str, len);
Copy link
Member

Choose a reason for hiding this comment

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

I'd call this function uart_tx_strn() to match the others here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was actually thinking of renaming the others to be honest, they're named like a generic uart function but in actuality they're hardcoded to repl / uart0 use

Copy link
Member

Choose a reason for hiding this comment

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

I meant to match usb_tx_strn and usb_serial_jtag_tx_strn (which are generic and not tied to the REPL).

Or how about uart_stdout_tx_strn()? Or uart_tx_strn(MICROPY_HW_UART_REPL, ...)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes I follow now, I've just updated as such

ports/esp32/mphalport.c Outdated
9E88
@@ -54,6 +44,8 @@
#include "mphalport.h"
#include "usb.h"
#include "usb_serial_jtag.h"
#include "uart.h"
#include "driver/uart.h"
Copy link
Member

Choose a reason for hiding this comment

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

Does it need driver/uart.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah probably not anymore, since wrapping the uart write function

ports/esp32/uart.c Outdated
.flow_ctrl = UART_HW_FLOWCTRL_DISABLE,
.rx_flow_ctrl_thresh = 0
};
uart_param_config(UART_NUM_0, &uartcfg);
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth making UART_NUM_0 a #define macro as well, eg MICROPY_HW_UART_REPL would match stm32.

ports/esp32/uart.c
};
uart_param_config(UART_NUM_0, &uartcfg);

const uint32_t rxbuf = 129; // IDF requires > 128 min
Copy link
Member

Choose a reason for hiding this comment

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

As pert stm32, maybe macro this 129 to MICROPY_HW_UART_REPL_RXBUF?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This rxbuf is not used ( wasted ram :-( ) it needs their high level API ISR to work, but there's an assert to ensure it's defined.

Copy link
Member

Choose a reason for hiding this comment

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

OK

@dpgeorge
Copy link
Member
dpgeorge commented Mar 1, 2022

Thanks for updating, merged in 2cc9232

@dpgeorge dpgeorge closed this Mar 1, 2022
@andrewleech andrewleech mentioned this pull request Mar 1, 2022
Closed
@andrewleech andrewleech deleted the esp32_uart_driver branch March 1, 2022 10:37
tannewt added a commit to tannewt/circuitpython that referenced this pull request Aug 24, 2023
@tannewt
Merge pull request micropython#8279 from dglaude/8.2.x-backport-picopad
f353fb1 
8.2.x backport picopad by @lynt-smitka
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DvdGiessen DvdGiessen DvdGiessen left review comments

@dpgeorge dpgeorge dpgeorge left review comments

Assignees
No one assigned
Labels
port-esp32
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@andrewleech @dpgeorge @DvdGiessen @pi-anl
0