8000 Shared uart isr by Makuna · Pull Request #5600 · esp8266/Arduino · GitHub
[go: up one dir, main page]
Skip to content

Shared uart isr #5600

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

Open
wants to merge 11 commits into
base: master
Choose a base branch
from
Open

Shared uart isr #5600

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
2e9a1c0
First Compile
Makuna Jan 7, 2019
5bc2ea5
subscribe and unsubscribe consistent
Makuna Jan 7, 2019
ae7903d
Apply review feeedback
Makuna Jan 8, 2019
6b87398
Merge branch 'master' into SharedUartISR
Makuna Jan 8, 2019
6564edb
static initialize fixed
Makuna Jan 11, 2019
285b7f5
Merge branch 'master' into SharedUartISR
Makuna Jan 11, 2019
c96209d
better function name
Makuna Jan 20, 2019
2e3ca18
Merge branch 'master' into SharedUartISR
Makuna Jan 20, 2019
d6de293
Merge branch 'master' into SharedUartISR
Makuna Jan 20, 2019
ab03100
Merge branch 'master' into SharedUartISR
Makuna Jan 20, 2019
b0df4d0
Merge branch 'master' into SharedUartISR
Makuna Jan 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Apply review feeedback
  • Loading branch information
@Makuna
Makuna committed Jan 8, 2019
commit ae7903dfb4484770f59415a6eb6cfa726bd648cd
15 changes: 11 additions & 4 deletions cores/esp8266/uart.c
View file
8000 Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,10 @@ struct uartIsrContext_t

// NOTE: GCC will generated an invalid warning for the following line
// see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
#pragma GCC diagnostic push
#pragma GCC diagnostic ignored "-Wmissing-braces"
static volatile struct uartIsrContext_t s_uartInterruptContext[2] = { 0 };
#pragma GCC diagnostic pop

