Shared uart isr #5600

Makuna · 2019-01-07T23:42:42Z

Notes:
Sorry about the small format changes, but my editor when swapping tabs to spaces also does standard formating, like putting spaces after if and the like.
NeoPixelBus has a branch created that uses these changes
https://github.com/Makuna/NeoPixelBus/tree/SharedUartIsr

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

d-a-v · 2019-01-08T00:48:18Z

cores/esp8266/uart.h

+
+    // uart_subscribeInterrupt & uart_unsubscribeInterrupt are not safe and must
+    // be called within ETS_UART_INTR_DISABLE()/ETS_UART_INTR_ENABLE() protection
+    //


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 ?

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.

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

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.

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.

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

@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.

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.

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.

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(); }

cores/esp8266/uart.c

devyte · 2019-01-12T03:56:26Z

cores/esp8266/uart.c

+    uart_t* uart = (uart_t*)arg;
+    uint32_t usis = USIS(uart->uart_nr);
+
+    if (usis & (1 << UIFF))


The sanity check if(uart == NULL || !uart->rx_enabled) is missing. It's supposed to guard against the ISR triggering by mistake, and in that case turn the interrupt off and keep going. I supposed it should really be an assert, because I think if it triggers by mistake it's really a programming error. Whether the original check or an assert, or something else, I think some sanity check here would be a good idea.

We have ASSERTs that turn off in a normal build?
I would agree with an ASSERT, but I disagree otherwise. This is an internal handler and not exposed, and all locations include an object. Param checks should be reserved for the outermost APIs. If hit such a check, then there is corruption.

As I said, some sanity check. An assert is ok by me, as long as it's clear what it checks (i.e.: include comments).

BTW, the original check here was invalid. It would have dereferenced the null on lines above this check, this is why it was removed.
But let me be more specific in my request above. WHAT is the "assert" method to use? If I use "assert" I get a compile error of "undefined reference to `assert'".

devyte · 2019-01-12T04:07:38Z

cores/esp8266/uart.c

