ADC initialization mistake in ADC.h

[murmuring]

As has been noted by "http://www.djerickson.com/arduino/" (all credits to that person).
It seems there may be a mistake in the ADC initialization which results in a relatively slow analogRead, for a full description of his finding go to the link, but the summary of the problem (quoted):

Here are the #defines for the STARTUP field from
\arduino\sam\system\libsam\include\adc.h

/* The normal adc startup time*/
#define ADC_STARTUP_NORM     40
/* The fast adc startup time*/
#define ADC_STARTUP_FAST     12

This may be a bug. STARTUP is a four bit field so 12 (0xC) works, but 40 (0x28) won't work. And 12 sets the value to 768 which is why the ADC is so slow. So I suspect that the programmer confused the value with the hardware settings. The table of values vs. register settings is in the SAM3S processor manual.

[jtouch]

The values 40 and 12 accurately describe the number of microseconds required, but there is no equation that goes from those times to the values set in the STARTUP bitfield; it's more of a map that correlates bitfield values to numbers of ADC clock cycles:
0 SUT0 0 periods of ADCClock
1 SUT8 8 periods of ADCClock
2 SUT16 16 periods of ADCClock
3 SUT24 24 periods of ADCClock
4 SUT64 64 periods of ADCClock
5 SUT80 80 periods of ADCClock
6 SUT96 96 periods of ADCClock
7 SUT112 112 periods of ADCClock
8 SUT512 512 periods of ADCClock
9 SUT576 576 periods of ADCClock
10 SUT640 640 periods of ADCClock
11 SUT704 704 periods of ADCClock
12 SUT768 768 periods of ADCClock
13 SUT832 832 periods of ADCClock
14 SUT896 896 periods of ADCClock
15 SUT960 960 periods of ADCClock

For the Due, the ADC clock is determined by a prescalar from MCK, and it's available in the code as a function. The code in adc.c for the function adc_init appears to lack this table above, and instead uses the values ADC_STARTUP_NORM directly.

[cmaglie]

The hardware specification state that 20 ADCClock cycles are required for each conversion, so the minimum safe value is "3" => 24 ADCClock.

Thanks!
C

[jtouch]

Hi, Cristian,

I could not determine the correct safe value because the number of
cycles depends on the prescale factor:

ADCClock = MCK / ( (PRESCAL+1) * 2 )

(spec sheet page 1346)

Can you clarify why 20 ADCClock cycles is always correct?

Joe

On 9/25/2013 4:25 PM, Cristian Maglie wrote:

The hardware specification state that 20 ADCClock cycles are required
for each conversion, so the minimum safe value is "3" => 24 ADCClock.

Thanks!
C

—
Reply to this email directly or view it on GitHub
#1418 (comment)
Bug from MailScannerWebBug

[cmaglie]

Well, it took some time for me to decode, from the following table:

