IRQ driven UART0 driver using uart_regs.h

[kanflo]

I have updated the UART driver to use the e-o-r registers rather than the ones from the SDK.

[projectgus]

Thanks @kanflo.

I pulled this down but I am a little concerned about the weak linking operation. If I compile the 'terminal' example at the moment, I don't actually get the driver linked in (verified in build/terminal.map).

For the linker to include a specific object file, it has to search for a symbol from that object's source file. If the linker has found a weak symbol already, it won't automatically search for a strong symbol of the same name. The linker is only guaranteed to link the strong symbol if it loads the object file with the strong symbol incidentally, while resolving a different symbol. (That's my understanding anyhow, there's probably a clearer and more correct explanation online somewhere.)

So, with the current code, you have to call uart0_num_char() if you want to be guaranteed of getting the driver - as calling into that function will link serial-driver.o.

Probably the "right" way to fix that problem is to expose the uart_rx_init() function (even if it's a no-op) and require people to call that from their program somewhere. Implement that in the same object file as the _read_r replacement, and the linker will pull in _read_r when it pulls in the init function. But, as we discussed, it's unclear where the best place to call the init function is.

Aside from that dilemma, I have another question on a totally different note. Do you have any thoughts about the hardware FIFO? If there really is a 128 byte UART FIFO in hardware, it seems like we may not need an 80 byte ringbuffer in RAM...?

[projectgus]

There is another maybe-hacky way around the weak linking problem, which is to modify LDFLAGS in the extras/serial-driver/component.mk to add a '-u' argument that adds an undefined symbol that happens to be defined in serial-driver.o. That way that symbol will be searched for if/when the component is compiled in.

I'd actually rather resolve the hardware FIFO thing first, though. I should have probably led off with that in my reply!

[kanflo]

@projectgus yup, I noticed yesterday that my test coverage was equal to {} as nothing got linked. Slightly embarrassed, I will redo things and as you suggested expose the init function. Perhaps not the most elegant solution but until there comes a "unified init function" of sorts that will do.

I would rather see driver merged before investigating the HW FIFO as it is usable as is. HW FIFO is a possible optimization down the road. The default size of the ring buffer could of course be trimmed.

[projectgus]

(I originally posted this comment to the C++ pull request by mistake.)

I realised we could fix the weak linking problem by just being intelligent about library order in the linker arguments. Not sure why I didn't realise that earlier.

If you rebase this pull against current master with 0090153, it will link correctly without any init function tricks because the strong-linked _read_r is seen by the linker first.

However to be honest I'm still not sure about merging this, I think a driver that uses the FIFO buffer registers will probably meet most people's needs (128 byte buffer) and has basically no resource overhead.

[kanflo]

Great, I'll try that! I'll also try and adapt to the HW FIFO.

[yesco]

I'd be happy to get this is in as is. I'm already using it ;-) and depend on it!

For my part the hardware FIFO can wait. I'm more of, "improve the world in small steps",
if it doesn't break anything existing and does something then it's good.

[projectgus]

Hi yesco,

That esp-lisp REPL looks like fun! Nice work.

Did your REPL not work without this PR? You should be able to use stdin read() from the UART even without it, and you'll get the 128 byte hardware FIFO buffer automatically . The only difference, theoretically, with this PR is that the buffer can be made larger than 128 bytes so you can receive >128 bytes in between calls to read().

However a couple of people are reporting problems reading from stdin, so it's possible that it's broken somehow - in which case we should fix that. I'm going to try and make time to look at it today or tomorrow.

[kanflo]

Glad you found it useful @yesco :) I have started looking at using the HW fifo instead of the SW ring buffer and hopefully will finish it this weekend. This has been on my todo list for far too long...

@projectgus, does the stdin read problems lie within this PR?

[yesco]

Hey, no idea.

Actually I tried using getc() getchar() and none of them were working,
acutally they still aren't.

For now, I'm happy as read(1) is blocking but not buffered!

It's used for this that I started hacking yesterday and today ported to the
esp!

https://github.com/yesco/imacs

