Jump to content
43oh

Strange problem: LED1 and LED2 react differently to same code


Recommended Posts

Hey guys, this is my first post here.  I'm new to micros and I've recently dived into the tutorials on the MSP430 Wiki page.  Very helpful stuff!  

 

Before I go on, I'd like to let you guys know that I am using the LaunchPad development board and using the MSP430G2553 chip.

 

I started with http://processors.wiki.ti.com/index.php/Blink_your_first_LED and recently this morning I got both LED1 LED2 to blink (the LEDs on the LaunchPad board itself) at the same time.  LED1 is connected to pin P1.0 and LED2 is connected to P1.6

 

My problem is when I'm trying to get the LEDs to blink alternatively (LED1 on, then off, LED2 on, then off).  It's not too difficult stuff, and I only added one line of code to the existing Wiki's code.  Here's the code itself before I go on.

 

#include <msp430.h>

unsigned int i = 0; //initialize variable "i"; used as a counter

void main(void)
{
	WDTCTL = WDTPW + WDTHOLD; //stops watchdog timer

	P1DIR |= 0x41;

	for(;
	{
		P1OUT ^= 0x40;
		for(i=0; i < 5000; i++); //playing around with the i value gives me seperate blinks or unison-blinks.
		P1OUT ^= 0x01;
	}
}

 

As you can see, to achieve the effect of alternative blinking, my "for(;;)" loop begins by setting LED2 high, waits whatever the max "i" value is, then turns on LED1 while quickly ending the for(; ;-) loop and starting back at the top, in which LED2 is now off (while LED1 is still on).  I guess there's a slight overlap when LED1 turns on and goes back to the top of the for(; ;-) loop; but it's probably negligible in human standards.  I still feel uncomfortable with it, but it's the only thing I could conjure up.  

 

My first question is: is this an effective strategy?  Are there better ways to do it?  I'm an absolute beginner in programming, so I'm sure I'll eventually find better ways as I keep learning, but I'd like to ask the experts beforehand anyways.

 

My main question involves the for(;; ) loop.

 

For my "for(i = 0; i < #; i++)" statement, the LEDs either blink on and off at the same time, or the process goes normally and the LEDs blink alternatively like they're supposed to.

 

In example, I set "i < 5000" and I compile and run the code.  The LEDs blink quickly in pattern, but success overall.  Then I would set "i < 10000" and for some odd reason, the LEDs blink in unison (not in pattern).  Then I'd try "i < 20000" and the LEDs would blink in the great pattern like I'd expect.  I played around with numerous numbers and found that the problem is completely random.  I can't find any correlation to why some numbers worked the way I wanted, while other numbers failed me.  I thought there was some borderline number I could come close to, in which that borderline number has something to do with the microcontroller itself; something that was beyond my scope of knowledge at the moment.  In example, "i < 10000" blinked the LEDs in unison while "i < 10001" worked the way I wanted. 

 

I'm extremely frustrated and do not understand why changing this value affects my blinking.  Isn't this only an incrementer type of thing?  Simply like a counter?  Why is this affecting the whole code itself?

 

Thank you for any future responses.  Looking forward to my time here at 43oh! 

Link to post
Share on other sites

I am no 'expert' by a long shot (IANEBALS).

 

Perhaps the line I quoted below is causing you a problem? Instead of 'i < 5000' - set another variable 'wait' and assign it the value for the number of steps you want (5000 or 10000 or 20000) and then change your code to:

 

**put this before your void main**

unsigned int wait = 5000; //initialize variable "wait"; used as a duration - can change this to 10000 or 20000 if you like

 

**replace your 'for' line with this one**

for(i=0; i < wait; i++); //playing around with the i (would be 'wait' instead of 'i' after the '<') value gives me seperate blinks or unison-blinks.

 

I could be completely wrong. There may be a bug in your compiler (would have to read any errata document, if there is one). I didn't see anything that defined which chip you are using, in your code. Perhaps the lack of defining your exact chip is causing the compiler to randomly pick one when you compile and may be choosing a different one each time you compile (would explain the randomness of the symptom). Maybe define your chip specifically in the code itself. Some chips don't do things the same way other chips do.

 

I notice that you nested a 'for' statement within a 'for (;;)'. Could that be the issue? I am not fluent with CCS at all. 'for (;;)' looks like a function and I guess that you chose to name that function 'for'. 'for (;;)' is maybe not the best choice to name it. Maybe change that name to 'myforfunction(;;)'.

 

		for(i=0; i < 5000; i++); //playing around with the i value gives me seperate blinks or unison-blinks.
Link to post
Share on other sites

Hi Potatochan,

 

looking at your code the problem you have is not to do with the for loop (which you are using as a software delay) but with the fact that your port isn't in a "known state"..  In pseudo code you have....

 

Put the WatchDogTimer on hold.. (Tell the watch dog.. Go on hold. If you don't do this your 430 will reset regularly)

Tell the 430 that in Port 1 bits 0 and 6 are used for output.

Loop forever    

    Toggle bit 6 of port 1      If the LED on bit 6 is on turn it off, if its off turn it on    

    count to 5000                Counting is purely as a delay    

    Toggle bit 0 of port 1      As with the other toggle.. but for the other LED

Continue the loop

 

As you can see.. all the interactions with P1OUT are dependent on what was there before.  You noticed them in sync and not in sync not because of what I was looping to, but because of what they were on before.

 

Im guessing you run the code (maybe stepping through) then edited the number, re-uploaded and run the code.  Depending on where you stopped the code the LEDs could have been on or off semi randomly.  The solution is to put the port into a known state.  After the line that sets P1DIR your problem would be solved if you put in P1OUT = 0; Which says take every bit low.

 

As to the counting loop being optimised out as pabigot said.. think about when you played Hide and Seek as a child.. (Im hoping this game is universal :smile:) The game starts with 1 child counting to a number maybe 100.  This was purely to give a delay before they started hunting.  Now some children counted slow and some counted fast, and some were clever and thought that the counting achieved nothing and could be optimised to "1, 2, skip a few, 99 100".

 

The compiler when in release mode can optimise your code and decide "That counting achieves nothing so I can safely ignore it".. so your code behaves differently in release mode from debug mode.  The solution here is to use __delay_cycles(xx) where xx is the number of cycles to pause.  At 1MHz using __delay_cycles(1000000) will always wait 1 second, __delay_cycles(100000) will always delay 1/10th of a second.  I can't remember the default speed of the 430 I think its either 1MHz or 1.1MHz.. I always set the clock to one of the calibrated speeds at the sametime as I put the WDT in hold mode.  The lines to do this are....

 

BCSCTL1 = CALBC1_1MHZ; // Set range
DCOCTL = CALDCO_1MHZ;  // Set DCO step and modulation
 

 

The way I would have written your code would be....

 

#include <msp430.h>

#define LED_RED			BIT0			// The Red LED is on P1.0
#define LED_GREEN		BIT6			// Green LED is P1.6

void main(void) {
	WDTCTL = WDTPW + WDTHOLD;

	BCSCTL1 = CALBC1_1MHZ;			// Tell the 430 to run at a calibrated
	DCOCTL = CALDCO_1MHZ;			// Clk speed of 1MHz

	P1DIR = LED_RED + LED_GREEN;		// We want outputs on Red and Green LEDs only
	P1OUT = 0;						// Start with both LEDs off

	while(1)
	{
		P1OUT ^= LED_GREEN;			// Toggle state of the green LED
		__delay_cycles(500000);		// Delay for 1/2 a second (clock is at a known 1MHz)
		P1OUT ^= LED_RED;			// Toggle the red LED
	}
}

 

I think this code would do what you want.  Things to notice are...

 

1) The #defines at the top.  These tell the compiler "Whenver you see LED_RED in the code replace it with BIT0". BIT0 is itself a #define for 0x1, BIT6 is a #define for 0x40.  These take a little longer than using 0x41 instead of LED_RED + LED_GREEN, but this delay is done by the compiler on your PC before the code gets to your 430.  So the 430 gets the exact same code as with what you had written and so runs with exactly the same efficiency, but for someone reading your code it is obvious which LED is being toggled at which line.

 

2) Setting the clock to calibrated speeds.  The 430 has a very flexible clocking system that run run the CPU at almost any speed upto 16MHz, the devices have certain speeds calibrated into them at the factory, i.e. the MSP430G2553 has 1MHz, 8MHz and 16MHz if I remember correctly.  We are saying run at a known speed of 1MHz.  If you wanted 16MHz use CALBC1_16MHZ and CALDCO_16MHZ.  If you did this though you would have to alter your __delay_cycles line to __delay_cycles(8000000) for a 1/2 second delay.

 

