Jump to content
Sign in to follow this  
igendel

Simplest code, oddest behavior

Recommended Posts

Hi all,

 

First, I apologize if this is the wrong forum for this - but it's almost 3 AM here and I'm beginning to lose sanity.

 

I wrote a short "bare-bone" code to test the new Energia with an out-of-the-box MSP430 Launchpad (2553 chip). Here's the code:

#include <msp430G2553.h>

main() {
  
 int i = 0;
  
  WDTCTL = WDTPW + WDTHOLD;
  P1DIR = 65;
  P1OUT = 0;
  
  while (1) {
    i++;
    if (i % 2000 == 0) { 
      P1OUT ^= 1;
      if (P1OUT & 1) P1OUT ^= 64;
      i = 0; 
    }
  }
}

Nothing fancy, blinks the LEDs as expected*. However, when I change the "i % 2000 == 0" condition to what should be an exact equivalent, i.e.

    if (i == 2000) { 

It doesn't work anymore - the LEDs light up and stay on indefinitely!

 

So, what obvious point am I missing? Can someone reproduce this?

 

Thank you very much in advance!

 

* Of course, it's quite odd to begin with that a 16MHz chip will count to 2000 so slowly, that the blink will be noticable (~2sec cycle). The Modulu shouldn't take THAT long... to be certain, I tried changing the type of "i" to "long int" and let it run to 20000, 200000 or 2000000 - still nothing.

 

 

Share this post


Link to post
Share on other sites

The Modulu shouldn't take THAT long... to be certain

It is quite slow unless a power of 2 is used, for example n % 512

 

I tried changing the type of "i" to "long int" and let it run to 20000, 200000 or 2000000 - still nothing.

try 200000UL and 2000000UL

Share this post


Link to post
Share on other sites

It is quite slow unless a power of 2 is used, for example n % 512

 

 

try 200000UL and 2000000UL

 

I can indeed confirm the slowness of the % operator for non-2^n-numbers; it's quite fast for 1024 but terribly slow for 1025.

However, I still don't get a response for the "i == XXXX" condition - I tried many numbers with UL, and in Hex .

Share this post


Link to post
Share on other sites

First, the default clock speed is 1MHz, not 16MHz.

 

When you change to

 if (i == 2000) {

your LED is blinking, but it's blinking very fast.

 

try changing i to long and do i == 200000 or similar

Share this post


Link to post
Share on other sites

First, the default clock speed is 1MHz, not 16MHz.

 

When you change to

 if (i == 2000) {

your LED is blinking, but it's blinking very fast.

 

 

Oh, I thought it was 16MHz by default. Can that be arranged in code BTW, and/or do I need an external crystal for that?

 

Still, no matter what numbers I use, the LEDs stay on when testing with "==".

Now, here's another crazy behavior: when I tried

  while (1) {
    i++;
    if (i == 2000) { 
     if (!random(100)) P1OUT ^= 1;
     if (P1OUT & 1) P1OUT ^= 64;
      i = 0; 
    }
  }

It worked with the expected erratic pattern.

When I removed the random and simply increased the 2000 to 200000 (or 200000UL), again I got the steady ON.

[Edit: Yes, i is long]

Share this post


Link to post
Share on other sites

Try this:

#include <msp430G2553.h>

main() {

 long i = 0;

  WDTCTL = WDTPW + WDTHOLD;
//for 16MHz clock, uncomment following 2 lines
//    BCSCTL1 = CALBC1_16MHZ;
//    DCOCTL = CALDCO_16MHZ;

  P1DIR = 65;
  P1OUT = 0;

  while (1) {
    i++;
    if (i == 200000) {
      P1OUT ^= 1;
      if (P1OUT & 1) P1OUT ^= 64;
      i = 0;
    }
  }
}

Share this post


Link to post
Share on other sites

RobG - The two lines for 16MHz seems to work, thanks for that!

 

However, I copy-pasted your code as is and still don't get the expected blinking, Does it work for you? (again, on the latest Energia).

 

I have to go to sleep now, can't keep my eyes open anymore :shock: but I'm looking forward for more answers tomorrow!

Share this post


Link to post
Share on other sites

I can reproduce this behavior. It's not a bug in the compiler.

 

The modulo operation is treated as a function call, so the compiler executes it and does not optimize away i. The equality check is inlined, at which point the compiler determines that i is unnecessary and reduces the loop to:

  while (1) {
    P1OUT ^= 1;
    if (P1OUT & 1) P1OUT ^= 64;
  }
In short, it is blinking, but so fast you can't see it. This is the classic mistake of trying to implement a delay with a loop.

 

If you declare:

  volatile int i;
the blink returns and occurs much faster because the overhead of the modulo operation is no longer in the loop.

 

*EDIT* Which is essentially what Rob said, though less explicitly. CCS apparently doesn't optimize the loop away completely; mspgcc does not change its visible behavior just because the comparison is with 20000 instead of 2000.

Share this post


Link to post
Share on other sites

If you don't want to turn off compiler optimization (i.e. take away your -O? in your CFLAGS), you can declare int i as "volatile int i;" to avoid gcc optimize your compare 2000 check.

 

I am not sure how gcc optimize, but I compile your code w/ listing and found out gcc thinks you just want to toggle bit0 continuously so he optimize it and make it blinks way fast. And we think the LED is constant on.

 

You can think that gcc did a good job and had optimized your program for "speed". Now it blinks faster than you can see.

Share this post


Link to post
Share on other sites

BTW: Although you can add "volatile" to the loop variable (as I and simpleavr have both noted), to avoid leading you into bad habits I want to be clear that that's not the right way to solve this problem. Do something like the following, if you don't want to go to the trouble of configuring a timer to enforce the delay:

 

#include <msp430.h>

main() {
  WDTCTL = WDTPW + WDTHOLD;
  P1DIR = 65;
  P1OUT = 0;

  while (1) {
    __delay_cycles(500000LU);
    P1OUT ^= 1;
    if (P1OUT & 1) P1OUT ^= 64;
  }
}

Share this post


Link to post
Share on other sites

I wonder if Energia is using a too aggressive optimization level.

 

Volatile forces the compiler to store i in memory, which wastes memory and reduces performance vs. keeping i in a register.

 

IMO as long as a variable is used, whether in an operator or a function call, it shouldn't be optimized away. Almost any optimization like this will cause unintended and very hard to find errors.

Share this post


Link to post
Share on other sites
Nothing fancy, blinks the LEDs as expected*. However, when I change the "i % 2000 == 0" condition to what should be an exact equivalent, i.e.
    if (i == 2000) { 

It doesn't work anymore - the LEDs light up and stay on indefinitely!

This is not equivalent, if you want (nigh to) identical behaviour, you should set i to 0 as the first statement in this if-block. Otherwise i will count to 65535 until it wraps around, not until 2000.

Additionally, note that once every 32 cycles or so the interval is different in the old situation: the if-block is triggered at 2000 and any 2000-fold, so 4000, 6000.... 60000, 62000, 64000 and then 0. This means the interval between 64000 and 0 is not 2000 but 65536-64000=1536, so 25% shorter.

Also, your loop does not contain any delays or timer triggers (always use timers when doing time-dependent things). This means that the loop speed is determined by the instructions. You've just replaced the by far longest instruction (modulo; % ) by a single compare. You could very well have saved several dozens of instructions and maybe even a hundred instruction cycles by this simple optimisation! The result could be that the LED is still blinking, but too fast for the eye to recognise as blinking (just like you don't notice most tube lights to blink).

 

Of course, it's quite odd to begin with that a 16MHz chip will count to 2000 so slowly, that the blink will be noticable (~2sec cycle). The Modulu shouldn't take THAT long... to be certain, I tried changing the type of "i" to "long int" and let it run to 20000, 200000 or 2000000 - still nothing.

This could be a debugger issue. Did you run the application after flashing it in and restarting your launchpad without having the debugger in your IDE enabled?

Share this post


Link to post
Share on other sites

Hello again,

 

- Defining i as volatile does indeed "solve" the problem.

 

- I wrote "solve" in quotes because, like Chicken here, I find this a workaround for an overly aggressive optimizer. When microcoltrollers are concerned, it should be well within my mandate to do such tricks for cycle wasting... lame as they may be.

 

- Roadrunner84 - yes, of course % is not equivalent to ==, I meant to say that they are functionally the same, in this particular context, for the purpose of checking whether i reached 2000 (note that I did set i to 0 at the end of that if block).

 

- I wonder if the Arduino IDE has the same issue  ;-) will check soon. I know that loop-delaying is not Best Practice, but I'm happy this issue was discovered like this, and not in some complex project where debugging would have taken days...

 

Thanks all for the answers!

 

Update: the Arduino does not over-optimize this way. So, out of curiosity, where exactly do I find and change the compiler flags in Energia? It'll be interesting to see what effects it will have.

Share this post


Link to post
Share on other sites

You shouldn't use this "solution", the problem is that your code looks like this

  while (1) {
    i++;
    if (i == 2000) { 
      P1OUT ^= 1;
      if (P1OUT & 1) P1OUT ^= 64;
      i = 0; 
    }
  }

which is equivalent to

  while (1) {
    i++;
  }

in 1999 out of 2000 cases. It is totally legitimate for a compiler to optimise this "empty" loop away, therefore, you should use timers (or if you must, delays) to iterate this loop.

void main()
{
  // set up the LED
  // set up a timer
}

#pragma set this vector to your timer
__interrupt void timer_isr()
{
  i++;
  if (i == 2000)
  {
    //code
    i = 0;
  }
}

Share this post


Link to post
Share on other sites

Create an account or sign in to comment

You need to be a member in order to leave a comment

Create an account

Sign up for a new account in our community. It's easy!

Register a new account

Sign in

Already have an account? Sign in here.

Sign In Now
Sign in to follow this  

×