Skip to content
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

Interrupts on UART2 (Serial console) on Nucleo-F401 #2119

Closed
hesolium opened this issue Jul 7, 2016 · 10 comments
Closed

Interrupts on UART2 (Serial console) on Nucleo-F401 #2119

hesolium opened this issue Jul 7, 2016 · 10 comments

Comments

@hesolium
Copy link

hesolium commented Jul 7, 2016

I have old program running on Nucleo-F401 and Nucleo-L152. All communication with the computer is performed through the serial (UART2) using interrupts. For transmit I use simple ISR:

void Console::writeService() {
    while(console.writeable()) {
        short c = consoleOut.char2send();
        if(c < 0)
            break;
        console.putc(c);
    }
}

I have a problem with the latest (MBED-DEV) library on Nucleo-F401 (on L152 is OK).
After execution command: console.attach(this, &Console::writeService, Serial::TxIrq);
the application runs very slowly (100x). I took advantage of gdb and I noticed that the interrupt handler (uart_irq in module serial_api.c) is executed very often. Neither the application nor the computer does not send any characters. It seems that this low level ISR takes 99% CPU power for no reason.

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 7, 2016

cc @bcostm @adustm @LMESTM

@hesolium
Copy link
Author

hesolium commented Jul 7, 2016

I found differences between old and new library.
Old library uses: __HAL_UART_ENABLE_IT(handle, UART_IT_TC);
new one uses: __HAL_UART_ENABLE_IT(handle, UART_IT_TXE);

I read docs and think that should be UART_IT_TC.
UART_IT_TC flag signals end of transmission (and buffer is empty of course:)
UART_IT_TXE flags signals transmition buffer is empty

If there no data to transmit then buffer is empty all the time and ISR is called constantly. Its useless.

@LMESTM
Copy link
Contributor

LMESTM commented Jul 7, 2016

Hi hesolium - thanks for highlighting this issue!

Actually both interrupts make sense and I think that we could end up with both being enabled.

It's good to know when there is room in the buffer to send next byte without waiting further (speed up transmission). But it also makes sense to know when the transmission is complete (I've been discussing that point here: #1801 but we could not find an agreement yet on how to solve this in mbed, I think an API change might be needed). All in all, as you can read in previous link, the trend in MBED is so far to rather try to send data as fast as possible, so we will keep UART_IT_TXE usage for now. .

It's good to know when there is room in the buffer to send next byte without waiting further (speed up transmission). But it also makes sense to know when the transmission is complete (I've been discussing that point here: #1801 but we could not find an agreement yet on how to solve this in mbed, I think an API change might be needed). All in all, as you can read in previous link, the trend in MBED is so far to rather try to send data as fast as possible, so we will keep UART_IT_TXE usage for now. .

But there is indeed a problem in the library (in F4 serial_api.c) - if we enable the TXE interrupt, then we need to clear the flag as well. This is not done now, so I guess this is the reason for the bug you see.

I'll verify this asap and keep you posted.
If confirmed I'll try to add
if (__HAL_UART_GET_FLAG(handle, UART_FLAG_TXE) != RESET) {
irq_handler(serial_irq_ids[id], TxIrq);
__HAL_UART_CLEAR_FLAG(handle, UART_FLAG_TXE);
}
in uart_irq function. you may want to try it out as well

@LMESTM
Copy link
Contributor

LMESTM commented Jul 7, 2016

Hi again -
I was a bit quick - the proposed fix in previous comment won't work - so no need to try it out.
I'll need more time for a proper handling of both interrupts, in the meanwhile using _TC interrupt instead will work ok.

@hesolium
Copy link
Author

hesolium commented Jul 7, 2016

@LMSTM:"It's good to know when there is room in the buffer to send next byte without waiting further (speed up transmission). "

You are right, but on the other hand, there is a large overhead on the CPU, because after the transmission of each character, UART fire interrupt that must be serviced. If only EOT (and empty buffer) is signaled then we can service interrupt only once per buffer length block character (16/32 bytes). And, additionaly, we need not enable/disable UARTs interrupts. I setup UART interrupts only once (in Serial initialization routine).

@hesolium
Copy link
Author

hesolium commented Jul 7, 2016

@LMSTM:"All in all, as you can read in previous link, the trend in MBED is so far to rather try to send data as fast as possible"

Maybe API should be extended. I see (in the source code of serial_api.c) that using DMA for transmit/receive is possible. I think it will be better direction to develop Serial class for speed-up transfers.

@0xc0170
Copy link
Contributor

0xc0170 commented Jul 8, 2016

You are right, but on the other hand, there is a large overhead on the CPU, because after the transmission of each character, UART fire interrupt that must be serviced. If only EOT (and empty buffer) is signaled then we can service interrupt only once per buffer length block character (16/32 bytes). And, additionaly, we need not enable/disable UARTs interrupts. I setup UART interrupts only once (in Serial initialization routine).

Serial API provides 2 methods - one character (attach()) and multiple characters (transfer()).

I think it will be better direction to develop Serial class for speed-up transfers.

Be more specific?

@LMESTM
Copy link
Contributor

LMESTM commented Jul 8, 2016

@hesolium fully agree with you.
I will send a PR to fix the wrong IT enabling.
In case of async API you will gain in performances - note that the async implementation takes benefit of the TXE and manages the enabling / disabling of the interrupt.

LMESTM pushed a commit to LMESTM/mbed that referenced this issue Jul 8, 2016
Reported in Issue ARMmbed#2119

There was some inconsistency in serial interrupt handling accross STM32
serial_api.c implementation. In case application wants to be notified of
Tx interrupt, it is mainly interested in transmission complete information,
which is the _TC interrupt.

The _TXE (Transmit Data REgister Empty) is used only within driver
in case SERIAL_ASYNCH is being supported to make the transmission
more efficient.
@hesolium
Copy link
Author

hesolium commented Jul 8, 2016

@0xc0170, @LMESTM :"Be more specific?"

You (and LMESTM) write about the performance of async API.
If someone uses this API (and interrupt) for data transfer, this means that speed (and performance) is not the most important factor. Much better results can be obtained using DMA (transfer method). The primary reason for using async API with interrupts is simplicity and portability. Large parts of my programs for MBED platform I test on Linux and therefore I do not use the advanced parts of the API library inside program logic. Speed of async UART is sufficient (for me:).

In this particular case (change to TXE flags) causes more complicated code. With the previous version of the library (TC flag) it was enough to setup interrupts (at program start) and initiate transmission on each blok of data.
Now, before transmitting, programmer have to additionally enable interrupt.
This is specific to MBED library piece of code inside (clear C++) program logic.
And to make matters worse, during the transfer you need to monitor end of transmission and disable interrupt. If this is not done, then uart_irq kill processor.

For me, more important than performance is the simplicity (and elegance:) of the program.

@LMESTM
Copy link
Contributor

LMESTM commented Jul 8, 2016

@hesolium - thanks for describing your usage of the firmware.
I posted a PR #2128 - this actually fixes the reported issue for F4 but also solves inconsistency for other families.

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

No branches or pull requests

3 participants