P1.3 button related, click registered only once

Arcady Trembovler

Other Parts Discussed in Thread: MSP430G2553

Okay, here's an easy one!

I wrote a program that whenever my launchpad gets its P1.3 button pressed, the leds flash - red, then green, 20 times..
For the sake of simplicity I wrote separate functions of lighting red led, and for lighting the green led..

VERSION #1: P1.3 isn't registering button presses after being pressed just once

#include <msp430.h>
#include "msp430g2553.h"

inline void blinkRedLED()
{
int originalP1OUT = P1OUT;
P1DIR = BIT0; // Set P1.0 to output direction
P1OUT = BIT0;
__delay_cycles(100000);
P1OUT = originalP1OUT;
}
inline void blinkGreenLED()
{
int originalP1OUT = P1OUT;
P1DIR = BIT6; // Set P1.6 to output direction
P1OUT = BIT6;
__delay_cycles(100000);
P1OUT = originalP1OUT;
}
inline void blinkAllLEDS()
{
int originalP1OUT = P1OUT;
P1DIR = 0x41; // Set P1.0 and P1.6 to output direction
P1OUT = 0x41;
__delay_cycles(100000);
P1OUT = originalP1OUT;
}
inline void stopAllLEDS()
{
P1DIR = 0x41; // Set P1.0 and P1.6 to output direction
P1OUT = 0x00;
}

int main(void)
{

WDTCTL = WDTPW + WDTHOLD;

BCSCTL1 = CALBC1_1MHZ;
DCOCTL = CALDCO_1MHZ;

__enable_interrupt();

P1OUT |= BIT3;
P1REN |= BIT3;
P1IE |= BIT3; // P1.3 interrupt enabled
P1IFG &= ~BIT3; // P1.3 IFG cleared
_BIS_SR(LPM4_bits + GIE); // Enter LPM4 w/interrupt
}

#pragma vector=PORT1_VECTOR
__interrupt void Port_1(void)
{
P1IFG &= ~BIT3; // P1.3 IFG cleared

unsigned char tick=0;
int i;

for(i=20;i>1;i--)
{

if (tick==0)
{
blinkRedLED();
}
else
{
blinkGreenLED();
}
tick ^= (tick || 1);

stopAllLEDS();
}

return;
}

For my great amusment, it worked, but just once - means if the lanuchpad senses I pressed P1.3, the leds will flash, it will exit the interrupt and will put the launchpad back to "sleep" so I suppose.. If the P1.3 then pressed again the launchpad wouldn't do anything (unless, ofcourse, it is restarted via the restart button).

I suppose that I should "restart" the button somehow inside the __interrupt... what do you think?

Another thing I noticed, when I make a little change in the main function - if I remove WDTCTL = WDTPW + WDTHOLD; the P1.3 works great (after function is done, if button is pressed again, the __interrupt is called) - but another problem arises: I used __delay_cycles(100000); when lighting the leds, now without this line - only my red led (P1.0) lights, the green isn't flashing at all...

VERSION #2: P1.3 is registering button presses after being pressed just once - but __delay_cycles(....) function isn't working well - so now leds aren't lighting properly..

#include <msp430.h>

#include "msp430g2553.h"

int main(void)
{

//WDTCTL = WDTPW + WDTHOLD; //difference is here!

BCSCTL1 = CALBC1_1MHZ;
DCOCTL = CALDCO_1MHZ;

__enable_interrupt();

P1OUT |= BIT3;
P1REN |= BIT3;
P1IE |= BIT3; // P1.3 interrupt enabled
P1IFG &= ~BIT3; // P1.3 IFG cleared
_BIS_SR(LPM4_bits + GIE); // Enter LPM4 w/interrupt
}

#pragma vector=PORT1_VECTOR
__interrupt void Port_1(void)
{
P1IFG &= ~BIT3; // P1.3 IFG cleared

unsigned char tick=0;
int i;

for(i=20;i>1;i--)
{

if (tick==0)
{
blinkRedLED();
}
else
{
blinkGreenLED();
}
tick ^= (tick || 1);

stopAllLEDS();
}

return;
}

How can I make the launchpad "catch" all the button presses AND flash the leds properly :-)

Thanks a bunch!!

over 13 years ago

0 OCY over 13 years ago

Expert 1620 points

Why did you set BIT3 of P1OUT? And why do you clear it?

0 Marco over 13 years ago

Expert 1480 points

Hello,

in version 2 you do not stop the watchdog timer (as you commented it out).

This means, that the watchdog continuously resets the device after the watchdog timeout is reached.

As you added a delay within "blinkRedLED": the Red LED is switched on, and during the "__delay_cycles(100000);" the MSP430 resets .

Regards Marco

0 Arcady Trembovler over 13 years ago in reply to OCY

Prodigy 85 points

OCY said:

Why did you set BIT3 of P1OUT? And why do you clear it?

do you mean why did I write the following?

