use uint8_t with the serial ringbuffer [imported]

[cmaglie]

This is Issue 1078 moved from a Google Code project.
Added by 2012-10-21T15:57:40.000Z by KevinCat...@gmail.com.
Please review that bug for more context and additional comments, but update this bug.

Original labels: Type-Defect, Priority-Medium

Original description

In HardwareSerial.cpp the ring buffer is defined as

struct ring_buffer
{
unsigned char buffer[SERIAL_BUFFER_SIZE];
volatile unsigned int head;
volatile unsigned int tail;
};

The head and tail variables should be redefined as uint8_t since the value is always between 0 and 64 (inclusive). This saves 2 bytes of SRAM per buffer.

Also, the current code looks like it has a subtle bug. Reading head and tail from outside the ISR is not atomic since they are 16 bit, yet the code does not disable interrupts to ensure atomicity. In general that could result in reading a value that the variable never actually had.

It turns out though that in this case the high byte will always be 0, and there is no hidden bug. Nevertheless some people have been confused by this[0]. Changing to uint8_t eliminates this confusion, while also saving space.

Footnote:
[0] For an example of people thinking there may be a bug here, see http://bleaklow.com/2012/02/29/why_im_ditching_the_arduino_software_platform.htm

[nickgammon]

Any reason this is not being done? Making that change makes a simple sketch smaller by 36 bytes, and the poster is correct that accesses to "head" and "tail" are not done atomically outside an ISR.

Example sketch on the Uno (IDE 1.0.5) that is smaller with the change:

void setup ()
  {
  Serial.begin (115200);
  }  // end of setup

void loop ()
  {
  while (Serial.available ())
     Serial.write (Serial.read ());  
  }  // end of loop

Suggested change: Make ring_buffer be:

struct ring_buffer
{
  unsigned char buffer[SERIAL_BUFFER_SIZE];
  volatile unsigned char head;
  volatile unsigned char tail;
};

[cmaglie]

Hi Nick,
it's the next change in my queue, using @matthijskooijman commits as a base.

C

[cmaglie]

Fixed.

[ZNikke]

This issue is closed, but I see no sign of the fix being committed to the main Arduino tree?

[EDIT]
Bah, it's in the ide-1.5.x branch. I got confused due to bad search karma and the link of the commit above lead me astray. Sorry for the noise.

[PaulStoffregen]

https://github.com/arduino/Arduino/blob/ide-1.5.x/hardware/arduino/avr/cores/arduino/HardwareSerial.h#L47