message and a warning regarding behavior in a FOR loop...

Go To Last Post
27 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I received the following following a build:

In teh FOR loop I have this:

 

And for 'adc_avg_array I have:

uint16_t adc_avg_array[16];		//16 element storage of ADC readings

I set up a 16 element array of unsigned integers as the ADC result is 12 bits.

 

As I try and figure out whats up, anyone have any ideas why I am getting this warning?  I follow Johans belief that Warnings are potential errors so I really would like this to build clean of course.

 

JIm

This topic has a solution.

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

Last Edited: Thu. Dec 10, 2020 - 05:51 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Looks like you should have < rather than <=.

C: i = "told you so";

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Gcc complains if you don’t have an integer index. Normally you’d use brackets with sizeof() eg sizeof(adc_avg_array)

Normally I’d use memset() to clear your array.
And rather than an average, maybe consider using a low pass filter. Requires less ram and is tunable for the response you want.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

cpluscon wrote:

Looks like you should have < rather than <=.

 

 

I tried your suggestion and I get the same warning back.....

 

<headscratch>

 

Jim

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

cpluscon wrote:
you should have < rather than <=

Indeed.

 

The size (length) of the array is 16 - so the highest index is 15

 

note that you could just do

memset( adc_avg_array, 0, sizeof adc_avg_array );  

 

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

jgmdesign wrote:
And for 'adc_avg_array I have:

uint16_t adc_avg_array[16];		//16 element storage of ADC readings

sizeof gives you the number of bytes occupied by the array - not the number of elements.

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

awneil wrote:

cpluscon wrote:
you should have < rather than <=

Indeed.

 

The size (length) of the array is 16 - so the highest index is 15

 

note that you could just do

memset( adc_avg_array, 0, sizeof adc_avg_array );  

 

 

Kartman wrote:

Gcc complains if you don’t have an integer index. Normally you’d use brackets with sizeof() eg sizeof(adc_avg_array)

Normally I’d use memset() to clear your array.

 

Ok, I can do that, but in teh other warning, in that spot I have:

 /********add the array elements together and then divide by size of array********/
    for(uint8_t i = 0; i < sizeof(adc_avg_array); i++)
        {
            adc_average_result = ((adc_average_result) + (adc_avg_array[i]));
        }

    adc_average_result = ((adc_average_result) / sizeof(adc_avg_array));		//calculate the average

YEah, the math is probably screwy but the warning points to:

 adc_average_result = ((adc_average_result) + (adc_avg_array[i]));

which I declared 'adc_average_result' as:

uint16_t adc_average_result;

so I dont know why the compiler is irritated.....

 

<more head scratching>

 

Jim

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I think #6 and #7 overlapped ?

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

awneil wrote:
sizeof gives you the number of bytes occupied by the array - not the number of elements.

 

Ahhhhh!

 

Ok, but if I declare the array as 16 integer elements, can I use Sizeof?

 

JIm

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

The usual idiom is:

sizeof(adc_avg_array) / sizeof(adc_avg_array[0])

 

But I would usually do

#define ADC_AVG_LEN 16

uint16_t adc_avg_array[ADC_AVG_LEN];	

 

EDIT

 

But the memset version works as previously shown - because that one does want the number of bytes.

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
Last Edited: Thu. Dec 10, 2020 - 05:41 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

awneil wrote:

I think #6 and #7 overlapped ?

 

Yes, somewhat.

 

I guess then I need to create a constant that can be compared against instead of using sizeof()?

 

JIm

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

LOL - another overlap!

 

laugh laugh laugh laugh 

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

awneil wrote:

LOL - another overlap!

 

laugh laugh laugh laugh 

At least the thoughts are on teh same track!

 

JIm

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

The solution seems to be a mix of cpluscon and awneil.

 

cpluscon was correct in I needed to remove the '=' as it would have had the for loop increment one too many times.

 

Andys idea of defining a constant to compare against cleared up the type conflict.

 

Thanks gents.  It builds....but does it work? surprise

 

JIm

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

awneil wrote:
The usual idiom is:

sizeof(adc_avg_array) / sizeof(adc_avg_array[0])

So I guess you could do:

#define SIZEOF_ARRAY(X) sizeof(X) / sizeof(X[0])

if you do a lot of this stuff ...

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

awneil wrote:

awneil wrote:
The usual idiom is:

sizeof(adc_avg_array) / sizeof(adc_avg_array[0])

So I guess you could do:

#define SIZEOF_ARRAY(X) sizeof(X) / sizeof(X[0])

if you do a lot of this stuff ...

Oh boy, here we go again...

 

I simply did:

#define adc_avg_len 16				//size of adc elements

and then did this:

for(uint8_t i = 0; i < sizeof(adc_avg_len); i++ )
        {
            adc_avg_array[i] = 0;		//write all 0's to each location in the array

        }

problem went away.

 

Once I get things going the way I want I can try everyone elses ideas...I need a stable foundation where I know the results before I start getting creative.

 

JIm

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

jgmdesign wrote:

I simply did:

#define adc_avg_len 16				//size of adc elements

and then did this:

for(uint8_t i = 0; i < sizeof(adc_avg_len); i++ )
        {
            adc_avg_array[i] = 0;		//write all 0's to each location in the array

        }

That may build, but won't work.

 

sizeof(adc_avg_len) will likely be 2 - ie, the size of one single int.

 

What you need there is:

for(uint8_t i = 0; i < adc_avg_len; i++ )
        {
            adc_avg_array[i] = 0;		//write all 0's to each location in the array

        }

ie, no sizeof.

 

