• State
  • Locked Locked
  • Replies 8 replies
  • Subscribers 81 subscribers
  • Views 2395 views
  • Users 0 members are here
Support feedback
Options
Options
Related

This thread has been locked.

If you have a related question, please click the "Ask a related question" button in the top right corner. The newly created question will be automatically linked to this question.

PxREN

John Schueckler
Prodigy 40 points

Other Parts Discussed in Thread: MSP430G2553

Some of my students are trying to teach themselves the MSP340G2553 in a LaunchPad. On the LaunchPad they want to program the push button at P1.3. They want to program the input pushbutton to have a pull up resistor so they can read the state of the switch as either pressed or not ie 1/0. Below is their code:

#include  <msp430g2553.h>

unsigned int i = 0;
unsigned int z = 1;

#define BTN       BIT3
#define red_LED   BIT0
#define grn_LED   BIT6

void main(void)
{
 WDTCTL = WDTPW + WDTHOLD;
 P1DIR |= 0x41;
 P1REN |= BIT3;
 P1OUT = BIT3;


    for (;;)
 {


  while ((P1IN & BTN) != BTN)
  {
   if(P1OUT & red_LED  == red_LED)
   {
    P1OUT ^= red_LED;
   }

   P1OUT ^= grn_LED;
   for (i = 0; i < 7000; i++);
  }

  P1OUT = 0x48;
  while ((P1IN & BTN) == BTN)
  {
  P1OUT ^= red_LED;
  P1OUT ^= grn_LED;
  for (i = 0; i < 15000; i++);
  }

 }
}

