Skip to content

Conversation

@projectgus
Copy link
Contributor

Thanks @kanflo for this contribution!

I have a few thoughts for discussion before merging this. I'm not asking you to necessarily do any of the things here, but it's a good chance to discuss the long-term goals around UART functionality.

There are some parts I'd like to get into esp-open-rtos, probably sooner rather than later:

  • Cleaner esp_reg_t macros for the UART registers, in include/esp/registers.h, so no need for the READ_PERI_REG/WRITE_PERI_REG stuff.
  • Remove sdk_os_putchar, sdk_os_readchar, baud rate functions, etc. entirely and replace them with an equivalent core/uart.c

I also think @foogod's suggestion of minimising bloat/automatically pulled in code is a good one. Relying on the linker for this is (IMHO) good, so weak linking and/or --ffunction-sections and/or having syscalls in their own source files are all good approaches - so you don't link _read_r unless you actually call read() somewhere in your program, etc.

I see UART functionality as "core" functionality to the chip, too, so I'd like to have whatever support is required for good UART interaction included in the core, rather than needing to specify an extra component for it. I see at the moment that you can use just the 'core' and have a busy-waiting read with no buffering, or if you use the extra driver then you get buffering and a cleaner blocking read(), which is clearly a much better configuration.

Anyhow, sorry to dump a bunch of requirements out of the blue! If you're not interesting in making further changes then that's fine, I can probably jump in a give them a shot soon - just let me know.

(Also if you think any of this is a bad idea, let me know about that as well!)

Angus

See examples/terminal/ for usage
@projectgus projectgus mentioned this pull request Aug 10, 2015
@projectgus
Copy link
Contributor Author

Here are @kanflo's original comments from Issue #22:

I have completed support for this in e648cd2

The driver is placed in extras/serial-driver and there is an example program in examples/terminal showing how to include the driver (it is not included by default).

TL;DR:
_read_r in newlib_syscalls.c is now declared as weak and extas/serial-driver/serial_driver.c >implements a new version of _read_r that blocks until data is available and reads from a circular buffer >filled by the UART0 IRQ handler. The IRQ handler is installed on the first call to _read_rwith a small >penalty for subsequent calls.

@kanflo
Copy link
Contributor

kanflo commented Aug 11, 2015

You're welcome. I wasn't aware of the esp_reg_t macros but yes, it would be nicer to have those instead of the PERI_REGs. I'll have a look at them when I find the time.

I also see UART functionality as core but one could always debate if UART RX is core. Either way, having RX included when referenced would be nice. And a lot nicer than having to specify extra components.

@projectgus
Copy link
Contributor Author

FWIW, @foogod has just contributed better UART register support. Should be possible to do away with the

I'm also planning to shortly write a set of uart functions in 'core' to replace the SDK getc/putc functions.

Something I've noticed mentioned in other places is that apparently the UART itself has a 128 byte FIFO buffer (one for each direction), so maybe software ringbuffering isn't actually necessary? I haven't verified this is actually the case yet, though, it may be that the 128 byte buffer is part of the SDK.

@kanflo
Copy link
Contributor

kanflo commented Aug 25, 2015

Yup, nice addition from @foogod. I might have messed things up slightly but there's a new pull request for here: #31

The ring buffering still needs to be verified. Also, this PR can be closed but I see no button for it.

@projectgus
Copy link
Contributor Author

Closing this in favour of #31.

@projectgus projectgus closed this Aug 26, 2015
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

Successfully merging this pull request may close these issues.

2 participants