/*
In the context of the naming conventions in this file, "_unsafe" means two things:
Expand Down Expand Up @@ -343,11 +346,13 @@ uart_isrDefault(void * arg)
}

void
uart_subscribeInterrupt(int uart_nr, uartInterruptHandler callback, void* param)
uart_subscribeInterrupt_unsafe(int uart_nr, uartInterruptHandler callback, void* param)
{
s_uartInterruptContext[uart_nr].callback = callback;
s_uartInterruptContext[uart_nr].arg = param;

// check if we are already attached to the interrupt for
// the other uart and only attach if not
int other_nr = (uart_nr + 1) % 2;
if (s_uartInterruptContext[other_nr].callback == NULL)
{
Expand All @@ -356,7 +361,7 @@ uart_subscribeInterrupt(int uart_nr, uartInterruptHandler callback, void* param)
}

bool
uart_unsubscribeInterrupt(int uart_nr, uartInterruptHandler callback)
uart_unsubscribeInterrupt_unsafe(int uart_nr, uartInterruptHandler callback)
{
if (s_uartInterruptContext[uart_nr].callback == callback)
{
Expand All @@ -368,6 +373,8 @@ uart_unsubscribeInterrupt(int uart_nr, uartInterruptHandler callback)
s_uartInterruptContext[uart_nr].callback = NULL;
s_uartInterruptContext[uart_nr].arg = NULL;

// check if we are also attached to the interrupt for
// the other uart and only deattach if not
int other_nr = (uart_nr + 1) % 2;
if (s_uartInterruptContext[other_nr].callback == NULL)
{
Expand Down Expand Up @@ -410,7 +417,7 @@ uart_start_isr(uart_t* uart)
// UIPE: parity error
// UITO: rx fifo timeout
USIE(uart->uart_nr) = (1 << UIFF) | (1 << UIOF) | (1 << UIFR) | (1 << UIPE) | (1 << UITO);
uart_subscribeInterrupt(uart->uart_nr, uart_isrDefault, (void *)uart);
uart_subscribeInterrupt_unsafe(uart->uart_nr, uart_isrDefault, (void *)uart);

}

Expand Down Expand Up @@ -651,7 +658,7 @@ uart_uninit(uart_t* uart)
if (uart->rx_enabled)
{
ETS_UART_INTR_DISABLE();
if (uart_unsubscribeInterrupt(uart->uart_nr, uart_isrDefault))
if (uart_unsubscribeInterrupt_unsafe(uart->uart_nr, uart_isrDefault))
{
ETS_UART_INTR_ENABLE();
}
Expand Down
4 changes: 2 additions & 2 deletions cores/esp8266/uart.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,10 +152,10 @@ extern "C" {
// uart_subscribeInterrupt & uart_unsubscribeInterrupt are not safe and must
// be called within ETS_UART_INTR_DISABLE()/ETS_UART_INTR_ENABLE() protection
//
Copy link
Collaborator

Choose a reason for hiding this comment

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

Other unsafe functions have _unsafe_ part of their name.
I think that is a good reminder for their users.
Or, why not calling INTR_EN/DISABLE() from inside these functions and remove that warning ?

Copy link
Collaborator Author
@Makuna Makuna Jan 8, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

If you glance at the use of subscribe inside uart.c, you will notice that a bunch of other stuff that needs to be done inside the INTR_EN/DISABLE sandwich. These other operations are similar but different from each use case. While unsubscribe does not; I decided to keep them consistent rather than only one require the sandwich. Keeping a balanced API.

I really don't like either direction (what I have and what you suggested). I am very open to other suggestions.

These are exposed, do we really want to mark them unsafe? While its self documenting, it seems inconsistent with any other API.

Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Rename them _unsafe, add new ones calling the unsafe version in a INTR_EN/DISABLE sandwhich

  • too bad it's C, otherwise a parameter bool protect with a default value would do the job

  • keep as it is, and let's rely on users' intelligence for reading doc and comments

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please implement like bullet 1 above. The unsafe word in the function name is meant to indicate it's not interrupt-safe and it shouldn't be exported, if at all possible.
The unsafe functions are meant for resuse from other unsafe functions and/or for sandwich wrapping into safe versions.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not quite resolved. I'll try to explain again.
I said:

The unsafe word in the function name is meant to indicate it's not interrupt-safe and it shouldn't be exported

In this case, there could be undefined behavior if there is an IRQ in the middle of calling these functions.
Instead, mark the current implementations of these functions as static, and implement new functions without the _unsafe() word. Then export those in the .h. These exported versions call the _unsafe() functions inside a sandwich.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, it is ok to skip argument sanity checks in the _unsafe functions, but the safe versions should implement checks on the received args.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@devyte Maybe I was not clear enough. You can't wrap the subscribe as the site of where you call it has to be already wrapped with it due to several processes of setting up the uart hardware are required to be called within the same DISABLE/ENABLE. This is what I was pointing out by referencing the call spots withing UART.C. This same setup; but different based on needs of the caller is done externally by anyone needing to use uart ISR.
IF DISABLE/ENABLE were actually ref-count like entities, then it would be a good practice, as the ENABLE inside would only ref-count and not truly enable; but this is not the case.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean these unsafe functions need to be called by the user?
I think we need an example given this discussion, but something simple, e.g.: open a file and print it out to serial asynchronously a chunk at a time, or something along those lines. I think it would help to illustrate the usage, and how a user would interact with the functionality.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

YES, these two unsafe functions are called by the user. I pointed to my project that consumes these in the pull comments at the top.

Can I just point them to my code? Ok, its not simple ;-)

The real use of this is where you aren't doing simple serial, otherwise you would just use Serial. In general, using the hardware to talk to devices that take a serial stream; often where translation is done from data on the MCU as you send. Which means configuring the UART to your custom needs (using all those esoteric features to invert, turn off stop bits, etc) that are not exposed by Serial or even the current uart.h.

Copy link
Collaborator Author
@Makuna Makuna Jan 20, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

Here is a link to my code that does this specifically, copied below
https://github.com/Makuna/NeoPixelBus/blob/959cfac07b97e95e10fc71f311f81fa3e2757895/src/internal/NeoEsp8266UartMethod.cpp#L75

void NeoEsp8266UartInterruptContext::Attach()  {
    // Disable all interrupts
    ETS_UART_INTR_DISABLE();
    // Clear the RX & TX FIFOS
    const uint32_t fifoResetFlags = (1 << UCTXRST) | (1 << UCRXRST);
    USC0(_uartNum) |= fifoResetFlags;
    USC0(_uartNum) &= ~(fifoResetFlags);
    uart_subscribeInterrupt_unsafe(_uartNum, Isr, this);
    // Set tx fifo trigger. 80 bytes gives us 200 microsecs to refill the FIFO
    USC1(_uartNum) = (80 << UCFET);
    // Disable RX & TX interrupts. It maybe still enabled by uart.c in the SDK
    USIE(_uartNum) &= ~((1 << UIFF) | (1 << UIFE));
    // Clear all pending interrupts in UART1
    USIC(_uartNum) = 0xffff;
    // Reenable interrupts
    ETS_UART_INTR_ENABLE();
}

void uart_subscribeInterrupt(int uart_nr, uartInterruptHandler callback, void* param);
void uart_subscribeInterrupt_unsafe(int uart_nr, uartInterruptHandler callback, void* param);
// if uart_unsubscribeInterrupt returns false, then ETS_UART_INTR_ENABLE() doesn't
// need to be called after it
bool uart_unsubscribeInterrupt(int uart_nr, uartInterruptHandler callback);
bool uart_unsubscribeInterrupt_unsafe(int uart_nr, uartInterruptHandler callback);

#if defined (__cplusplus)
} // extern "C"
Expand Down
0