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.

MSP430FR2355: exit.c -- forever loop -- ???

Part Number: MSP430FR2355

I have a function (see below) and no matter what I try (both timer3 or UCRXIFG checking) I end up in a forever loop that doesn't come back in the RESPONSE section of the state machine....Can anyone tell me what I am missing??.....

#include <msp430.h>
#include <string.h>
#include <stdint.h>
#include "rc.h"
#include "LPRSradio.h"

char* radioReg(char *p) {

    char f;
    radioStates state;
    const char *configAck = "ACK";
    char response[10] = {0};
    char *pResponse;
    uint8_t length;

    length = strlen(p);
    state = CMD;
    pResponse = response;
    /*
     * categorizing radio commands into 1 of 3 bins
     */
    if ((*(p + (length - 2)) == 'T') || (*(p + (length - 3)) == 'N') || (*(p + (length - 3)) == 'L'))
        f = 2;
    else if (*(p + (length - 1)) == '?')
        f = 1;
    else
        f = 0;

    while (state != DONE) {
        switch (state) {
        case CMD:
            tx = T; ack = F;
            UCA1TXBUF = (*p);
            UCA1IE |= UCTXIE;
            __bis_SR_register(LPM3_bits + GIE);
            *(p++);
            if (*p == '\0') {
                UCA1IE &= ~UCTXIE;
                if ((f == 2) || (f == 0))
                    state = ECHO;
                else
                    state = RESPONSE;
            }
            break;
        case ECHO:
            tx = F; ack = F;
            if (howMany < length)
                __bis_SR_register(LPM3_bits + GIE);
            else {
                howMany = 0;
                p = configAck;
                pRx = incoming;
                state = ACK;
            }
            break;
        case ACK:
            tx = T; ack = T;
            UCA1TXBUF = (*p);
            UCA1IE |= UCTXIE;
            __bis_SR_register(LPM3_bits);
            *(p++);
            if (*p == '\0') {
                UCA1IE &= ~UCTXIE;
                p = *(configCmd);
                if (f == 0) {
                    state = DONE;
                }
                else {
                    state = RESPONSE;
                }
                /*
                 * give rx a chance to respond with
                 * response...longest response is 17 char
                 * at 19200 baud or 8.85ms + about 3.5ms
                 * delay after ack = ~12.5ms...timer set
                 * to wake up after 20ms
                 */
//                __bis_SR_register(LPM3_bits);
            }
            break;
        case RESPONSE:
            tx = F; ack = F;
            memset(incoming, 0, 20);
            TB3CTL |= MC__UP;
            TB3CCTL0 |= CCIE;
            if ((!timerExpired)) {//(UCA1IFG & UCRXIFG)
                __bis_SR_register(LPM3_bits);
            }
            else {
                pRx = incoming;
                howMany = 0;
                memcpy (response, incoming, sizeof(incoming));
                state = DONE;
            }
            break;
        case DONE:
            break;
        default : break;
        }
    }
    return pResponse;
}

  • Does the TIMER3_A0_VECTOR set timerExpired=1? Is timerExpired declared "volatile"?

  • Yes and Yes

    ISRs

    #pragma vector=USCI_A1_VECTOR  //radio communication
    __interrupt void RF_ISR(void)
    {
        switch(__even_in_range(UCA1IV, USCI_UART_UCTXCPTIFG))
        {
          case USCI_NONE: break;
          case USCI_UART_UCRXIFG:
              if (!tx) {
                  *pRx = UCA1RXBUF;
                  howMany++;
                  pRx++;
                  __bic_SR_register_on_exit(LPM3_bits);
              }
              break;
          case USCI_UART_UCTXIFG:
              if (tx)
                  __bic_SR_register_on_exit(LPM3_bits);
              break;
          case USCI_UART_UCSTTIFG: break;
          case USCI_UART_UCTXCPTIFG: break;
        }
    }
    #pragma vector=TIMER3_B0_VECTOR
    __interrupt void TimeoutRF(void)
    {
        TB3CCTL0 &= ~CCIE;
        TB3R = 0;
        UCA1IFG &= ~UCRXIFG;
        timerExpired = T;
        __bic_SR_register_on_exit(LPM3_bits);
    }
    

    Tried both timeout and RXIFG but....The only thing that for sure stops it and brings me back is if I do something like if (howMany <= #)....The problem with that however is # is dynamic in length and depends on the command that was sent 

  • >  char response[10] = {0};

    >            memset(incoming, 0, 20);

    >            memcpy (response, incoming, sizeof(incoming));

    What I deduce from these is that either:

    1) incoming is size 10, and the memset overruns incoming[] or

    2) incoming is size 20, and the memcpy overruns response[]

    [Edit: Also, it looks like you're returning a pointer to a stack variable, which is, as they say, "hazardous".]

  • Bruce....Comment and Question...

    Comment first:  Seems like if I go to sleep in an if - else area (in this case the if) and then I wake up within that if I never do the else....Does this seem logical?  I say that bcz I decided to try the following change to the code and I now seem to come back to main and stop.  Notice I got rid of the else and replaced it with a second if

                if ((!timerExpired)) {//(UCA1IFG & UCRXIFG)
                    __bis_SR_register(LPM3_bits);
                }
                if (timerExpired ){
                    pRx = incoming;
                    howMany = 0;
                    memcpy (response, incoming, sizeof(response));
                    state = DONE;
                }
                break;
    

    Question: Can you tell me more about the return pointer?  I am having trouble bringing back the pointer to the array response so that I can use that data moving forward.....

    Thanks

    Steve

  • In C, either the if block or the else block is executed, not both. A sleep is nothing special as far as the compiler is concerned -- it just sets a bit in a register.

    The way your loop was constructed, it would stay in RESPONSE state, polling timerExpired (in the big loop) until it became non-0, at which point the else block would be executed.

    ---

    Returning from a function (conceptually) eradicates all its stack variables. The next function call (for real) eradicates all those stack variables, by writing its own stack variables over top. That pointer might hang around for a long time after the thing it's pointing to has turned into who-knows-what. This hazard was mentioned in K&R 1ed., maybe even in the original Bell Labs C Report.

    The cheap-imitation fix is to make the pointer target "static", to move it out of the stack. This avoids the big problems, though the next time you call that function the pointer's target (contents) will still change. This is one of those things where, if I find myself about to do it, I stop and re-think.

  • A little confused here as I take it from your statement above then that there is no logical reason why the if-else shouldn't have worked?  I am confident that the timer had expired as I saw the variable showing such.....I (I guess like you) saw the return from the ISR as waking up within the if, recognizing the 'if' portion was false, executing at that moment the 'else' or worst case breaking out of the 'if-else' running to the end then through the whole case process again until it got around to that section and then execute the 'else' portion......Clearly something was going on bcz if-if is working....

  • No, it wouldn't re-evaluate the if() after waking up. Since that block didn't change "state" , it would fall out of the case, then the big loop would come back to the same case and re-evaluate the if().

    But then (I suspect) when it executed the "else"  the memcpy overwrote "state" and the big loop didn't exit.

  • Ahh...interesting....although I did fix the issue you commented earlier on ie memcpy (.....sizeof(response)), as I did have that wrong....but it didn't make any difference as to the forever loop.....seems odd that memcpy overwrote state but in any event my issue seems to be fixed....Thanks for all the help!!

    Steve

  • Can you elaborate more on the pointer as I don't understand.....

    My main code looks like this:

        pTx = *configCmd;
        pRadioResponse = radioReg(pTx);
        pTx = *(configCmd+1);
        pGroupNo = radioReg(pTx);
        pTx = *(configCmd+2);
        pRadioResponse = radioReg(pTx);
        pTx = *(configCmd+3);
        pRadioResponse = radioReg(pTx);
        pTx = *(configCmd+4);
        pRadioResponse = radioReg(pTx);
        pTx = *(configCmd+5);
        pRadioResponse = radioReg(pTx);
        pTx = *(configCmd+6);
        pRadioResponse = radioReg(pTx);
        pTx = *(configCmd+7);
        pRadioResponse = radioReg(pTx);
        pTx = *(configCmd+8);
        pVersionNo = radioReg(pTx);
        pTx = *(configCmd+9);
        pRadFrq = radioReg(pTx);
        pTx = *(configCmd+10);
        pRadioResponse = radioReg(pTx);
        __bis_SR_register(LPM3_bits + GIE);
    

    I have tried making the return pointer from function radioReg with and without static but to no avail.  If I don't use static and stop at __bis_SR....everything reads 0....If I use static and stop there all the pointers read the last command response....How is it that pGroupNo, pVersionNo which are unique get over-written with the next command response?  

    Thanks

  • If you look at those pointers, you'll see they are all the same, i.e. they point to the same place. Each call to radioReg overwrites that "same place" with something new. 

    The difference between the stack and static methods is that with "static" that pointer (what it points to) is "safe" until you call radioReg again, but with "stack" it's only "safe" until any other function is called (that includes an ISR, which you don't have much control over).

    Unless you're going to do something with the pointer (what it points to) immediately, you should copy the contents somewhere else. Once you think about doing that, you'll probably conclude that it would be easier to just pass the eventual destination buffer (pointer) directly into radioReg in the first place.

  • Thanks Bruce....

    I understand....The good news is I am going to act on the response immediately in this instance as these are all configuration commands...My actual payload data is handled in a different manner.

    Steve

**Attention** This is a public forum