Go Down

Topic: Improving core Arduino Code for ADC initialization (Read 5200 times) previous topic - next topic

MrAlvin

In the Arduino core code I have noticed this text:
Code: [Select]
#if defined(ADCSRA)
// set a2d prescale factor to 128
// 16 MHz / 128 = 125 KHz, inside the desired 50-200 KHz range.
// XXX: this will not work properly for other clock speeds, and
// this code should use F_CPU to determine the prescale factor.
sbi(ADCSRA, ADPS2);
sbi(ADCSRA, ADPS1);
sbi(ADCSRA, ADPS0);

// enable a2d conversions
sbi(ADCSRA, ADEN);
#endif


I have two questions:

1.  I wonder if the following code might not do what is asked for?

2.  Why is the sbi() function used four times? Does it not take more instructions than just setting the entire register by loading one byte into the register?

Code: [Select]
  // To get the fastest (and still reliable) ADC (Analog to Digital Converter)
  // operations, set the ADC_Clk in the ADCSRA register
 
                                  // Preferred: 50KHz < ADC_Clk < 200KHz
#if  F_CPU == 16000000L
#define ADC_SPEED B00000111       //ADC_Clk = F_CPU / 128 => 125KHz
#elif F_CPU == 8000000L
#define ADC_SPEED B00000110       //ADC_Clk = F_CPU /  64 => 125KHz
#elif F_CPU == 4000000L
#define ADC_SPEED B00000101       //ADC_Clk = F_CPU_Pre /  32 => 125KHz
#elif F_CPU == 2000000L
#define ADC_SPEED B00000100       //ADC_Clk = F_CPU_Pre /  16 => 125KHz
#elif F_CPU == 1000000L
#define ADC_SPEED B00000011       //ADC_Clk = F_CPU_Pre /   8 => 125KHz
#elif F_CPU == 500000L
#define ADC_SPEED B00000010       //ADC_Clk = F_CPU_Pre /   4 => 125KHz
#elif F_CPU == 250000L
#define ADC_SPEED B00000001       //ADC_Clk = F_CPU_Pre /   2 => 125KHz
#elif F_CPU == 125000L
#define ADC_SPEED B00000000       //ADC_Clk = F_CPU_Pre /   1 => 125KHz
#elif F_CPU == 62500L
#define ADC_SPEED B00000000       //ADC_Clk = F_CPU_Pre /   1 => 62.5KHz
#endif

ADCSRA = ( 10000000 | ADC_SPEED);  // Activate ADC and set ADC_Clk

kowalski

#1
Mar 08, 2014, 08:49 pm Last Edit: Mar 08, 2014, 08:51 pm by kowalski Reason: 1
Why is the original code better? It uses the correct symbols from AVR and not the bit values.

What could be better in both? Actually it is the enable of the converter. This could be done later on and only when needed. If using a strategy for low power design this is not a very good idea. The Analog Comparator should also be disabled.

I agree that the setting can be written as a single line. There is no need for four sbi(). In the large scale of things this is not very important. A few bytes extra. And a few extra cycles at start up (once).

Below is what you find in Cosa init; https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/main.cpp#L46
Code: [Select]

void init()
{
 // Set analog converter prescale factor and but do not enable conversion
#if F_CPU >= 16000000L
 ADCSRA |= (_BV(ADPS2) | _BV(ADPS1) | _BV(ADPS0));
#elif F_CPU >= 8000000L
 ADCSRA |= (_BV(ADPS2) | _BV(ADPS1)             );
#else
 ADCSRA |= (             _BV(ADPS1) | _BV(ADPS0));
#endif
 
 // Disable analog comparator
 ACSR = _BV(ACD);

 // The bootloader connects pins 0 and 1 to the USART; disconnect them
 // here so they can be used as normal digital IO.
#if defined(UCSRB)
 UCSRB = 0;
#elif defined(UCSR0B)
 UCSR0B = 0;
#endif

 // Allow interrupts from here on
 sei();

 // Attach the USB module and possible CDC/HID
#if defined(USBCON)
 USBDevice.attach();
#endif
}

What would be even better? I think defining the prescale value from conversion and system clock. PRESCALE = F_CPU / F_ADC. This is what inspired me from this topic.

Cheers!

BTW: I would like to encourage you to make a pull request to the Arduino repository.

MrAlvin

#2
Mar 09, 2014, 12:49 pm Last Edit: Mar 09, 2014, 08:57 pm by MrAlvin Reason: 1

Why is the original code better? It uses the correct symbols from AVR and not the bit values.

Ah, yes. That makes a lot of sense.


What could be better in both? Actually it is the enable of the converter. This could be done later on and only when needed. If using a strategy for low power design this is not a very good idea. The Analog Comparator should also be disabled.


Yes, there are more details that needs to be looked into, when one tries to save power.  For now (in this thread anyway), I am however going to separate the two issues.


I agree that the setting can be written as a single line. There is no need for four sbi(). In the large scale of things this is not very important. A few bytes extra. And a few extra cycles at start up (once).

For the sake of using best practices/official AVR codestyle it is indeed worth using a few extra bytes of code.

I have since learned, that setting bits directly is called "using magic numbers", and it is not considered good coding style, as it typically leads to  poor documentation and/or code that is difficult to decipher/maintain.



