External Interrupt in MSP430f5438A & Experiment Board

Hi there,

First of all, your variables ok, i, AUX are not declared volatile, hence when they are accessed in your ISR the behaviour is undefined, as you don't know how the compiler optimized the accesses to the variables (they may have been optimized away completely). It's by pure luck that your code sometimes work.

Also, you do not need to set the PxSEL bits since by default they are 0, ie. general IO.

BTW, instead of doing something like

P2DIR &= 0x7F;

It's much better to specify the bits explicitly in your code, eg

P2DIR &= ~(BIT7);

There's no additional cost since the compiler translate them into BIC instructions.

Finally, you do not need an empty else statement following every if statement; if you have only one condition to handle, the if itself suffices.

Tony

TonyKao said:

when they are accessed in your ISR the behaviour is undefined

well, 'undefined' is a bit vague. Actually, it is undefined when (or whether) a change made to a variable inside an ISR is noticed by the main thread. Or when a change to the variable done in main is actually arriving in the variables memory location adn can be noticed by the ISR.

For the first case, it depends on the compilers analysis of variable usage. a while(x); will probalby loop forever because x is read once form memory and then held in a register and thested there ever and ever again, not noticing any change by an ISR.
In the second case, any change to the variable done by main will be written to memory prior to any function call, but not necessarily at the point where it is done in the source code. Most likely, it will also be written before leaving a memory block {...}. But until then, the change might be just inside a processor register (and therefore invisible to an ISR) or delayed completely (if the variable is not used anywhere else).

TonyKao said:

BTW, instead of doing something like
P2DIR &= 0x7F;
It's much better to specify the bits explicitly in your code, eg
P2DIR &= ~(BIT7);
There's no additional cost since the compiler translate them into BIC instructions.

Well, if BIT0..3 are used, the compiler can generate a BIC instruction using the constant generator instead of an AND operation with explicit numerical operand, a BIC instruction with the constant generator is generated, which is one clock cycle faster and 2 bytes smaller.
However, this only works if the operand is a single bit 0..3.
I'm not sure whether the compiler is smart enough to convert &=0xFE into a BIC BIT0. :) (well, it might...)

TonyKao said:

Finally, you do not need an empty else statement following every if statement; if you have only one condition to handle, the if itself suffices.

Right. But for people who like to write single statements after an IF without brackets, the superfluous else ensures the correct assignment of else to the appropriate (intended) if. Just a bad habit compensating another bad habit :)

Thanks for your attention.

About empty else statements, it' s an "order" of my chief, I know that it isn't necessary, bit we do that for sharing the same "code rules" between us.

About register configuration,  I have usually written hexadecimal in university, so it's a habit for me, not for technical issues. If it's more comfortable for understanding to use BIT, I will start to use it:

Well, after rewrite as volatile int, my timing response of S2 is still "lagging", that is to say, I push S2, enter in interruption, push again S2 and it doesn't enter in interrupt. I have to wait some seconds, push S2 again, and then it enters in interruption and toggle LEDs.

I don't know if that is the standard timing response because if I comment "P2DIR&=0x7F", that is, the pin 2.7 as output (see the line before "P2DIR|=0xFF"), the timing response is "instantaneous". But with that line  no commented P2.7 as an input, it happens the problem which I have told you in previous paragraph.

Thanks for all and sorry for my English.

Is T da S said:

About register configuration,  I have usually written hexadecimal in university, so it's a habit for me, not for technical issues. If it's more comfortable for understanding to use BIT, I will start to use it:

The advantage is that if you use symbols, you document what you mean. If you use hex values, one can only guess and hope that there is a comment that fits the value.
In case of bits, it is not a big deal (but will make errors more obvious than a hex value that must be interpreted first for verification). For other registers, saying "register = option1 | trigger' is better than writing 'register = 0x1234'. Becaus the intention is clear, wrong options are rather obvious, so it is much, much easier to debug. Or to change in case of experiments.

Is T da S said:

Thanks for all and sorry for my English.

Many, if not most people here are no native speakers (including me), so no need to apologize. As long as your English is good enough to understand your problem (and understand our answers)...

Is T da S said:

P1OUT = ~AUX;          // Load saved configuration after two LEDs turned on

