UART Receive Buffering | Hacker News Reader

UART Receive Buffering

simplyembedded.org

Important to remember for each data byte in the buffer to also store the LSR along with that data. Windows serial port driver fails to do this, meaning you can tell there was (for example) a parity error on a byte in your fifo, you just don't know on which byte.

Line Status Register http://en.wikibooks.org/wiki/Serial_Programming/8250_UART_Pr... Had to look it up, my brain was coming up with Logical Shift Right.

How would you implement that though, assuming memory is tight? The simplest would be to just store a multibyte entry for each item in the ring buffer: the actual data, and the status flags set at the time it was copied. But that doubles your memory requirements, and you'll (probably) not need that status info all the time.

I suppose you could define 'normal' and keep a smaller structure which holds only abnormal values, but then you need a way to map which input byte each value refers to, and make sure it's invalidated when the ring buffer rolls around.

Or you could put some of the error-handling logic into your buffering code (but it'd be in an interrupt handler, which you probably don't want), and you might need your higher level framing or packetisation to know how far back you need to invalidate.

I'm not sure there is a single optimal solution (although if you know of one, I'd love to know). Windows getting it wrong is interesting/odd though, unless it just dates to a time when tehy were cpu/memory restricted, adn it's just stuck about for compatibility.

I assume that your serial port will run some kind of machine-to-machine protocol, and not just a serial console with user data (for which, frankly, I'll just ignore most errors). These serial prototols (like modbus, or the 10000s of ad-hoc specified "proprietary" protocols many devices speak) are usually very simple.....

And if memory is tight, you'll probably also want to move some part of your protocol handling down the stack, probably up to the interrupt handler, because otherwise you'll needlessly have double buffers (characters, and decoded protocol datagrams). For most stuff running on serial ports the code will be simple enough not to create too many headaches. Then just store the fact that you've encountered a parity error next to your datagram. (msg->flags |= MSGFLASG_PARITYERR );

And yes, I know that this is a "blatant layering violation" for a proper operating system, but on a microcontroller really no one cares that you have two variations of an interrupt handler, specific to your protocol handling.

EDIT/ADDED: It's not only parity errors, but there are also things like RS485 links with 9-bit addresses, BREAK characters with semantic meaning and sometimes your communication protocol might specify when you switch directions of half-duplex links, with hard timing requirements.

You might implement it with a separate bitmap. One bit for each char in buffer. If a parity error occurs, set the corresponding bit on in the bitmap.

The mix of global buffer and passed-in struct defining the buffer made me sad. So did the lack of atomic increments and decrements (volatile ain't sufficient). Don't get me started on the getchar routine.

This stuff just isn't hard. Sigh.

Why isn't volatile sufficient here? If they were both modifying the same variable, I can see how

A:read Ax; (Interrupt: B:read Bx; B:write Bx+1; return); A: write Ax+1 # (wrong!)

could occur. But the only error I can see here is that sometimes put() might fail thinking there is overflow when there's maybe one space left.

Am I missing something?

(Further investigation: This[1] thread suggests you can run into problems if your counter type is larger than the native ALU size, because the interrupt could occur midway through your increment operation. But here the size_t is 16 bits, so that should 't be a problem?)

[1] http://www.embeddedrelated.com/showthread/msp430/20400-1.php

Also the update of the write pointer could be reordered to occur before storage of the actual data flushed to the buffer.

      e.g.
      buf[volatile_write_ptr] = c
      volatile_write_ptr++;

might occur the other way 'round for the code that is reading from buf[].

For typical "simple" single processor controllers this will not be an issue, though, and "volatile"+spinlocks/disenable-enable-interrupts will work just fine.

Yes, the ceremony to set up a ring buffer was confusing.

But isn't getchar() at least doing the same as before?

If the ring buffer is empty, ring_buffer_get() won't write to its data argument, and since getchar() passes in -1, that's what it's going to be returning if the buffer was empty. Just like it says it did before using a ring buffer.

> The mix of global buffer and passed-in struct defining the buffer made me sad. So did the lack of atomic increments and decrements (volatile ain't sufficient). Don't get me started on the getchar routine.

It's for MSP430, and seems to be oriented in single core embedded microcontrollers. Those things have no atomic operations, other than disabling interrupts.

You'd be right for multicore chips or if data type is larger than word size or if data is not aligned.

But in this case, this code is correct for this architecture. All of those things you mentioned are perfectly normal and acceptable when programming microcontrollers.

http://www.simplyembedded.org/tutorials/msp430-architecture/

http://en.wikipedia.org/wiki/TI_MSP430

I guess you're assuming the compiler will issue an "increment" instruction for the ++, and that instruction is atomic.

It's not a great coding practice. Move to a different processor (this happens!) and your code turns into a load-add-store, gets interrupted halfway through, and you scratch your head for a while about the mysterious and flaky FIFO failures.

Been there. At least a gesture towards an atomic operation would be nice, when you have data shared between interrupt and non-interrupt code.

How about putting

#if !defined(__AVR__) && !defined(__MSP430__)

#warning "Fix this FIFO - ++/-- possibly not atomic!"

#endif

into the header of that thingy then. Save work on small uCs, and be warned when moving it somewhere else.

There's at least 1 simple bug that I found: ring_buffer_get() returns an error code, but it's being ignored in uart_getchar().

Love seeing embedded stuff on HN!

Reply on news.ycombinator.com