3) Changing your for(;; ;-) to while(1).. This one is personal taste.  Each says "Loop forever".  Personally I think the while(1) is easier to understand but many many other people prefer the for(; ;-) method so you will see both in examples round the net.

 

I meant this as a quick response and got carried away as usual..  I hope it helped.

 

Dan

 

P.S. One further thing to add.  Putting the CPU into a known good state is a good practice to get into.  For example when you start using interrupts each interrupt has an interrupt flag that says this interrupt has occured.  Good practice says to always clear this flag before enabling the interrupt.  It is a good habit to get into and saves much hair pulling, keyboard bashing later on (I know.. I learned this one the hard way)

Link to post
Share on other sites

Dan, thank you for the excellent explanation.  I learned a great deal.  You are exactly right about my method; I was changing the " i < 5000 " value and hit debug + run which created the problem you explained.  I added the "P1OUT = 0x00" statement and it fixed the issue immediately.  Any numerical value I put for " i < ### " causes LED1 and LED2 to blink in the alternating pattern.  

 

Thanks for including more important concepts for me to learn as well.  When you say

 

"

The compiler when in release mode can optimise your code and decide "That counting achieves nothing so I can safely ignore it".. so your code behaves differently in release mode from debug mode.  The solution here is to use __delay_cycles(xx) where xx is the number of cycles to pause.  At 1MHz using __delay_cycles(1000000) will always wait 1 second, __delay_cycles(100000) will always delay 1/10th of a second.  I can't remember the default speed of the 430 I think its either 1MHz or 1.1MHz.. I always set the clock to one of the calibrated speeds at the sametime as I put the WDT in hold mode.  The lines to do this are....

 

 

