Is this against C standard?

I had troubles getting two things working, until I rewrote the code.

1. sizeof() an array

I have declared an array like this:

uint16_t main_theme[] = {
  B4, AISS4, A4, GISS4, G4, C5, B4, AISS4, END
};

Those macros are just uint16_t numbers everyone (frequencies to be played). Later I play those tones with this:

void play_melody(uint16_t *mel)
{
  for (uint16_t *p = mel; *p != END; p++)
  {
    if (*p != Z)
      carrier.Buzzer.sound(*p);
    delay(TONEDELAY); 
    carrier.Buzzer.noSound();
    delay(TONEOFFTIME);
  }
}

That works quite well. TONEDELAY is a value of 70 and TONEOFFTIME is 3. In my code I call the function like this:

  if (soundOn)
    play_melody(main_theme);
  else
    delay((TONEDELAY + TONEOFFTIME) * (sizeof(main_theme) - 2) / 2);

Everything works like a charm when I have the sound on. But when the sound is off and the delay is supposed to happen, which should last as long as the melody, the delay is not calculated. Each element except the terminating END in the array is a tone that lasts 70 milliseconds (TONEDELAY), after that there's a 3 millisecond silence (TONEOFFTIME). I expect sizeof() to give the total number of bytes in the array. (I take 2 off for the terminating array element, then divide by two to get the number of uint16_t elements).
The strange thing is that declaring the array with uint16_t main_theme[] = { will make sizeof() go lost. It couldn't see the size of the array, it sounded like it just gave me the size of one uint16_t. I changed the code to uint16_t main_theme[9] = { and now it worked. To check whether I found the solution I changed back to uint16_t main_theme[ ] = {. It still worked! It didn't go back to treating the array as if it had only one element. I repeated the thing with another array. Same happened! uint16_t new_ball_melody[ ] = { didn't work. Changing to uint16_t new_ball_melody[13] = { made it work. Changing back to uint16_t new_ball_melody[ ] = { still working!
My theory is that the compiler doesn't do its job. First, when it sees sizeof(new_ball_melody), it fails to count the number of elements in the following ={} thing. It just goes with the datatype. Then when I add the array size, it does its job. It doesn't have to check the {} listing, it just relys on what's in [ ]. And when I change back to [ ] without telling the number of elements, the compiler does a peculiar choice to neglect the empty [ ], probably because now there's no info about a changed array size, so it goes with the size from the previous compilation.

2. char *name = "JOHAN"

I declare a string like char *name = "JOHAN". Then I have a function which should alter the string:

void alter(char *strng)
{
    ... // stuff like strng[1] = 'G';
}

Everything compiles. But program gets freezed. Somehow I doubted the memory handling here. So I changed it to:

char *name;
name = new char[8];
strcpy(name, "JOHAN");

And now it worked! What happened here? Did I come across two things, where the Arduino compiler doesn't obey the rules of C?

Highly unlikely. But impossible to confirm without seeing the exact code that caused issues.

You are not allowed to write to string literals. The compiler should have warned you about that, if it didn't, please go to the preferences in the IDE and enable all warnings.

String literal - cppreference.com :

Attempting to modify a string literal results in undefined behavior

Avoid naked calls to new at all costs. If you're going to dynamically allocate strings, use a wrapper like the String or std::string classes.

In this case, there's no reason for dynamic memory, just copy the string literal into a character array:

char name[] = "JOHAN";

Arduino uses C++, not C.
It compiles your code with a standard GCC C++ compiler, with GNU extensions enabled, and with the very unfortunate -fpermissive flag.

1 Like

Yes.

It is worth taking whatever time you need to understand, or asking whomever you trust most to explain the subtle difference between

and

which one could be forgiven for thinking were saying the same thing. Especially important once you get to pointers and arrays in their larger uses and how they are often exposed to new learners of C/C++.

a7

1 Like

regarding the array, if you declare a global variable like this,

uint16_t main_theme[] = { B4, AISS4, A4, GISS4, G4, C5, B4, AISS4, END};

you get the count of elements by doing

size_t mainThemeCount = sizeof main_theme / sizeof main_theme[0];

that is you count the number of bytes in the array, divided by the number of bytes used for the first entry.
if you don't want to count the END piece, just remove 1.

size_t mainThemeCount = (sizeof main_theme / sizeof main_theme[0]) - 1;

Now if you pass an array pointer to a function like play_melody(main_theme);, what happens is that the array decays into a pointer to the first entry of the array.

So the function is taking a uint16_* as a parameter and the size of the array is unknown to the function. So if in the function you ask for the sizeofof the parameter, you are asking for the number of bytes to represent a pointer, not the array. That's why you'll see 2 (or 4 on 32 bits microcontrollers)

does this help?

I do pass the array to the play function, but the problem doesn't happen in the play function. It happens in setup(), where I use the array just as a global. Could it be that the compiler turns a line into a function with a parameter, if the line occurs almost as the same many times? What I have here is:
delay((TONEDELAY + TONEOFFTIME) * (sizeof(end_game_melody) - 2) / 2);
I thought myself to turn this into a function:

void mute_delay(uint16_t *melody)
{
  delay((TONEDELAY + TONEOFFTIME) * (sizeof(melody) - 2) / 2);
}

That wouldn't have worked due to the array turning into a pointer, like you said. I know compilers do "clever" things like turning simple functions into written out code to avoid function calls if speed can be gained. Here the compiler might want to save memory (because we're writing code for microcontrollers, not computers). The line occurs 6 times in my sketch. It might save some bytes to write it as a function. If the compiler does that, it might have missed that sizeof() won't work in the function. I know it sounds stupid, but the behaviour seemed very stupid in the first place. And now that I've learned the string literal thing, I find it stupid that trying to write to such causes a warning, not an error. I bet I understand that, too, after some more learning.

As you refer explicitly to the global variable and this is calculated at compile time, no way there is a problem there.

How are your TONE constants defined? and what hardware are you running this on?

have you tried printing?
Serial.print((TONEDELAY + TONEOFFTIME) * (sizeof(end_game_melody) - 2) / 2);

this way you'll know what's the calculated value

This topic was automatically closed 120 days after the last reply. New replies are no longer allowed.