Enable interrupt mode of UART tx

[descampsa]

Current version of Serial1.write will never use the interrupt mode transmission, because it fails to check if the holding register (fifo) is empty. It only check if ring buffer is empty, and use it only if it is not empty, which is a classic chicken and egg problem.
This commit fix that , and several (maybe not all?) bugs related to interrupt mode serial transmission.

This sketch demonstrate the problem, the write time is much lower after the change:
serial4_error.ino.txt

The test sketch Serial1_error.ino.txt pass, but a regression in some edge case is not impossible, interrupt mode is more tricky than poll mode. Feel free to comment, test, and try to break it.

Do not merge yet, this is not complete, i will also change the interrupt handler to send several bytes at a time, to exploit tx fifo and reduce interrupt overhead.
Done, the tx fifo should be used correctly now.

[descampsa]

Maybe this is a nitpick, but i have a doubt about my current implementation of the irq handler.

The first thing i do in the irq handler is to check the IIR register to know the source of interrupt (line status, received data, etc).
The IIR register should be read only once, to avoid any race condition, and because reading the IIR register reset the Receiver Line Status interrupt.

How this is currently accomplished is by calling first uart_irq_update, that only force a read on the IIR, to update its cached value, and then functions like uart_irq_rx_ready, that perform the actual test, without forcing the value to update, and thus probably use the cached value.
The problem here is the 'probably' : nothing guarantee that the compiler will not decide to actually read the register in these functions if the code or compilation option change, leading to different behaviour, and maybe subttle bug.

In my opinion, it would be better to read the register in a switch/case, that would guarantee that we read it only once, and would also result in a code easier to understand. However, the existing functions in 16550.c seem to be designed for the current implementation (and the switch/case implementation would require to add another function), so maybe i am missing some advantage of this method?

[eriknyquist]

@descampsa sorry I took so long to respond on this, I was working on something else. Thanks for the fix, I'll take a look and do a bit of testing here.

[eriknyquist]

Why is TEMT not sufficient? With our UART configuration it should be set when both THR and FIFO are empty, right?

[eriknyquist]

Nevermind, you're right, TEMT is set when THR and TSR are empty. Since uart_poll_out only places a byte in THR, then THRE is sufficient.

[descampsa]

Hey @eriknyquist, have you had the time to test it further?
What do you think about my nitpick?

[eriknyquist]

in regards to your efforts to read the IIR register only once, I have no idea. Can you update the pull request once you've figured it out? Your first comment kind of gave me the idea that this isn't finished yet. So I thought I'd wait for you to finish it.

[descampsa]

You can forget my comment about the IIR register, i just noticed that i misread the code and did not see that it was using a separate cache variable (iirCache).
You may review the pull request, it is finished for me.

[eriknyquist]

@descampsa trying to test but there are conflicts, can you resolve them please, and you also need to re-build libarc32_arduino101.a and add it to the commit (since you changed files in the system/ directory).

[descampsa]

@eriknyquist Ok, it should be good now.

[eriknyquist]

@descampsa thanks

[eriknyquist]

So, I can see a difference with short strings, but longer strings still seem to block for most of the transfer:

before applying this patch:

Time to print "000": 5ms

Time to print "0000000000000000000000": 25ms

after applying this patch:

Time to print "000": 0ms

Time to print "0000000000000000000000": 23ms

[descampsa]

Strange, with this code : test_serial_tx.txt, i get:

Before:
Time to write "OOO" : 18 ms Time to write "0000000000000000000000" : 181 ms

After:
Time to write "OOO" : 0 ms Time to write "0000000000000000000000" : 0 ms

[eriknyquist]

You're timing Serial.write(). I was timing Serial.print() since that one is more commonly used.

[descampsa]

Hmm, yeah, that could have been that, but i still can't reproduce it with print.
I tried the raw data and the String versions, but still the same result.

Could you share the complete code you are using?
Do you see a difference between print and write on your side?

btw, it is probably a typo, but just to be sure, you mean Serial1.print(), not Serial.print(), right?

[eriknyquist]

Yes, sorry, I did mean Serial1.print. I have attached the sketch I am using.

get_avg_printing_time.txt

[descampsa]

Oh, i see, this behaviour is expected, because you exceed the uart buffer size (64 byte) by printing a 22 bytes string 10 times.
Try to increase the bufffer size (UART_BUFFER_SIZE) to 512, and you will see that there will be no blocking anymore.
You may also try with a delay between each iteration, or a call to Serial1.flush().

[eriknyquist]

Ah OK, yes I see. OK, can you squash your 4 commits into 1 commit please? I think all these changes belong together, and besides, no commits before 175319e will compile

[descampsa]

Yes of course, commits squashed.

[descampsa]

Hey @sgbihu, i see that you have reverted this change in ade23f0 .

Do you know why exactly it breaks the BLE communication?

[sgbihu]

Please see the below commits that I try to fix it in my branch.

sgbihu@faf41cb
sgbihu@9639f7f