P1OUT |= BIT3;
P1REN |= BIT3;
P1IE |= BIT3; // P1.3 interrupt enabled
P1IFG &= ~BIT3; // P1.3 IFG cleared
_BIS_SR(LPM4_bits + GIE); // Enter LPM4 w/interrupt

0 Arcady Trembovler over 13 years ago in reply to Marco

Prodigy 85 points

Hey Marco,

Yes, seems like you are really correct about that.

I tried to replace the __delay_cycles(100000) with an empty for(......) loop but that's when funny things start happening.. It flashes about 2 times (both red and green leds) but not 20 as I wanted.. I guess that this watchdog gets too sleepy and goes to take a nap in the middle of the for loop :))

I thought to disable the watchdog inside the functions that light the leds, and enable it back again after the whole for loop inside the __interrupt is performed.

Works like a charm! - any other ideas for improvment?

Here's my code:

#include <msp430.h>
#include "msp430g2553.h"

inline void blinkRedLED()
{
WDTCTL = WDTPW + WDTHOLD;
int originalP1OUT = P1OUT;

P1DIR = BIT0; // Set P1.0 to output direction
P1OUT = BIT0;
__delay_cycles(50000);

P1OUT = originalP1OUT;

}
inline void blinkGreenLED()
{
WDTCTL = WDTPW + WDTHOLD;
int originalP1OUT = P1OUT;

P1DIR = BIT6; // Set P1.6 to output direction
P1OUT = BIT6;
__delay_cycles(50000);

P1OUT = originalP1OUT;

}
inline void blinkAllLEDS()
{
int originalP1OUT = P1OUT;

P1DIR = 0x41; // Set P1.0 and P1.6 to output direction
P1OUT = 0x41;
__delay_cycles(50000);

P1OUT = originalP1OUT;
}
inline void stopAllLEDS()
{
P1DIR = 0x41; // Set P1.0 and P1.6 to output direction
P1OUT = 0x00;
}

int main(void)
{

//WDTCTL = WDTPW + WDTHOLD; // -->> watchdog should be off

BCSCTL1 = CALBC1_1MHZ;
DCOCTL = CALDCO_1MHZ;

__enable_interrupt();

P1OUT |= BIT3;
P1REN |= BIT3;
P1IE |= BIT3; // P1.3 interrupt enabled
P1IFG &= ~BIT3; // P1.3 IFG cleared
_BIS_SR(LPM4_bits + GIE); // Enter LPM4 w/interrupt
}

#pragma vector=PORT1_VECTOR
__interrupt void Port_1(void)
{
P1IFG &= ~BIT3; // P1.3 IFG cleared

unsigned char tick=0;
volatile int i;

for(i=20;i>1;i--)
{

if (tick==0)
{
blinkRedLED();
}
else
{
blinkGreenLED();
}
tick ^= (tick || 1);

stopAllLEDS();
}

WDTCTL = WDTPW+WDTCNTCL; //turning the watchdog back on after whole loop was performed
return;
}

0 Jens-Michael Gross over 13 years ago

Guru 227245 points

Arcady Trembovler said:
P1OUT |= BIT3;
P1REN |= BIT3;
P1IE |= BIT3; // P1.3 interrupt enabled
P1IFG &= ~BIT3; // P1.3 IFG cleared

You should first clear teh IFG bit, then set the IE bit, or else any pending interrupt will be handled as soon as you set P1IE.

Then, it's a very, very bad idea to do things like delay_cycles or loops or anythign else that wastes time inside an ISR. For these tasks, use a timer and handle the next stepo in the timer interrupt. For more complex tasks, use a state machine that executes the next step on the next timer interrupt.

Arcady Trembovler said:
tick ^= (tick || 1);

What is this intended to do? (tick||1) will always be 'true' (logic OR, not binary).

Arcady Trembovler said:
unsigned char tick=0;

So tick will be 0 on each interrupt. if you want it to keep the last interrupt state, it must be declared static.

Arcady Trembovler said:
inline void stopAllLEDS()
{
P1DIR = 0x41; // Set P1.0 and P1.6 to output direction
P1OUT = 0x00;
}

the last instruciton does as side-effect turn the pullup resistor into a pulldown resistor. Even if you release the button now, the signal will naver rise again and therefore subsequent button presses cannot be detected - the input is tied to GND and the button-press virtually takes forever.

If you want to set individual bits in a port, use a |= BITb. if you want to clear them, use a &= ~(BITb).
Do not use = on registers with independent bits. It will change them all.

0 old_cow_yellow over 13 years ago in reply to Arcady Trembovler

Guru 58965 points

Arcady Trembovler said:

Why did you set BIT3 of P1OUT? And why do you clear it?

do you mean why did I write the following?

P1OUT |= BIT3;
P1REN |= BIT3;
P1IE |= BIT3; // P1.3 interrupt enabled
P1IFG &= ~BIT3; // P1.3 IFG cleared
_BIS_SR(LPM4_bits + GIE); // Enter LPM4 w/interrupt