I need a stable foundation where I know the results before I start getting creative.

+1

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

awneil wrote:

That may build, but won't work.

 

sizeof(adc_avg_len) will likely be 2 - ie, the size of one single int.

 

as I read this I realised my boo boo....live and learn.

 

JIm

I would rather attempt something great and fail, than attempt nothing and succeed - Fortune Cookie

 

"The critical shortage here is not stuff, but time." - Johan Ekdahl

 

"Step N is required before you can do step N+1!" - ka7ehk

 

"If you want a career with a known path - become an undertaker. Dead people don't sue!" - Kartman

"Why is there a "Highway to Hell" and only a "Stairway to Heaven"? A prediction of the expected traffic load?"  - Lee "theusch"

 

Speak sweetly. It makes your words easier to digest when at a later date you have to eat them ;-)  - Source Unknown

Please Read: Code-of-Conduct

Atmel Studio6.2/AS7, DipTrace, Quartus, MPLAB, RSLogix user

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

jgmdesign wrote:
so I dont know why the compiler is irritated.....
A linter produced a loss of sign message.

Another take :
 

#include <stdint.h>

#define lengthof(x) (sizeof(x)/sizeof(x[0]))

void out(uint16_t val);

int main() {

    uint16_t adc_average_result = 0;
    uint16_t adc_avg_array[16];		//16 element storage of ADC readings

     /********add the array elements together and then divide by size of array********/
    for(uint8_t i = 0; i < lengthof(adc_avg_array); i++)
        {
            adc_average_result = (uint16_t)(adc_average_result + adc_avg_array[i]);
        }

    adc_average_result = ((adc_average_result) / lengthof(adc_avg_array));		//calculate the average
    out(adc_average_result);
}

 


macro via C Programming/Arrays and strings - Wikibooks, open books for an open world

PC-lint Plus Online Demo - Gimpel Software - The Leader in Static Analysis for C and C++ with PC-lint Plus

 

"Dare to be naïve." - Buckminster Fuller

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
for(uint8_t i = 0; i < 16; i++)

In reality you can do it this way.  Some may object to the redundancy, and the risk of magic number discrepancy.  But it is correct, and reads well.  

 

Generally there are two approaches shown here for array count processing.  IMO, the sizeof approach should be reserved for arrays that aren't explicitly sized, and may grow or shrink for whatever reason:
 

int syntaxarray[] = {
// values
};
int nsyntaxarray = sizeof(syntaxarray)/sizeof(int);
//..
for(i=0;i<nsyntaxarray;i++)

In your present case, this is likely the most sensible.
 

#define NSAMPLES 16
uint16_t array[NSAMPLES];
//...
for(i=0;i<NSAMPLES;i++)

 

C: i = "told you so";

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I find this monster lurking in the depths of my code:

//
// Array Length Calculation
// Stolen from https://stackoverflow.com/questions/4415524/common-array-length-macro-for-c
//
#define ARRAY_LENGTH(arr) ((sizeof(arr)/sizeof(0[arr])) / ((size_t)(!(sizeof(arr) % sizeof(0[arr])))))

TLDR; For those who don't wish to read the original stackoverflow question.

If you pass in a pointer to an array instead of the actual array variable (a common mistake apparently) you'll get a division-by-zero error from the compiler.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

jgmdesign wrote:

awneil wrote:

 

That may build, but won't work.

 

sizeof(adc_avg_len) will likely be 2 - ie, the size of one single int.

 

as I read this I realised my boo boo....live and learn.

 

JIm

Coding before the design was complete (i hope)!

 

 

 

(Possum Lodge oath) Quando omni flunkus, moritati.

"I thought growing old would take longer"

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The sizeof(anything) is of type size_t, which is a positive integer. If a library function can return a negative size_t, it's a ssize_t.

 

How many bits it is depends on the machine/compiler/etc. I'm currently setup here for a 32 bits so I can't check for AVR. Seems unlikely it's as small as 8 bits.

 

It may not make sense to compare this with a value of another integral type, e.g. a uint8_t, certainly without a cast.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

jgmdesign wrote:

awneil wrote:
sizeof gives you the number of bytes occupied by the array - not the number of elements.

 

Ahhhhh!

 

Ok, but if I declare the array as 16 integer elements, can I use Sizeof?

 

JIm

Use (sizeof(array)/sizeof(the type of the array elements))

Last Edited: Fri. Dec 11, 2020 - 12:02 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Remember that sizeof gives you the declared size, not the occupied size. For example if you declare a 10 byte array to hold a string and you put a 5 byte string plus a null terminator, you still get sizeof returning 10. 

 

Jim

 

Until Black Lives Matter, we do not have "All Lives Matter"!

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Not an issue for AVR but, in general, sizeof will also include any padding ...

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

My first thought on reading this was "why are you quoting "Error List"? it usually tells you nothing". If you want to study a build error look at the Output tab where the complete error (over several lines) is shown - not some one line summary that offers no context.

 

BTW, like others here, I habitually use:

#define NUM_ELEMENTS(x) ( sizeof(x) / sizeof(x[0]) )

then use as:

typedef struct {
    int n;
    char c;
    long l;
} data_t;

data_t my_array[17];

...

  for (int i = 0; i < NUM_ELEMENTS(my_array); i++) {
      process( my_array[i] );
  }

of course the alternative is:

typedef struct {
    int n;
    char c;
    long l;
} data_t;

#define ARRY_SIZE 17

data_t my_array[ARRAY_SIZE];

...

  for (int i = 0; i < ARRAY_SIZE; i++) {
      process( my_array[i] );
  }