AVR ADC sample fails

[Ethernut]

Hi all,

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. :-(

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.

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!

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

Harald




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.

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.

_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion

[Ole Reinhardt-2]

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:        [phone] |
| Luisenstraße 29      Mobil:             [phone] |
| 57076 Siegen         eMail:    ole.reinhardt@... |
| Germany              Web:         http://www.embedded-it.de |
|_____________________________________________________________|

_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion

[Ethernut]

Ole Reinhardt wrote:

> 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));

This fixed the problem, NutSleep is working again. Many thanks for
looking into it.


>> 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 :)

My thanks from above are canceled. ;-)

Harald


Off topic:

> 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;

The disadvantage is, that something is hidden. What's the advantage then?



_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion

[Ole Reinhardt-2]

Hi!

> > 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;
>
> The disadvantage is, that something is hidden. What's the advantage then?

The advantage is that you know what this array index is used for. On the
other hand it's no good idea to save the head and tail pointer in the
buffer at all. It better should be solved as seperate variables. But
this design allows you to just pass the buffer array to a function
instead of several variables. Yes, we should better put all this stuff
in a struct.

Ole


--
 _____________________________________________________________
|                                                             |
| Embedded-IT                                                 |
|                                                             |
| Ole Reinhardt        Tel. / Fax:        [phone] |
| Luisenstraße 29      Mobil:             [phone] |
| 57076 Siegen         eMail:    ole.reinhardt@... |
| Germany              Web:         http://www.embedded-it.de |
|_____________________________________________________________|

_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion

[Ethernut]

Ole Reinhardt wrote:

>>> 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;
>> The disadvantage is, that something is hidden. What's the advantage then?
>
> The advantage is that you know what this array index is used for. On the
> other hand it's no good idea to save the head and tail pointer in the
> buffer at all. It better should be solved as seperate variables. But
> this design allows you to just pass the buffer array to a function
> instead of several variables. Yes, we should better put all this stuff
> in a struct.

I didn't understand really what's going on in this code, but now it
makes sense. I agree with both, that the special use of the 2 upper
elements requires the defines and that a structure would be easier to
understand.

Indeed, parameter passing consumes a lot of code space on the AVR.

Harald


_______________________________________________
http://lists.egnite.de/mailman/listinfo/en-nut-discussion