[/quote]

Yes, I mean that. I asked that question to find out if you know the reason behind it.

I also asked "And why do you clear it?" You cleared BIT3 of P1OUT in four other places and they should not have done that giving the reason behind the first question.

I think if you take your first version of the code and change four lines, it will work.

(1) P1OUT = BIT0; ==> P1OUT = BIT0 | BIT3;

(2) P1OUT = BIT6; ==> P1OUT = BIT6 | BIT3;

(3) P1OUT = 0x41; ==> P1OUT = 0x41 | BIT3;

(4) P1OUT = 0; ==> P1OUT = BIT3;

0 Marco over 13 years ago in reply to Arcady Trembovler

Expert 1480 points

Hello,

this also means that you have to understand how a watchdog works.

There are 2 ways:

1.) disable watchdog and ignore it

2.) enable watchdog, and as soon as the software forgets to feed the dog, the watchdog will attack the MSP430 (resets the chip)

Regards Marco

0 Arcady Trembovler over 13 years ago in reply to Jens-Michael Gross

Prodigy 85 points

Jens-Michael Gross said:
P1OUT |= BIT3;
P1REN |= BIT3;
P1IE |= BIT3; // P1.3 interrupt enabled
P1IFG &= ~BIT3; // P1.3 IFG cleared
[/quote]
Agreed and DONE!

Jens-Michael Gross said:
You should first clear teh IFG bit, then set the IE bit, or else any pending interrupt will be handled as soon as you set P1IE.

Agreed and DONE!

Jens-Michael Gross said:
Then, it's a very, very bad idea to do things like delay_cycles or loops or anythign else that wastes time inside an ISR. For these tasks, use a timer and handle the next stepo in the timer interrupt. For more complex tasks, use a state machine that executes the next step on the next timer interrupt.

I had an awful lot of trouble using a timer, because apparently - when the "time is right" it launches an interrupt.. but in my example, it seems not to work when I try to do nest an interrupt (timer) into interrupt (when p1.3 button is pressed) - my intuition says I'm doing something wrong..

Jens-Michael Gross said:
tick ^= (tick || 1);
What is this intended to do? (tick||1) will always be 'true' (logic OR, not binary).[/quote]
It was supposed to do "not" on the expression in tick.. I agree it was totally stupid - so I replaced it with tick ^= 1

Jens-Michael Gross said:
inline void stopAllLEDS(){
P1DIR = 0x41; // Set P1.0 and P1.6 to output direction
P1OUT = 0x00;
}

the last instruciton does as side-effect turn the pullup resistor into a pulldown resistor. Even if you release the button now, the signal will naver rise again and therefore subsequent button presses cannot be detected - the input is tied to GND and the button-press virtually takes forever.

If you want to set individual bits in a port, use a |= BITb. if you want to clear them, use a &= ~(BITb).
Do not use = on registers with independent bits. It will change them all.[/quote]
Agreed and DONE!

0 Jens-Michael Gross over 13 years ago in reply to Arcady Trembovler

Guru 227245 points

Arcady Trembovler said:
I had an awful lot of trouble using a timer, because apparently - when the "time is right" it launches an interrupt.. but in my example, it seems not to work when I try to do nest an interrupt (timer) into interrupt (when p1.3 button is pressed) - my intuition says I'm doing something wrong..

When an ISR is executed, the GIE bit is cleared and no other interrupt will be handled.
It is possible to set GIE again whiel still inside the ISR, but this is very dangerous. It may result in the same ISR being alled over and over again (if the corresponding IFG bit wasn't cleared yet), the ISR interruptign itself and various otehr unwanted effects. Most people who try this end up spending most of their time fighting stack overflows and other 'unexplainable' behavior. So unless you're a very experienced MSP coder, you should NOT do this and rather re-organize your program flow. Almost every situation can be solved without interrupt nesting. (one exception could be a premptive task scheduler when all tasks are waiting for an event)

In my suggestion of using a timer, I didn't mean waiting in the port ISR for the tiemr to expire. I meant start a timer and then immediately exit the port ISR. And when the timer expires, the timer ISR does what has to be done after the delay. You may set a global (volatile) variable to tell the tiemr ISR which action to do when the timer expires. So the timer ISR can even retrigger itself for the next action of a chain, if mroe thanm one delay is needed. This is called a state-machine. In teh meantime, the MSp can sleep or handle other unrelated interrupts.

BTW: 'holding' the watchdog doesn't reset the watchdog counter. It just suspends it. So if you 'unhold' it, it will continue towards timeput where it was before. Eventually timing out and resettign the MSP. Unless you also set the WDT clear bit. In regular intervals smaller than the WDT timeout.

**Attention** This is a public forum

MSP low-power microcontrollers

MSP low-power microcontroller forum

P1.3 button related, click registered only once