Now, I just need a filesystem to the eps-open-rtos! ;-)

On Sat, Oct 3, 2015 at 6:23 AM, Johan Kanflo [email protected]
wrote:

Glad you found it useful @yesco https://github.com/yesco :) I have
started looking at using the HW fifo instead of the SW ring buffer and
hopefully will finish it this weekend. This has been on my todo list for
far too long...

@projectgus https://github.com/projectgus, does the stdin read problems
lie within this PR?

—
Reply to this email directly or view it on GitHub
#31 (comment)
.

.sigh

[yesco]

I will clarify: I haven't had any problems with using the patch, I haven't
tried without it. Assumed it didn't work before, as getc() getchar() wasn't
working at all, just blocking.

I never seen it miss any characters, and I haven't seen any trouble at all.
But I only use it to read a character at a time.

Not sure what problems people have seen. I guess it could be useful to
ascertain if one could do non blocking read using read() from the keyboard.

Thanks!

On Mon, Oct 5, 2015 at 1:00 PM, Jonas S Karlsson (☯大鱼) [email protected]
wrote:

Hey, no idea.

Actually I tried using getc() getchar() and none of them were working,
acutally they still aren't.

For now, I'm happy as read(1) is blocking but not buffered!

It's used for this that I started hacking yesterday and today ported to
the esp!

https://github.com/yesco/imacs

Now, I just need a filesystem to the eps-open-rtos! ;-)

On Sat, Oct 3, 2015 at 6:23 AM, Johan Kanflo [email protected]
wrote:

Glad you found it useful @yesco https://github.com/yesco :) I have
started looking at using the HW fifo instead of the SW ring buffer and
hopefully will finish it this weekend. This has been on my todo list for
far too long...

@projectgus https://github.com/projectgus, does the stdin read
problems lie within this PR?

—
Reply to this email directly or view it on GitHub
#31 (comment)
.

.sigh

.sigh

[projectgus]

Hi @yesco,

Thanks for the detailed updated. I'm sorry, I should have tested more functionality myself before asking you anything - on testing it's clear that UART RX functionality is currently broken.

I have a fix in progress and I'll update #52 as it progresses.

[kanflo]

I'd actually rather resolve the hardware FIFO thing first, though. I should have probably led off with that in my reply!

@projectgus, it seems the SW ring buffer is needed, unfortunately.

My plan was to use the RX FIFO full IRQ with a one byte watermark (as I want to catch every single byte arriving). The UART ISR would clear the RX FIFO full IRQ and call xSemaphoreGiveFromISR. Back in 'userland', _read_r would take the semaphore and read directly from the HW FIFO.