??? This line sets the whole port 1 to the inverted value of what was saved in AUX previously. It does not restore the saved value.Probably not what you want and a possible source for erroneous habit of the LEDs.

Is T da S said:

  __bis_SR_register(GIE); // Interrupt enable

It's a bad, bad, bad idea to enable interrupt inside an ISR. this may lead to strange results up to stack overflow, device resets and such. This technique is called inrerrupt nesting and should be only used by people who exactly knwo what theya re doing. And it serves no purpose in your case anyway.

Is T da S said:

    P2IE |= 0x80;

THis is superfluous in the ISR. You alread set this bit, as it is what brought you into the ISR. And you didn't clear it inside the ISR. So you don't need to set it again. It does no harm anyway - but will if you change your code later to a different pin and forget to adjust this too.

Is T da S said:

if((i!=0)&&(i%5==0))

"I!=0" is equal to "i". I is either zero (= false) or non-zero (=true). It doesn't affect the generated code, but is unnecessaary typing. Well, if it is required by design rules...
However, '%' actually is a division. And divisions are slooow. So you shouldn't use it if possible. If oyu don't need I to be a continuous counter, just do a compare with 5 and reset i to 0 then. That's much, much faster.

If you need division, maybe you can substitute them with shifts (/4 = >>2) by properly adjusting the values. In most cases, clever multiplicaiton (some MSPs have a hardware multiplier) folowed by a shift can be used. Or the picked value isn't really necessary, and chosing a power of 2 value instead gives the same effect, but is much faster.

In your code, you do P2SEL &=7f. Why that? It will clear bit 0..6, assigning them to GPIO use. And leaves BIT7 (the one you want to use for port pin interrupt) to its previous state. Which is 0 anyway after power-up. And good for you, since if it were set, interrupts would be disabled for the port pin you want to generate interrupts. So this instruciton is superfluous at best.

Jens-Michael Gross said:

well, 'undefined' is a bit vague. 

I should've said "undefined in the C/C++ language standard sense" which means that the behaviour is "implementation specific", i.e. the compiler can do whatever it wants. It actually says so in the standard that non-volatile access to volatile objects result in "undefined" behaviour.

I'm not sure whether the compiler is smart enough to convert &=0xFE into a BIC BIT0. :) (well, it might...)

Yes, however it could generate BIC #0xFE, DST which is just one cycle longer, not much difference unless you're dependent on cycle counting.

Tony

Is T da S,

Your code

for(;;)
{
  if(ok==1)
  {
    i++;
    P1OUT ^=0xFF;
    if (i%5==0)
    {
      AUX = P1OUT;
      P1OUT |=0xFF;
    }
//...
    ok=0;
  }
//...
}

Will only trigger the "toggle" after a few presses because your variable i is only incremented once; hence i%5 will only be true after a few iterations of the interrupt, since the variable ok is only set to 1 in the ISR.

You should get rid of the modulo tests altogether. And if you want to "count" how many button presses you have, just increment a volatile global inside the ISR.

Tony

Thanks for your answers.

Step by step, and working with the board, I am learning some things like interrupt enables, flags, registers, etc; that I don't understand at 100% reading the user's guide, so I have to try on the board, and probe it.

With the code, I want to change LED state  (if it's on, interrupt turns off and vice versa) every time I push S2(P2.7) (enter in interruption) and every 5 pulsation of S2 both LEDs turn on. If I push S1(P2.6), both LEDs turn off (OFF STATE). If I push LEFT(P2.1), both LEDs turn on(ON STATE).

When I talk about "delay", I refer to:

1) Push S2 for first time, it enters in interrupt and LED toggle.

2) Immediately, I push S2 again, but LED1 doesn't toggle. I wait for a 1 or 2 secs, I push S2 again, and then LED1 toggles (or both LEDs turn on if it's the other case). It happens with the other buttons enabled, always the same interrupt enters consecutively (with their button pulsation, one after the other, of course) 

It seems like there is a time when the interrupt can't be attended after the same interruption has just been attended. If I configure the P2.7/6/1 only as an output (that is wrong, isn't it?), this delay time disappears.

The new code is :

//

//           MSP430F5438

//         ---------------

//     /|\|           P2.7|<--S2