We deduce that ADCClock should be <= 20MHZ. The CPU on the Due runs at 84MHz so we are forced to use PRESCALER=2 that gives 84/(2+1)_2=14Mhz. Using PRESCALER=1 gives 84/(1+1)_2=21 that is 1Mhz off (the ADC will probably runs fine also with a small overclock like this, but that its the final user's choice, we can't deliver code that don't operate in the safe area).

From the following table:

we see that conversion time is always 20 ADCClock (well, its 20 T_cp_adc that is 1/f_adc).

This is my understating of the Datasheet, I'll be glad if someone can confirm if is correct.

Thanks!
C

[jtouch]

Hi,

On 9/26/2013 2:19 AM, Cristian Maglie wrote:

Well, it took some time for me to decode, from the following table:

sam3-1
https://f.cloud.github.com/assets/423515/1216435/9eb1047a-268a-11e3-8640-def9ae2e27ad.png

We deduce that ADCClock should be <= 20MHZ.

I agree.

The CPU on the Due runs at
84MHz

The CPU runs at HCLK rates.

The ADC is derived from MCK rates. MCK is derived from the external
crystal tied to XIN/XOUT, which is 12 MHz here, which can be scaled down
further.

I'm not sure what the scale for MCK is. However, assuming no scale, MCK
is at most 12 MHz.

so we are forced to use PRESCALER=2 that gives 84/(2+1)*2=14Mhz.

ADCClock = MCK / ( (PRESCAL+1) * 2 )

MCK = 12 MHz (let's assume)

Using PRESCALER =1 (is that what it is?) then ADCClock is 3 MHz. Even
though it can be faster, it isn't here.

Using PRESCALER=1 gives 84/(1+1)*2=21 that is 1Mhz off (the ADC will
probably runs fine also with a small overclock like this, but that its
the final user's choice, we can't deliver code that don't operate in the
safe area).

From the following table:

sam3-2
https://f.cloud.github.com/assets/423515/1216436/a2553830-268a-11e3-824a-031747e335a0.png

we see that conversion time is always 20 ADCClock (well, its 20 T_cp_adc
that is 1/f_adc).

I don't understand that. STARTUP varies from 0 to 960 periods of the
ADCClock. There would be no point in having that table if the answer was
always "20 clocks" - i.e., if so, there would never be a reason to set
STARTUP to a value other than 3.

So I don't think this is correct.

I think the answer in the code should be:

given ADC_STARTUP_NORM  (which we have)
given PRESCAL (which we need to get somehow from elsewhere in
    the code??)
given MCK = 12 MHz (which we know from the Due schematic)

ADCClock = ADCClock = MCK / ( (PRESCAL+1) * 2 )

STARTUP = lookup_in_table(ADC_STARTUP_NORM / ADCClock)

(where "lookup_in_table" finds the STARTUP value that corresponds to the
number of ADC clock cycles needed as per the table I posted earlier)

I cannot see a way to avoid including the table and a lookup function. I
also have no way to figure out what PRESCAL is.

Joe

This is my understating of the Datasheet, I'll be glad if someone can
confirm if is correct.

Joe

Thanks!
C

—
Reply to this email directly or view it on GitHub
#1418 (comment)
Bug from MailScannerWebBug

[cmaglie]

Ok, let's face one piece at a time.

The 12MHz external oscillator goes through PLLA that scales clock to 84Mhz. The Due Board runs at 84Mhz there is no doubt about that, so MCK is 84M.

Setup code here:
https://github.com/arduino/Arduino/blob/ide-1.5.x/hardware/arduino/sam/system/CMSIS/Device/ATMEL/sam3xa/source/system_sam3xa.c#L28

Internal schematics here:

MCK==84M means that we must set the PRESCAL value to 2, because

ADCClock = MCK / ( (PRESCAL+1) * 2 ) = 84M / ((2+1)*2 = 14M

using 1 we get:

ADCClock = MCK / ( (PRESCAL+1) * 2 ) = 84M / ((1+1)*2 = 21M > 20M (as per datasheet specification)

do you agree until now?

[jtouch]

Hi, Christian,

On 9/26/2013 5:49 AM, Cristian Maglie wrote:

Ok, let's face one piece at a time.

The 12MHz external oscillator goes through PLLA that scales clock to
84Mhz.

PLLA is a divider (increases frequency), so you're saying DIV is set to
7. AOK.

The Due Board runs at 84Mhz there is no doubt about that, so MCK
is 84M.

Setup code here:
https://github.com/arduino/Arduino/blob/ide-1.5.x/hardware/arduino/sam/system/CMSIS/Device/ATMEL/sam3xa/source/system_sam3xa.c#L28

Internal schematics here:
sam3-3
https://f.cloud.github.com/assets/423515/1217403/a85268f0-26a5-11e3-98dc-bf232391a2c5.png

AOK. Got it ;-)

MCK==84M means that we must set the PRESCAL value to 2, because

ADCClock = MCK / ( (PRESCAL+1) * 2 ) = 84M / ((2+1)*2 = 14M

using 1 we get:

ADCClock = MCK / ( (PRESCAL+1) * 2 ) = 84M / ((1+1)*2 = 21M > 20M (as
per datasheet specification)

do you agree until now?

Yes. I see now.

I agree that the conversion time is 20 ADC clocks, but that's not
relevant to the STARTUP time.

If ADCClock is 14MHz (I agree now), then:

STARTUP = lookup_in_table(ADC_STARTUP_NORM / ADCClock_cycle)

ADC_STARTUP_NORM is 40 (microseconds)
in hardware/arduino/sam/system/libsam/include/adc.h
also as per max time from "off" in the specs on Table 46-28. Channel
Conversion Time and ADC Clock

ADCClock_cycle = 1 / ADCClock

i.e., 71.143 ns

so we should lookup the appropriate value for (40 microsec / 71.143 ns) or

562.25

So STARTUP should be "9" - i.e., 576 cycles.

8 is too small (512 cycles), and 3 (24 cycles) is way too small.

AFAICT ;-)

Joe

[jtouch]

PS - this also explains why the original value of writing
ADC_STARTUP_NORM into the STARTUP register worked.

Writing 40 effectively writes 12 (because the register uses only the
lower 4 bits). 12 represents 768 clock cycles, which is more than enough
for the max of 560 needed (14 MHz * 40 microseconds).

Note that the max is 40 microseconds only when going from off to normal
mode. The max is 12 when going from Standby, where the voltage reference
is already on. In that case, 14 MHz * 12 microseconds would be 168
cycles, which corresponds to a STARTUP value of 8 (512 cycles).

If we went with typical values rather than max (worst case), we'd get a
lot better performance especailly for the latter case (from Standby),
e.g., 112 cycles (exactly a STARTUP of 8), which is 4x faster.

This all matters only when going from OFF to running, or standby to
running. In the usual case, when everything is up, the 20 cycle delay
you cited earlier would apply - i.e., 700,000 samples/second if
everything else could keep up.

Joe

On 9/26/2013 6:24 AM, Joe Touch wrote:

Hi, Christian,

On 9/26/2013 5:49 AM, Cristian Maglie wrote:

Ok, let's face one piece at a time.

The 12MHz external oscillator goes through PLLA that scales clock to
84Mhz.

PLLA is a divider (increases frequency), so you're saying DIV is set to
7. AOK.

The Due Board runs at 84Mhz there is no doubt about that, so MCK
is 84M.

Setup code here:
https://github.com/arduino/Arduino/blob/ide-1.5.x/hardware/arduino/sam/system/CMSIS/Device/ATMEL/sam3xa/source/system_sam3xa.c#L28

Internal schematics here:
sam3-3
https://f.cloud.github.com/assets/423515/1217403/a85268f0-26a5-11e3-98dc-bf232391a2c5.png

AOK. Got it ;-)

MCK==84M means that we must set the PRESCAL value to 2, because

ADCClock = MCK / ( (PRESCAL+1) * 2 ) = 84M / ((2+1)*2 = 14M

using 1 we get:

ADCClock = MCK / ( (PRESCAL+1) * 2 ) = 84M / ((1+1)*2 = 21M > 20M (as
per datasheet specification)

do you agree until now?

Yes. I see now.

I agree that the conversion time is 20 ADC clocks, but that's not
relevant to the STARTUP time.

If ADCClock is 14MHz (I agree now), then:

 STARTUP = lookup_in_table(ADC_STARTUP_NORM / ADCClock_cycle)

ADC_STARTUP_NORM is 40 (microseconds)
in hardware/arduino/sam/system/libsam/include/adc.h
also as per max time from "off" in the specs on Table 46-28. Channel
Conversion Time and ADC Clock

 ADCClock_cycle = 1 / ADCClock

i.e., 71.143 ns

so we should lookup the appropriate value for (40 microsec / 71.143 ns) or

 562.25

So STARTUP should be "9" - i.e., 576 cycles.

8 is too small (512 cycles), and 3 (24 cycles) is way too small.

AFAICT ;-)

Joe

[cmaglie]

Joe,

you're right, I misinterpreted that part of the datasheet and totally agree with you now.

Looking at the adc_init function I see also that is buggy and it fails calculations:

https://github.com/arduino/Arduino/blob/ide-1.5.x/hardware/arduino/sam/system/libsam/source/adc.c#L84

uint32_t adc_init(Adc *p_adc, const uint32_t ul_mck, const uint32_t ul_adc_clock,
   const uint32_t ul_startuptime)
{
   uint32_t ul_prescal, ul_startup;
   p_adc->ADC_CR = ADC_CR_SWRST;

   /* Reset Mode Register. */
   p_adc->ADC_MR = 0;

   /* Reset PDC transfer. */
   p_adc->ADC_PTCR = (ADC_PTCR_RXTDIS | ADC_PTCR_TXTDIS);
   p_adc->ADC_RCR = 0;
   p_adc->ADC_RNCR = 0;
   ul_prescal = ul_mck / (2 * ul_adc_clock) - 1;
   ul_startup = ((ul_adc_clock / 1000000) * ul_startuptime / 8) - 1;
   p_adc->ADC_MR |= ADC_MR_PRESCAL(ul_prescal) |
         ((ul_startup << ADC_MR_STARTUP_Pos) &
         ADC_MR_STARTUP_Msk);
   return 0;
}

in our case:

adc_init(ADC, SystemCoreClock, ADC_FREQ_MAX, ADC_STARTUP_FAST);

it will set ul_precal to = ul_mck / (2 * ul_adc_clock) - 1 = 84M / (2 * 20M) -1 = 1
where it should be 2, and also the ul_startup will be wrong because it uses a linear scale of 8 instead of the conversion table... ARGH.

Besides that, remains the last question: if this is the "startup" time, it should be applied only to the first ADC conversion, while the following should run at the maximum speed allowing a theoretical conversion rate of 700K in our case right?

C

[jtouch]

Hi, Cristian,

On 9/26/2013 7:38 AM, Cristian Maglie wrote:

Joe,

you're right, I misinterpreted that part of the datasheet and totally
agree with you now.

We both learned a lot in this process, IMO ;-)

Looking at the adc_init function I see also that is buggy and it fails
calculations:

https://github.com/arduino/Arduino/blob/ide-1.5.x/hardware/arduino/sam/system/libsam/source/adc.c#L84

|uint32_t adc_init(Adc *p_adc, const uint32_t ul_mck, const uint32_t ul_adc_clock,
const uint32_t ul_startuptime)
{
uint32_t ul_prescal, ul_startup;
p_adc->ADC_CR = ADC_CR_SWRST;

/* Reset Mode Register. */
p_adc->ADC_MR = 0;

/* Reset PDC transfer. */
p_adc->ADC_PTCR = (ADC_PTCR_RXTDIS | ADC_PTCR_TXTDIS);
p_adc->ADC_RCR = 0;
p_adc->ADC_RNCR = 0;
ul_prescal = ul_mck / (2 * ul_adc_clock) - 1;
ul_startup = ((ul_adc_clock / 1000000) * ul_startuptime / 8) - 1;
p_adc->ADC_MR |= ADC_MR_PRESCAL(ul_prescal) |
      ((ul_startup << ADC_MR_STARTUP_Pos) &
      ADC_MR_STARTUP_Msk);
return 0;

}
|

in our case:

|adc_init(ADC, SystemCoreClock, ADC_FREQ_MAX, ADC_STARTUP_FAST);
|

it will set ul_precal to = ul_mck / (2 * ul_adc_clock) - 1 = 84M / (2 *
20M) -1 = 1
where it should be 2, and also the ul_startup will be wrong because it
uses a linear scale of 8 instead of the conversion table... ARGH.

Right; that's what threw up a red flag to me; I expected the table.

Besides that, remains the last question: if this is the "startup" time,
it should be applied only to the first ADC conversion, while the
following should run at the maximum speed allowing a theoretical
conversion rate of 700K in our case right?

AFAICT, yes - and there should be one of two different startups
depending on whether it's a cold start or wakeup from sleep, but I don't
suspect the sleep function is typically used much.

Joe

C

—
Reply to this email directly or view it on GitHub
#1418 (comment)
Bug from MailScannerWebBug

[cmaglie]

I've pushed a fix to the adc_init() function, now the register configuration is correct, but the gain in ADC performance is gone (it runs almost at the same speed as before).

I see that the analogRead() function enable and disable the ADC each time is called, may this force the ADC to always wait the startup time causing the poor conversion speed?

C