The problem is that besides clearing the RX FIFO full IRQ the HW FIFO needs to be emptied. If the IRQ is cleared but the FIFO left untouched, I will get thrown back into the ISR again and again until the watchdog bites :(

[projectgus]

Thanks for the update, @kanflo.

Just to get back to basics and make sure we're both on the same page - what's the goal of the work you're doing?

Is the goal allowing the micro to receive more than 128 bytes of UART traffic between calls to read(), so there can be a larger backlog of bytes to read from stdin later on?

Or is the goal something else, related to the performance overhead of blocking for more data?

Cheers,

Angus

[kanflo]

However to be honest I'm still not sure about merging this, I think a driver that uses the FIFO buffer registers will probably meet most people's needs (128 byte buffer) and has basically no resource overhead.

@projectgus, my goal is to get #31 merged to master :D

That humorus note aside, I was hoping the circular RAM buffer could be removed as there was one "huge" in HW but it seems this is not the case.

[foogod]

I think what @projectgus was getting at (and I've had a bit of a question about as well) is what advantage does this IRQ-driven system have over what the hardware already does by default? Even if you just read (polled) directly from the hardware register, you automatically get a 127-byte buffer of all input data in hardware with no special software fiddling/driver/etc needed.

There are two reasons I can see wanting to use the IRQ-driven functionality of the UART on the ESP8266: One is in the case where you want to be doing other things normally, but want to get notified asynchronously when there's data available to be read (which I haven't looked too closely but it doesn't look like this driver really helps with). The other I can see is if for some reason you need a bigger buffer than the HWFIFO provides (i.e. your app is doing polled IO, but it's waiting for very long periods between polling), (which, given the small size of your buffer, this driver doesn't seem to really help with either)..

For anything else, you might as well just let the HWFIFO do its job and read directly from the UART port when you want data, as most stuff already does..

BTW, regarding the issue of using the HWFIFO with the IRQ, I suspect you may be thinking about things wrong: First, you don't need to generate an IRQ on every byte (that defeats the point of having the FIFO). All you need to do is make sure that if something tries to read and your memory buffer is empty, before saying "it's empty!" you first check to see whether there's anything in the HWFIFO too, and if so trigger an early read from the HWFIFO. That way you only need to do transfers out of the HWFIFO into your memory buffer when the HWFIFO gets close to full, or when something actually asks for it. Note also that in addition to a "FIFO high water mark" interrupt, the UART can also generate an interrupt when the FIFO hasn't reached the interrupt point, but there's been some data sitting in it for a minimum amount of time. You can set that timeout relatively short (a couple hundred ms), and it won't interfere when you're getting data at full speed, but if somebody sends (for example) just one character, you'll still get notified about it reasonably quickly (fast enough that no human would notice the difference, for example).

The ESP8266's UART hardware is actually a pretty amazing design, IMHO, and much better than most I've seen elsewhere. It'd really be a shame to not use the hardware they've provided to its full potential..

[kanflo]

what advantage does this IRQ-driven system have over what the hardware already does by default?

Ah, I guess I could have been more clear as to the purpose of the PR. Sorry about that.

What I want to achieve is blocking a reading thread until there is data available on the UART. If there is data in the HW FIFO the thread can of course pull a byte from it as you say but if the FIFO is empty I want the thread to sleep until data arrives.

If there is only one thread in the application and it is waiting for data it could while(1) until data arrives but that would of course waste power. For some systems I build, there might be a worker thread doing the heavy lifting in the app and a UART terminal thread for debugging, state inspection and changing eg. factory data. That thread can obviously not while(1) until data arrives as that would starve the worker thread. Having the terminal thread take a semaphore that is signaled from the UART IRQ handler means the thread consumes no CPU cycles until there is work to be done. Does that make sense?

BTW, regarding the issue of using the HWFIFO with the IRQ, I suspect you may be thinking about things wrong

I had that feeling too and I will need to dig deeper into the ESP8266 UART design.

the UART can also generate an interrupt when the FIFO hasn't reached the interrupt point, but there's been some data sitting in it for a minimum amount of time.

That seems quite interesting. Do you mean UART_CONF1.rx_tout_thrhd? If the HW FIFO is empty, the terminal thread could enable the UART_RX_TOUT_THRHD interrupt and take the semaphore. The ISR would disable the interrupt and signal the semaphore. Will test tomorrow.

It'd really be a shame to not use the hardware they've provided to its full potential..

+1!

[projectgus]

Ah, I see now!

To allow a situation where the UART interrupt handler triggers a "data waiting" semaphore, without getting stuck in the IRQ, you can probably mask/unmask the interrupt.

In the task, assuming you've already set the config registers for the interrupt you want (trigger when 1 byte available):

/* Block until some bytes appear in the UART */
_xt_isr_unmask(1<<INUM_UART);
xSemaphoreTake(uart0_sem, portMAX_DELAY);

/* check UART has some bytes in it, then read from the HW UART */

Then in the UART IRQ:

/* clear the UART interrupt bits you were expecting */
_xt_isr_mask(1<<INUM_UART);
_xt_clear_ints(1<<INUM_UART);
xSemaphoreGiveFromISR(uart0_sem, &xHigherPriorityTaskWoken);
if(xHigherPriorityTaskWoken) {
   portYIELD();
}

The xHigherPriorityTaskWoken stuff is required if you want the blocked thread to be high priority and wake up instantly when data is available, instead of waking on the next 10ms timer tick.

(NB: The macro portYIELD() is called portYIELD_FROM_ISR() or similar in most FreeRTOS ports, but I'm 99% sure our portYIELD() is safe to call from an ISR also.)

it could while(1) until data arrives but that would of course waste power

I totally agree with you that "busy-polling" is generally a sign of bad design, but it's worth also remembering that esp-open-rtos (or any other esp framework I know of) doesn't really save power when idle at the moment. So even if all the tasks in the user program are sleeping the total power consumption is unlikely to change by much.

So if you can live with some latency in responding to UART commands, making the UART command thread busy-wait may be a design compromise you can live with.

Something like this if the UART thread is lowest priority in the system (you have to ensure higher priority tasks block regularly enough not to starve it):

while(FIELD2VAL(UART_STATUS_RXFIFO_COUNT, UART(uart_num).STATUS) == 0)
    portYIELD();

Or if the UART thread is higher priority than other tasks in the system (if highest, you get worst case 10ms latency):

while(FIELD2VAL(UART_STATUS_RXFIFO_COUNT, UART(uart_num).STATUS) == 0)
    vTaskDelay(1); /* let lower priority tasks run for 1 tick */

(at 115200bps, 10ms is 144 bytes so you would stand the possibility of losing data if you're expecting big bursts of it.)

The first solution with the IRQ is probably the better one, but I thought I'd put both out there. Also, I haven't compiled or run any of this code so probably best to consider is pseudo-code at best. :)

Hope some of it helps,

Angus

[foogod]

Ah, ok, I understand better now too..

As @projectgus mentioned, this is probably a bit of putting the cart before the horse, as esp-open-rtos currently runs the CPU at full speed even when completely idle.. this could, however, change in the future (one of the things on my "want to do" list is to look into whether we can put the chip into a lower power mode in the idle task, possibly even using FreeRTOS's "tickless idle" functionality). At that point it might actually become useful..

In any case, for that scenario, you really only care about generating an interrupt when you're actively waiting for input, so as mentioned, you might as well just disable the interrupt until you actually need it. Actually, you could potentially optimize things even more if you know you want to read a minimum amount of data, as then you could actually set up the UART interrupt to only fire when there's that much in the FIFO..

I was also thinking that you probably don't need a full-fledged semaphore for this either. You can probably just do something like:

1. Check to see if there's some/enough data in the FIFO already, and if so return it.
2. Save the current task handle (from xTaskGetCurrentTaskHandle) in an appropriate global variable.
3. Configure the FIFO interrupt threshold and enable the interrupt.
4. Call vTaskSuspend(NULL) to suspend the current task.

Then just set up the UART ISR such that when it gets called, it looks up the TaskHandle and calls vTaskResumeFromISR() to wake up the sleeping task, and disables the interrupt (and that's it).

Actually, it occurs to me that this general "suspend task until interrupt" idiom might be useful enough for all kinds of things that we might want to build some support for it into the core OS. Maybe a suspend_task_until_interrupt(intnum) function which makes sure the specified interrupt is enabled, stores the current task handle in a list somewhere, and then suspends, and make the general ISR routine (in addition for looking up the handler and calling it when a particular interrupt happens) check to see if any tasks are blocked on the particular interrupt and if so wake them. Then for this "UART driver" all you'd need to do is set the threshold, and call suspend_task_until_interrupt(INUM_UART)..

[foogod]

Actually, it just occurred to me that there's a potential race condition between steps 3 and 4 in my previous comment.. In fact, it looks like the vTaskResumeFromISR docs actually mention this and explicitly suggests using a semaphore instead for that reason, so maybe it is necessary after all.. oh well. (the bit about maybe wanting to make this core functionality is still valid, I think, though..)

[kanflo]

Thanks! I will test with @projectgus' IRQ masking solution this weekend.
@foogod, pity about the race condition. I rather liked your minimalistic solution.

[kanflo]

@projectgus, I have implemented your suggestion in a new branch and will create a new PR.