+    int other_nr = (uart_nr + 1) % 2;
+    if (s_uartInterruptContext[other_nr].callback == NULL)
+    {
+        ETS_UART_INTR_ATTACH(uart_isr, s_uartInterruptContext);


In uart_start_isr(), the current code calls ETS_UART_INTR_ATTACH() and then ETS_UART_INTR_ENABLE() right after. This new code does not.

See line 619, this is where it is enabled.

Ah, I had missed the significance of that, the disable-enable sandwich is moved there.
In that case, it seems to me that the semantics of uart_start_isr() changes, given that the isr isn't really started. I suggest a a change in the function name, and a comment for the function explaining the new behavior (i.e.: inits this, prepares that, blah, does not enable the interrupt yet, etc)

renamed to uart_init_default_isr

devyte · 2019-01-12T04:15:05Z

cores/esp8266/uart.h

+
+    // uart_subscribeInterrupt & uart_unsubscribeInterrupt are not safe and must
+    // be called within ETS_UART_INTR_DISABLE()/ETS_UART_INTR_ENABLE() protection
+    //


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.

devyte · 2019-01-23T20:58:29Z

@Makuna can you please take another look at this now? There are conflicts that need to be resolved, and some of your code could need adapting.
BTW, the conflicts are due to #5559 , which adds support for debugging with gdb while sharing the hw uart with the app. I'm not sure how/if that impacts the idea here.

Makuna · 2019-01-27T22:21:15Z

I will need to rewrite this some to address all the changes made on the other pull. Ignore this for now and once I create the new one I will close this.

earlephilhower · 2019-10-01T20:16:28Z

Thanks for your PR, but the core and libraries have changed enough that this PR now has a merge conflict.

Could you merge it manually with the latest core, so we can consider it for future releases?

Makuna · 2019-10-02T01:31:10Z

Last time I looked, the changes that were taken made my changes/solution IMPOSSIBLE. I really felt the debug stuff should not have gone in, it had inverted API use at the minimum and lots of poor assumptions. I felt rather put down since we had two pushes BEFORE it and we were the ones who had to react; TWO TIMES already.

Was that fixed?

devyte · 2019-10-02T03:52:25Z

@Makuna I don't remember the status anymore, I got sidetracked too many times with stability pursuits. Can you please handle this so that we can look at it? We're currently trying to recover proposals that got put aside for various reasons.
If this is dependent on something else, please explain.

Makuna added 2 commits January 6, 2019 23:30

First Compile

2e9a1c0

subscribe and unsubscribe consistent

5bc2ea5

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

Makuna requested review from igrr, d-a-v and me-no-dev January 7, 2019 23:42

d-a-v reviewed Jan 8, 2019

View reviewed changes

cores/esp8266/uart.c Show resolved Hide resolved

devyte self-requested a review January 8, 2019 02:15

Makuna commented Jan 8, 2019

View reviewed changes

cores/esp8266/uart.c Outdated Show resolved Hide resolved

Makuna added 2 commits January 7, 2019 22:39

Apply review feeedback

ae7903d

Merge branch 'master' into SharedUartISR

6b87398

earlephilhower reviewed Jan 9, 2019

View reviewed changes

cores/esp8266/uart.c Outdated Show resolved Hide resolved

Makuna added 2 commits January 11, 2019 13:23

static initialize fixed

6564edb

Merge branch 'master' into SharedUartISR

285b7f5

d-a-v approved these changes Jan 12, 2019

View reviewed changes

devyte requested changes Jan 12, 2019

View reviewed changes

Makuna added 5 commits January 20, 2019 12:23

better function name

c96209d

Merge branch 'master' into SharedUartISR

2e3ca18

Merge branch 'master' into SharedUartISR

d6de293

Merge branch 'master' into SharedUartISR

ab03100

Merge branch 'master' into SharedUartISR

b0df4d0

d-a-v added the waiting for feedback Waiting on additional info. If it's not received, the issue may be closed. label May 25, 2019

earlephilhower added the merge-conflict PR has a merge conflict that needs manual correction label Oct 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shared uart isr #5600

Shared uart isr #5600

Makuna commented Jan 7, 2019 •

edited

Loading

d-a-v Jan 8, 2019

Makuna Jan 8, 2019 •

edited

Loading

d-a-v Jan 8, 2019

devyte Jan 8, 2019

devyte Jan 12, 2019

devyte Jan 12, 2019

Makuna Jan 12, 2019

devyte Jan 15, 2019

Makuna Jan 15, 2019

Makuna Jan 20, 2019 •

edited

Loading

devyte Jan 12, 2019

Makuna Jan 12, 2019

devyte Jan 15, 2019

Makuna Jan 20, 2019 •

edited

Loading

devyte Jan 12, 2019

Makuna Jan 12, 2019

devyte Jan 15, 2019

Makuna Jan 20, 2019

devyte Jan 12, 2019

devyte commented Jan 23, 2019

Makuna commented Jan 27, 2019

earlephilhower commented Oct 1, 2019

Makuna commented Oct 2, 2019

devyte commented Oct 2, 2019

Shared uart isr #5600

Are you sure you want to change the base?

Shared uart isr #5600

Conversation

Makuna commented Jan 7, 2019 • edited Loading

Choose a reason for hiding this comment

Makuna Jan 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Makuna Jan 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Makuna Jan 20, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

devyte commented Jan 23, 2019

Makuna commented Jan 27, 2019

earlephilhower commented Oct 1, 2019

Makuna commented Oct 2, 2019

devyte commented Oct 2, 2019

Makuna commented Jan 7, 2019 •

edited

Loading

Makuna Jan 8, 2019 •

edited

Loading

Makuna Jan 20, 2019 •

edited

Loading

Makuna Jan 20, 2019 •

edited

Loading