% Division in USART interrupt code...

Function store_char() (in HardwareSerial.cpp), called from USART interrupt, use % division, which is mutch slower (208 ticks, as i measure), than & operation. If divider is power of 2, why not use & ?

For example:

define SERIAL_BUFFER_SIZE 64

.....

int i = (unsigned int)(buffer->head + 1) % SERIAL_BUFFER_SIZE; are equal to: int i = (unsigned int)(buffer->head + 1) & (SERIAL_BUFFER_SIZE - 1);

The GCC compiler is pretty smart and it should convert the % to & automatically during its optimization phase. Try it: I bet the code generated will be the same size with both approaches (as long as the modulus is a power of 2).

Knowing this, it’s always better to write clear(er) code than to prematurely optimize it.


The Gadget Shield: accelerometer, RGB LED, IR transmit/receive, speaker, microphone, light sensor, potentiometer, pushbuttons

llevinson:
If divider is power of 2, why not use & ?

Just way less readable and maintainable, in a pretty significant way. Only worth this kind of optimization in cases where serial performance was the bottleneck.

RuggedCircuits:
The GCC compiler is pretty smart and it should convert the % to & automatically during its optimization phase. Try it: I bet the code generated will be the same size with both approaches (as long as the modulus is a power of 2).

Ah yes, this is right, too.

int main(void)
{
 SPDR = SPDR % 64;
}
00000080 <main>:
  80:   8e b5           in      r24, 0x2e       ; 46
  82:   8f 73           andi    r24, 0x3F       ; 63
  84:   8e bd           out     0x2e, r24       ; 46

Hm… Very strange.
Yes, your code do not generate any divisions.

But when a compile simple sketch:
void setup() {
Serial.begin(9600);
}
void loop() {}

and do (on Windows):
c:\BIN\Arduino\arduino-1.0\hardware\tools\avr\bin\avr-objdump.exe -d test123.cpp.elf

i get:

00000162 <__vector_19>
162: 1f 92 push r1
164: 0f 92 push r0

1bc: 01 96 adiw r24, 0x01 ; 1
1be: 60 e4 ldi r22, 0x40 ; 64
1c0: 70 e0 ldi r23, 0x00 ; 0
1c2: 0e 94 34 03 call 0x668 ; 0x668 <__divmodhi4>

Maybe, this code is from another place… I don’t know assembler wery well…

maniacbug:

llevinson: If divider is power of 2, why not use & ?

Only worth this kind of optimization in cases where serial performance was the bottleneck.

I try to do simple audio recorder from the arduino uno (just for fun and training). I programmed Timer2 for generate interrupts at 16KHz (every 62.5uS) and set ADC prescaler to 64 (so, ADC conversion must take 52uS). But sometymes, ADC don't have to complete previous conversion, when the next interrupt occurs.

I think, it is because other interrupts brings a time jitter to time, when Timer2 interrupt handler raise...

Thank you, for your quick response!

Maybe, this code is from another place... I don't know assembler wery well...

You know it well enough. That's a problem.

• Open HardwareSerial.cpp

• At or near line 206, change this...

  tx_buffer.tail = (tx_buffer.tail + 1) % SERIAL_BUFFER_SIZE;

...to this...

  tx_buffer.tail = (unsigned int)(tx_buffer.tail + 1) % SERIAL_BUFFER_SIZE;

• At or near line 230, change this...

  tx_buffer1.tail = (tx_buffer1.tail + 1) % SERIAL_BUFFER_SIZE;

...to this...

  tx_buffer1.tail = (unsigned int)(tx_buffer1.tail + 1) % SERIAL_BUFFER_SIZE;

• At or near line 247, change this...

  tx_buffer2.tail = (tx_buffer2.tail + 1) % SERIAL_BUFFER_SIZE;

...to this...

  tx_buffer2.tail = (unsigned int)(tx_buffer2.tail + 1) % SERIAL_BUFFER_SIZE;

• At or near line 264, change this...

  tx_buffer3.tail = (tx_buffer3.tail + 1) % SERIAL_BUFFER_SIZE;

...to this...

  tx_buffer3.tail = (unsigned int)(tx_buffer3.tail + 1) % SERIAL_BUFFER_SIZE;

• At or near line 385, change this...

  int i = (_tx_buffer->head + 1) % SERIAL_BUFFER_SIZE;

...to this...

  int i = (unsigned int)(_tx_buffer->head + 1) % SERIAL_BUFFER_SIZE;

• Save and close HardwareSerial.cpp

Basically, put "(unsigned int)" in front of every expression that includes "% SERIAL_BUFFER_SIZE".

And, as a nice side-effect, the entire ISR is much smaller.

Sounds like someone needs to submit a patch :smiley:

http://code.google.com/p/arduino/issues/detail?id=776

Nice. Although if you actually attach a patch file, I think it increases the odds of it getting in...

patch attached. I actually had this almost ready based on the discussions over in:
http://arduino.cc/forum/index.php/topic,85207.0.html

And "while you are in there", why not modify the code to use 8 bit variables for head and tail? It saves hundreds of bytes of code to do this. It is one of the first things I change on every single Arduino release. And if you are worried about somebody making SERIAL_BUFFER_SIZE larger than 256 then that can also be handled compile time with a #if to change back to using ints.

Example:

struct ring_buffer
{
  unsigned char buffer[SERIAL_BUFFER_SIZE];
#if SERIAL_BUFFER_SIZE > 256
 volatile int head;
 volatile int tail;
#else
 volatile uint8_t head;
 volatile uint8_t tail;
#endif

};

But in the bigger picture, larger than 8 bit indexes won't really work anyway because the code that uses it does not properly deal with atomicity of the 2 byte value when doing things like compares. So it might as well be an 8 bit value anyway since 16 bit (multi byte) values will have issues when the value is larger than 255.

--- billl

Interesting, this is just a regression in1.0, I checked my 0022 HardwareSerial and all the buffer size lines are properly cast.

Half of a regression. Some of the effected code is new. :D

Can an authoritative, knowledgeable person change hardware/arduino/cores/arduino/HardwareSerial.cpp:

from: volatile unsigned int head; volatile unsigned int tail;

to: volatile uint8_t head; volatile uint8_t tail;

The most effective way to get a change made to the software is to...

• Create an issue here... http://code.google.com/p/arduino/issues/list

• Either attach a diff file to the issue or create a pull request here... https://github.com/arduino/Arduino/

• You may want to work against the more actively developed version 1.5... https://github.com/arduino/Arduino/tree/ide-1.5.x

I would not hold your breath on changes.

the number of bugs in the code that are known and documented is depressing.

if your on windows, you might want to look at this,

http://arduino.cc/forum/index.php/topic,118440.0.html

hes sorted out quite a few buts and pieces that the official guys dont seem to be able to,