Below is what you find in Cosa init; https://github.com/mikaelpatel/Cosa/blob/master/cores/cosa/main.cpp#L46
Code: [Select]

void init()
{
 // Set analog converter prescale factor and but do not enable conversion
#if F_CPU >= 16000000L
 ADCSRA |= (_BV(ADPS2) | _BV(ADPS1) | _BV(ADPS0));
#elif F_CPU >= 8000000L
 ADCSRA |= (_BV(ADPS2) | _BV(ADPS1)             );
#else
 ADCSRA |= (             _BV(ADPS1) | _BV(ADPS0));
#endif
 
 // Disable analog comparator
 ACSR = _BV(ACD);

 // The bootloader connects pins 0 and 1 to the USART; disconnect them
 // here so they can be used as normal digital IO.
#if defined(UCSRB)
 UCSRB = 0;
#elif defined(UCSR0B)
 UCSR0B = 0;
#endif

 // Allow interrupts from here on
 sei();

 // Attach the USB module and possible CDC/HID
#if defined(USBCON)
 USBDevice.attach();
#endif
}


I see that you use a different style of setting bits in a register. I Further have learned that the sbi() macro style has been deprecated by the avr-libc team, in favor of the "setting bits" style you have in your example.


What would be even better? I think defining the prescale value from conversion and system clock. PRESCALE = F_CPU / F_ADC. This is what inspired me from this topic.

And F_ADC would be? The desired ADC sample rate?

I think it would be nice to be able to set the MPU main Prescale rate at run time, as a standard feature within the Arduino IDE. I do however think that we are quite a long way away from making this happen. Especially as it seems that the Arduino team (and rightly so) is very focused on making everything Arduino; very, very easy to use and comprehend for the entry level user.

So I think I will again keep the two issues separate, in this thread anyway.


BTW: I would like to encourage you to make a pull request to the Arduino repository.


Ah yes, that might eventually happen. I am however not a very experienced programmer or git user, so right now it seems like a bit of a daunting task.
So I thought that it might be prudent to use this forum to prepare better, and learn more about a few AVR standards/best practices, before I would venture into trying to enforce my ideas upon the entire Arduino World ;-)


So I greatly appreciate you feedback and input. Thank you!


Inspired by your code example, and to use the sbi() way of doing things, I have so far come up with the following:
Code: [Select]
#if defined(ADCSRA)
// To get the fastest (and still reliable) ADC (Analog to Digital Converter)
// operations, set the ADC_Clk in the ADCSRA register
 
                          // Preferred: 50KHz < ADC_Clk < 200KHz
#if  F_CPU >= 16000000L    //ADC_Clk = F_CPU / 128 => 125KHz
sbi(ADCSRA, ADPS2);
sbi(ADCSRA, ADPS1);
sbi(ADCSRA, ADPS0);     
#elif F_CPU >= 8000000L    //ADC_Clk = F_CPU /  64 => 125KHz
sbi(ADCSRA, ADPS2);
sbi(ADCSRA, ADPS1);
cbi(ADCSRA, ADPS0);     
#elif F_CPU >= 4000000L    //ADC_Clk = F_CPU  /  32 => 125KHz
sbi(ADCSRA, ADPS2);
cbi(ADCSRA, ADPS1);
sbi(ADCSRA, ADPS0);
#elif F_CPU >= 2000000L    //ADC_Clk = F_CPU  /  16 => 125KHz
sbi(ADCSRA, ADPS2);
cbi(ADCSRA, ADPS1);
cbi(ADCSRA, ADPS0);
#elif F_CPU >= 1000000L    //ADC_Clk = F_CPU  /   8 => 125KHz
cbi(ADCSRA, ADPS2);
sbi(ADCSRA, ADPS1);
sbi(ADCSRA, ADPS0);
#elif F_CPU >= 500000L     //ADC_Clk = F_CPU  /   4 => 125KHz
cbi(ADCSRA, ADPS2);
sbi(ADCSRA, ADPS1);
cbi(ADCSRA, ADPS0);
#elif F_CPU >= 250000L     //ADC_Clk = F_CPU  /   2 => 125KHz
cbi(ADCSRA, ADPS2);
cbi(ADCSRA, ADPS1);
sbi(ADCSRA, ADPS0);
#elif F_CPU >= 125000L     //ADC_Clk = F_CPU  /   1 => 125KHz
cbi(ADCSRA, ADPS2);
cbi(ADCSRA, ADPS1);
cbi(ADCSRA, ADPS0);
#elif F_CPU >= 62500L     //ADC_Clk = F_CPU  /   1 => 62.5KHz
cbi(ADCSRA, ADPS2);
cbi(ADCSRA, ADPS1);
cbi(ADCSRA, ADPS0);
#endif

// enable a2d conversions
sbi(ADCSRA, ADEN);  
#endif


This code improves what is already there, without changing the way things are currently done in the official Arduino core.

It also prepares for the idea of compiling for F_CPU speeds other than 16 or 8MHz, without increasing the size of the resulting compiled code.


One question is however if the cbi() lines can be omitted, as one, at this point in the code, can assume that the reset condition of ADCSRA == 0 is still in effect.


Go Up