They are claiming the only way to continue to use P1.3 as an input with the internal pull up resistor was to continually output P1OUT.BIT3 as 1 whenever they used P1OUT. That seems redundant. I researched the specsheets and user's manual and did not find a clear example on the use of P1REN. Does the PullUp/PullDown have to be set each time P1OUT is used?

  • 0
    Guru 102065 points

    The pull-up/down resistor setting follows the value of the respective P1OUT bit.

    So: Yes, if you want to keep P1.3 pulled Up all the time, you need to keep P1OUT.3 = 1 all the time. But P1OUT.3 doesn't change unless you change it.

    The reason it looks odd is that they are doing direct assignments to P1OUT, e.g. "P1OUT=0x48", which they should Not be doing -- direct assignments to PxOUT are in most code quite rare, since doing that changes bits that are not (at the moment) relevant. They should rather be manipulating individual bits with |, &, and ^, not touching the others.

  • 0
    Guru 102065 points

    John Schueckler said:

       if(P1OUT & red_LED  == red_LED)

    Unsolicited: This statement will come back to bite someone someday. Double-check the operator precedence.

  • 0 in reply to Bruce McKenney
    Prodigy 40 points

    Thank you for your input. I changed the P1OUT to |=0x40. and the program performsas desired. You know students ... BRUTE force it is the first choice.  Howeveris  the order of programming the resistors correct P1REN then P1OUT to determine pullUp or pullDown? Another post (not here) seemed to suggest you could P1OUT.BIT3 as a 1 then then P1REN.

  • 0 in reply to Bruce McKenney
    Prodigy 40 points

    Thank You,

     I see that the exactly equals '==' is higher priority than the AND '&'. I will have then use '()'.

  • 0
    Guru 227245 points

    There are several things in this code which aren't, well, 'consistent'. But none of them is related to the state of the pin pullup. It is set and remains set all the time.

    John Schueckler said:
    P1DIR |= 0x41;
     P1REN |= BIT3;
     P1OUT = BIT3;

    Why aren't the previous defines used here? If the configuration changes later, those 'implicit' assignments need to be traced down one by one. Using BTN, red_LED etc. will change the whole code when the configuraiton changes.

    John Schueckler said:
     while ((P1IN & BTN) != BTN)

    This is inefficient. Since BTN is a bit, the result of the AND can only be 0 or BTN. No need to compare it with BTN then. A simple zero/non.zero comparise is shorter and faster (and even typed faster): while (!(P1IN&BTN)). Same for the followign IF and the othe rwhile. (well, the IF is redundant anyway, see below)

    John Schueckler said:
       if(P1OUT & red_LED  == red_LED)
       {
        P1OUT ^= red_LED;
       }

    unnecessarily complex. It toggles red_LED if red_LED is set. So it always clears red_LED. Replacing the whole if by a simple P1OUT &= ~red_LED does the same in one line and one instruction: clear red_LED (whether it is set or not).

    John Schueckler said:
       for (i = 0; i < 7000; i++);

    delay loops are risky. The loop doesn't change the state of the system by any means (except for setting i = 7000), so dependign on compiler and optimization level, the whole loop may go into the void. Especially since the next isntruciton that uses i sets it to 0 and doesn't read it. The compiler doesn't kow that wasting time by executing useless code is the intention. Also, even if the compiler doesn't eliminate the whole loop, you don't knwo what it will compile too. A change on a completely different place may result in a completely different code generated for the loop, with completely different timing.
    Use _delay_cycles() instead. This expresses the intention of a delay of an exact number of CPU cycles. Or use a timer (that's what timers were invented for).

    John Schueckler said:
      P1OUT = 0x48;

    This always sets grn_LED and also BTN. If this code was originally P1OUT = 0x40 to set grn_LED, then it was at the same time clearing BTN and switching the pullup to pulldown. I assume that was the original bug. Which then was fixed by also setting BTN in the assignment instead of using the correct instruction P1OUT |= grn_LED; to set BIT6 and keep the rest untouched.

  • 0 in reply to Jens-Michael Gross
    Prodigy 40 points

    Jens-Michael,

    Thank you for the input you are correct.  These are student learning the MSP430G2553, and I want to get them  to be users first then elegant programmers. Syntax aside, the question was in the use PxREN, the order must be PxREN to enable resistors then PxOUT to determine pullUP/DOWN?

  • 0
    Prodigy 10 points


    //  by    repo tang

    #include "msp430g2553.h"
    int main( void )
    {
      WDTCTL = WDTPW + WDTHOLD;        
      P1DIR |= BIT0 + BIT6;            
      P1OUT |= BIT0 + BIT6 +BIT3;         
      P1REN |= BIT3;                       
      P1IE  |= BIT3;                      
      P1IES &= ~BIT3;                      
      //P1IES |=  BIT3;                     
      P1IFG &= ~BIT3;                   
      __enable_interrupt();             
      while(1)                           
      {
        ;
      }
    }


    #pragma vector = PORT1_VECTOR
    __interrupt void Port_1(void)
    {
      if(P1IFG & BIT3)                   
      {
        P1OUT ^= BIT0 +BIT6;             
        P1IFG &= ~BIT3;                
      }
    }

  • 0 in reply to John Schueckler
    Guru 227245 points

    John Schueckler said:
    These are student learning the MSP430G2553, and I want to get them  to be users first then elegant programmers.

    My comments weren't about elegance. Elegant coding is often enough the death of efficient microcontroller coding.
    But establishing some selected coding habits right form the start, such as using defines for signals or avoiding for-loop delays,  saves a lot of future problems. And makes the code easier readable, understandable and debuggable, especially by 3rd party. :)

    John Schueckler said:
    the order must be PxREN to enable resistors then PxOUT to determine pullUP/DOWN?

    Not necessarily. PxOUT can be set first an then the pullups can be enabled. Things are executed in order. Initial state is high impedance input. If you set PxOUT and not PxDIR before, it remains high impedance input. Then you set PxRSEL and the pullup or pulldown becomes active. If you first activate PxRSEL, the pullup will be active and since PxOUT is undefined after power-on, the pullup might be a pulldown first until you set it with PxOUT, generating a probably unwanted low pulse on the pin. However, it has no impact on the future pin operation. You can take a look at the device datasheet, it contains a port pin schematics.
    For the digital I/O, things happen exactly when done in the code. Together when done together (e.g. for multiple bits), or after each other when done after each other (when using separate instrucitons or - 666- using bitfields). (for the complex ADC and USCI modules, things are a bit different, as they use state machines).
    The bits written to the port registers are direct input signals into the port schematics.

    However, the key problem seemd the  P1OUT = 0x48; assignment which (I assume, as you didn't post the non-working code) was originally a P1OUT = 0x40, which not only set the green LED but also turned the pullup into a pulldown by clearing its bit. Using "|=" operator isntead of "=" to just set the green LED bit but leave the rest untouched would have worked as well.

**Attention** This is a public forum