[En-Nut-Discussion] AVR ADC sample fails

[Ole Reinhardt]

Hi Harald,

> So far we at egnite never used the ADC routines, but regularly receive
> requests about its API functions. Now we decided to provide a sample,
> but miserably fail ourselves. :-(

It's me who introduced this code and as far as I can remember we used it
ourself without problem for a long time now.

> Using a simple loop of ADC-reads plus a NutSleep(10000) lets the
> NutSleep fail. It returns immediately. This look like a memory access
> problem and I looked into arch/avr/dev/adc.c and started wondering about
> a number of things, which I do not understand.

Could you please provide me your demo code in a private mail? I'll test
it on my Ethernut 1.3 board and will correct the code (if needed)
immediately.

> The allocation of ADC_buffer in ADCInit() is
>  ADC_buffer = NutHeapAlloc(sizeof(uint16_t) * ADC_BUF_SIZE + 2);
> 
> Immediately after this, ADCBufInit() is called, which contains:
> 
>  buf[_adc_buf_head] = 0;
>  buf[_adc_buf_tail] = 0;
> 
> The related defines are
> 
>  #define _adc_buf_head ADC_BUF_SIZE
>  #define _adc_buf_tail ADC_BUF_SIZE+1
> 
> Actually
> 
>  buf[_adc_buf_tail] = 0;
> 
> is
> 
>  buf[ADC_BUF_SIZE+1] = 0;
> 
> and will corrupt heap memory. Did I overlook something? This code is
> nearly 5 years old!

Indeed you found a bug :) The intention was to _not_ overwrite memory as
we (want) to allocate two more words than ADC_BUF_SIZE. But instead we
just allocate 2 more bytes! The line should be:

ADC_buffer = NutHeapAlloc(sizeof(uint16_t) * (ADC_BUF_SIZE + 2));

> Please tell me, that I'm not getting too old for this stuff,

You are getting too old for this stuff, but you are still young enough
to find very old bugs :)

> P.S.:
> 
> More things to wonder about. Like in
> 
> inline int ADCBufRead(uint16_t * buf, uint16_t * read)
> {
>     uint8_t tail, head;
>     tail = buf[_adc_buf_tail];
>     head = buf[_adc_buf_head];
> 
> Why the hell doesn't the compiler complain? buf[] is 16 bit, tail is 8
> bit, so the upper byte is lost.

Indeed it should! But this is no problem as the value of head and tail
will never be greater than 255 as long as ADC_BUF_SIZE will not be
larger.

> There are other things I still do not understand, but let's solve these
> two first.
> 
> IMHO, the problem with the buffer overwrite is typical for _overusing_
> preprocessor macros. To me
> 
>  buf[ADC_BUF_SIZE+1] = 0;
> 
> is much easier to read than
> 
>  buf[_adc_buf_tail] = 0;
> 
> Furthermore, the naming is most confusing. _abcd looks like a system
> variable, while ABCD looks like a macro. Last not least, defining macros
> 
>  #define _adc_buf_tail ADC_BUF_SIZE+1
> 
> without brackets may be very dangerous, because _adc_buf_tail*2 will
> give the wrong result.

Yes yes yes, you won! I reused some quite old code then, and never
touched the driver again. Indead the brackets are missing. But I'd still
vote for 

#define ADC_BUF_TAIL (ADC_BUF_SIZE+1)
[...]
buf[ADC_BUF_TAIL] = 0;

instead of using 

buf[ADC_BUF_SIZE+1] = 0;

Bye,

Ole

-- 
 _____________________________________________________________
|                                                             |
| Embedded-IT                                                 |
|                                                             |
| Ole Reinhardt        Tel. / Fax:        +49 (0)271  7420433 |
| Luisenstraße 29      Mobil:             +49 (0)177  7420433 |
| 57076 Siegen         eMail:    ole.reinhardt at embedded-it.de |
| Germany              Web:         http://www.embedded-it.de |
|_____________________________________________________________|