What does it mean when the compiler is in release mode?  Is this another way of describing the code itself when it is implemented into the microcontroller chip itself?  Sorry, just a little confused with the technical vocabulary, I'm sure I'll begin getting used to it.  Also, I'm a bit confused about why the clock speed must be calibrated.  I understand now that the compiler may optimize my code and decide the "for(i = 0; i < 10000; i++)" statement is unnecessary (thus deleting it).  Is it just a better idea to use the clock delays to have the delay-effect?  For some reason, my code has not been optimized.  When you say the compiler will ignore it, how come it didn't ignore it in my particular circumstance?

 

Thanks everyone for helping a beginner like myself.  It is greatly appreciated.

Link to post
Share on other sites

In CCE there are two compile modes.  Debug and Release.  I believe you find the option to switch them by long pressing the compile button (left of the debug button).

 

The clock speed doesn't *have* to be calibrated...it's a "best practice" sort of thing.  It is set to a default value that is listed in the data sheet....but relying on default values is not a great idea.  Say for instance you decide to port this code to a different chip that happens to default to a different clock speed.  This would present itself as a bug in the system, and could be somewhat difficult to track down unless you happened to document that you were assuming a default clock speed of xxx.  The simple way out of this and many other issues is to just initialize things to the default values you want...even if the statements don't actually change anything.

 

If I remember correctly, __delay_cycles() uses assembler primitives to do the delay, so it is as optimized as you are going to get and should never be optimized out by a compiler.  If you must delay like that, it is the way to go.  The downside is that delayed cycles are wasted cycles.  The (arguably) better approach would be to use a timer and interrupts, which would allow the MCU to continue processing or go to sleep during the delay. 

 

I'm not sure why the compiler didn't optimize the loop away....could be the compiler...could be the level of optimization selected....Compilers sort of have personalities when it comes to optimization.  The best way to avoid issues is to be very specific about what you want to happen.

Link to post
Share on other sites

The variable used is a non-static global, so the compiler won't optimize it away because it could be referenced in other object files (created from other source files that the compiler has no immediate knowledge of).

I don't believe there's any implicit "volatile" qualifier simply because an object is declared with global scope.  If the locally visible declaration is not marked as volatile, and the code sequence does not invoke operations that are not defined within the translation unit, the compiler may be entitled to decide it knows everything about how the value is accessed and therefore optimize it in any way consistent with the visible manipulation of the data: in this case, eliminating the loop.  Whether any particular compiler makes that assumption is separate from whether it's allowed to make that assumption.  As mbeals suggests, the best solution is to be explicit about your intent.

Link to post
Share on other sites

Not suggesting an implicit volatile.

 

Also not suggesting that a for() loop should be used as a delay - no matter how the loop var is declared.

 

So...

 

 

int x; // global
 
void foo(void)
{
  ++x;
}

 

Should ++x be optimized away?  When foo() is called it is expected that x will increment. How would the compiler know that the delay loop (in the code in the first post) isn't doing something important with the global variable?

Link to post
Share on other sites

Not suggesting an implicit volatile.

 

Also not suggesting that a for() loop should be used as a delay - no matter how the loop var is declared.

 

So...

 

 

int x; // global
 
void foo(void)
{
  ++x;
}

 

Should ++x be optimized away?  When foo() is called it is expected that x will increment. How would the compiler know that the delay loop (in the code in the first post) isn't doing something important with the global variable?

No, I agree that shouldn't be optimized away.  However, in the case of a for loop, the compiler should be allowed to replace the loop with a single assignment that stores the final value of the variable.

Link to post
Share on other sites

I disagree! In your example the compiler is completely free to optimize x away, unless x is read at another place and that read cannot be optimized away!

int x;

void foo(void)
{
  ++x;
}

void bar(void)
{
  if (x == 0)
  {
  }
  else
  {
  }
}

Even in this case, the if-clause in bar can be optimized away, so so can the increment.

I'm not saying that your compiler will do this (most won't), but it's free to do so if it detects the redundancy.

Link to post
Share on other sites

Join the conversation

You can post now and register later. If you have an account, sign in now to post with your account.

Guest
Reply to this topic...

×   Pasted as rich text.   Paste as plain text instead

  Only 75 emoji are allowed.

×   Your link has been automatically embedded.   Display as a link instead

×   Your previous content has been restored.   Clear editor

×   You cannot paste images directly. Upload or insert images from URL.

×
×
  • Create New...