//      | |           P2.6|<--S1

//      --|RST        P2.1|<--LEFT

//         |                         |

//        |           P1.1|-->LED2

//        |           P1.0|-->LED1

//

 

 

#include "msp430x54x.h"

 volatile int ok=0;                     // Select function to do in  main

 volatile int i=0;

 volatile int AUX=

 volatile int state=0;                         // state = 1, both LEDs have been turned on/off

 

void main(void)

{

       WDTCTL = WDTPW + WDTHOLD;            // Stop WDT

       P1DIR |= 0x03;                    // P1.0 & P1.1 output

       P2DIR |= 0xFF;                           

       P2DIR &= 0x3D;                          // P2.7/6/1 input (LEFT)

 

       P1OUT &= 0xFE;                          // P1.0 off (LED1)

       P1OUT |= 0x02;                           // P1.1 on (LED2)

 

//P2IN &= 0xFD;                   // P2.1 in, low. Write -> CONSUMPTION

       //P2IN |= 0x02;                   // P2.1 in, high

 

       P2IES |= 0xC2;                                              // L-H edge interrupt

       //P2IES &= 0x7F;                                      // H-L edge interrupt                                                                 

       //P2SEL &= 0x3D;                                      // Pin function (low)

 

 

       P2IFG &= 0x00;                          // Reset flags ¿necessary?

       P2IE |= 0xC2;                     // Enable P2.7/6/1 interrupts

       AUX=P1OUT;

 

  __bis_SR_register(GIE);               // General Enable

 

 for(;;)

       {

       switch(ok)

              {

                    case 1:                                                    // Changing LEDs

                                  i++;

                                if (i==5)

                                  {

                                         AUX = P1OUT;                // Save configuration before two LEDs turned on for next S2 interrupt

                                         P1OUT |=0xFF;              // Turn on all LEDs

                                                                     

                                  }

                                  else

                                               {

                                                     P1OUT ^=0xFF;                   // Write NOT-Output

                                                     AUX = P1OUT;                    // Save configuration

                                               }

                                              

                                  ok=0;               // Finished job in main

                                  break;

                                 

                    case 2:                           // ****OFF STATE****

                                  P1OUT &= 0x00;

                                  ok=0;               // Finished job in main

                                  state=1;                                      

                                  break;

                                 

                    case 3:                                 // *****ON STATE****

                                  P1OUT |=0xFF;

                                  ok=0;               // Finished job in main

                                  state=1;

                                  break;

                                 

                    default: break;           

       }

      

}

}

 

#pragma vector=PORT2_VECTOR

__interrupt void PORT2_ISR(void)

{

 

       switch(__even_in_range(P2IV,16))

       {

         case  0: break;                         

         case  2: break;   

                                                 

         case  4:                                              // Turn on LEDs interrupt

                           P2IFG &= 0xBD;             // Reset flag

                           ok=3;

        

                           break;                         

         case  6: break;

         case  8: break;                          

         case 10: break;                         

         case 12: break;                         

         case 14:                                           // Turn off LEDs interrupt

                           P2IFG &= 0xBF;             // Reset flag

                           ok=2;

                           break;   

                                                  

         case 16:                                               // Change LEDs interrupt

                            P2IFG &= 0x7F;                // Reset flag

                                              

                            if((i!=0)&&(i==5))             // For loading previous LED configuration before two LEDS turned on

                                  {

                                        P1OUT = AUX;                  

                                        i=0;                                     

                                  }

                                  else

                                        {

                                                                                                                                //nothing to do here

                                        }

                           if (state==1)

                                  {

                                        P1OUT=AUX;                                       // Load saved configuration previous turned off or turning on STATE

 

                                        state=0;

                                  }

                                  else

                                  {

                                                                                                                                       //nothing to do here

                                  }

                           ok=1;                                                                                                // Start job in main

          

                           break;                          

                                 

         default: break;

  }

      

}

Thanks again.

Hi,

As I have said before, this condition

if((i!=0)&&(i==5))

is the reason why the "toggle" in the ISR only executes after several button presses, since i is incremented only twice with each button press. Even worse than before, now the code will only reach the toggle once, since you replace the modulo with ==. Well, until i overflows, but that'll take quite a few button presses.

Tony