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
subscribe and unsubscribe consistent 
Since subscribe can not be automatically disable and enable interrupts due to other external uart setup must be done at the same time within the disabled interrupts, then unsubscribe should be made consistent
  • Loading branch information
@Makuna
Makuna committed Jan 7, 2019
commit 5bc2ea5e82d53c96d2c2e81381fdbb272f651dff
30 changes: 16 additions & 14 deletions cores/esp8266/uart.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,13 +72,15 @@ struct uart_
struct uart_rx_buffer_ * rx_buffer;
};

struct uartIntContext_t
struct uartIsrContext_t
{
uartInterruptHandler callback;
void* arg;
};

static volatile struct uartIntContext_t s_uartInterruptContext[2];
// NOTE: GCC will generated an invalid warning for the following line
// see https://gcc.gnu.org/bugzilla/show_bug.cgi?id=53119
static volatile struct uartIsrContext_t s_uartInterruptContext[2] = { 0 };

/*
In the context of the naming conventions in this file, "_unsafe" means two things:
Expand All @@ -91,16 +93,13 @@ static volatile struct uartIntContext_t s_uartInterruptContext[2];
wrap the unsafe ones with disabling/enabling of the uart interrupt for safe public use.
*/



// called by ISR
inline size_t ICACHE_RAM_ATTR
uart_rx_fifo_available(const int uart_nr)
{
return (USS(uart_nr) >> USRXC) & 0xFF;
}


/**********************************************************/
/************ UNSAFE FUNCTIONS ****************************/
/**********************************************************/
Expand Down Expand Up @@ -312,7 +311,8 @@ uart_isr(void* arg)
}
else
{
// Clear all interrupts flags (just in case)
// No registered handler, so
// clear all interrupts flags (just in case)
USIE(uartNum) = 0;
USIC(uartNum) = 0xffff;
}
Expand Down Expand Up @@ -355,11 +355,9 @@ uart_subscribeInterrupt(int uart_nr, uartInterruptHandler callback, void* param)
}
}

void
bool
uart_unsubscribeInterrupt(int uart_nr, uartInterruptHandler callback)
{
ETS_UART_INTR_DISABLE();

if (s_uartInterruptContext[uart_nr].callback == callback)
{
// turn off uart
Expand All @@ -376,12 +374,13 @@ uart_unsubscribeInterrupt(int uart_nr, uartInterruptHandler callback)
// detach our ISR
ETS_UART_INTR_ATTACH(NULL, NULL);

// return so we don't enable interrupts since there is no ISR anymore
return;
// return false so we don't enable interrupts
// since there is no need for the ISR anymore
return false;
}
}

ETS_UART_INTR_ENABLE();
return true;
}


Expand Down Expand Up @@ -485,7 +484,6 @@ uart_wait_tx_empty(uart_t* uart)

while (uart_tx_fifo_available(uart->uart_nr) > 0)
delay(0);

}

void
Expand Down Expand Up @@ -652,7 +650,11 @@ uart_uninit(uart_t* uart)

if (uart->rx_enabled)
{
uart_unsubscribeInterrupt(uart->uart_nr, uart_isrDefault);
ETS_UART_INTR_DISABLE();
if (uart_unsubscribeInterrupt(uart->uart_nr, uart_isrDefault))
{
ETS_UART_INTR_ENABLE();
}
free(uart->rx_buffer->buffer);
free(uart->rx_buffer);
}
Expand Down
9 changes: 6 additions & 3 deletions cores/esp8266/uart.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,10 +149,13 @@ extern "C" {
void uart_start_detect_baudrate(int uart_nr);
int uart_detect_baudrate(int uart_nr);

// attach MUST be called within a
// ETS_UART_INTR_DISABLE()/ETS_UART_INTR_ENABLE() sandwich
// 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 comm D7AE ent

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_unsubscribeInterrupt(int uart_nr, uartInterruptHandler callback);
// 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);

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