ISR Functionality on the MSP430

Hi

I suggest you to completely rewrite your code. Don't know exactly what you intended to program but the WHILE inside the Interrupt should not be necessary. Also to test for Interrupt flags in the main Loop can't be the right approach. 

After the SPI TX-buffer is transferred to the SPI-shift register, the TX-buffer is ready to receive the next byte. This is indicated by a TX-Interrupt and this is the right moment with plenty of time to prepare the next byte to be sent, (independent of the master’s clock because this happens when the master’s clock is “asking” for the first bit.

The RX-Interrupt on the other hand should contain a short command-interpreter. I Guess the main loop can be left empty in the right approach and timing in general is not (that) important anymore. Everything runs seamless then.

Matthias

Matthias Siegenthaler said:

the WHILE inside the Interrupt should not be necessary.

I should probably refer to the previous post that I've mentioned.  The link is here. 

The reason why I am doing this can be shown in the figure below.  As you can see here, as well as in the previous post, I an collecting multiple bytes of data, and in order to stay at the most optimal speeds, I need to stay in the ISR until the program has completed the tasks in this state.  

But I've also found some of the sources to the problem.  After using a Beagle Board, I collected the data getting sent and received between the SPI Master and Slave.  The data is shown below.

The data is represented in the table above shows that the data is initially being transmitted and received correctly by the MSP430 before it begins to fail.  After combing through the code, I've found that the reason why this is happening is because the MSP430 stops entering the ISR.

 Matthias, I understand that the ISR should hold very little logic, but If I leave any logic outside the ISR, then the program will have latency issues.  I am open for alternatives though
 

>      while (!(UCA0IFG & UCRXIFG));

Since you're catching the RXIFG in the ISR, main() will never see it. main() will be stopped at this statement forever.

> UCA0IFG &= ~UCRXIFG;

Fetching from RXBUF already clears this bit. Doing it again sets up a race (dropped byte) which could produce the symptom you describe. Make a point to only clear RXIFG by reading RXBUF (even if the code is simply "(void)UCA0RXBUF;/*read and discard*/")

> while ((UCA0STATW & UCBUSY) == 1);

Please don't do this. It accidentally works, but only because it happens that UCBUSY==1, and it sets off alarms in my brain.

Bruce McKenney47378 said:

while ((UCA0STATW & UCBUSY) == 1);

Please don't do this. It accidentally works, but only because it happens that UCBUSY==1, and it sets off alarms in my brain.

I know that this may not be the best way to do this, but notice in the figures in the previous posts that the SPI Master sends multiple bytes in series of one another.  Therefore, in order for the MSP430 to respond quickly enough, the ISR must remain running until the last byte has been transmitted.  Unless you know of another approach to doing this, I have no choice but to work with this.

I understand the first two arguments, and I have noticed it being mentioned in the datasheet.  The problem is that the ISR is not functioning the way it should, and unless I do leave those in there, it will not work.

Below is the code that initializes the SPI.  I want to include that so that I make sure that I've optimized the SPI ISR to respond with highest priority (as well as quickly as possible).

void SPI_init(void)
{
   UCA0CTLW0 |= UCSWRST; // **Put state machine in reset**
   // 4-pin, 8-bit SPI slave
   // Clock polarity high, MSB
   // 4-Pin SPI, w/ UCxSTE Active Low
   UCA0CTLW0 |= UCSYNC + UCMSB + UCMODE1 + UCCKPH;
   UCA0CTLW0 |= UCSSEL_2; // ACLK
   UCA0BR0 = 0x02; // /2
   UCA0BR1 = 0; //
   UCA0MCTLW = 0; // No modulation
   UCA0CTLW0 &= ~UCSWRST; // **Initialize USCI state machine**
   UCA0IE |= UCRXIE;// + UCTXIE; // Enable USCI_A0 RX interrupt
   UpCntr = 0x90;
}

For now, I'm going to step back on my code and attemp to just make the ISR respond as it should.  Please feel free to give me more suggestions.  I definitely appreciate the feedback I've received so far.

Your trace seems to show 2x bytes following a 0x01 opcode and 1x bytes following a 0x03 opcode. Your code appears to expect the opposite. Have I missed something?

-------

while ((UCA0STATW & UCBUSY) == 1);

Sorry, I was being glib. This line of code does what you want, but for non-obvious reasons. What you've actually coded here is:

while ((UCA0STATW & UCBUSY) == UCBUSY);

but no one reading it will figure this out without pulling out the UG and looking up the bit definition. Personally, I prefer

while ((UCA0STATW & UCBUSY));

Bruce McKenney47378 said:

Your trace seems to show 2x bytes following a 0x01 opcode and 1x bytes following a 0x03 opcode. Your code appears to expect the opposite. Have I missed something?

Your analysis of the trace is correct.  When the 0x01 instruction is delivered from the master to the slave, the master sends 2 more bytes (one being the address to write to, and the other being the data to be written).  When the 0x03 instruction is delivered from the master to the slave, the slave must transmit the single byte status register to the master. Hence, this is why it is a bit trickier.

I've updated the code to remove any while loops out of the ISR, as well as reduce the amount of logic that is done in the ISR.  Below is my updated version.

#define SET_WIP (Status_Reg |= 0x01) 
#define CLR_WIP (Status_Reg &= (~(0x01)))
#define CHK_WIP (Status_Reg & 0x01)

int main(void)
{
   SYSTEM_init();
   Status_Reg = 0xF0; //0b11110000; // Status Register - [1][1][1][1][BP1][BP0][WEL][WIP]
   while (1)

   {
      while (byte_count == 0);

      //=================================================================
      //=================================================================
      //=================================================================
      if (Temp_SPI_Read == 0x03)
      {

         UCA0TXBUF = Status_Reg;
         STROBE1_TOGGLE;
         while (byte_count == 1);

         UCA0TXBUF = 0x00;
         byte_count = 0;
         STROBE1_TOGGLE;
         Temp_SPI_Read = 0;
      }

      //=================================================================
      //=================================================================
      //=================================================================
      else if (Temp_SPI_Read == 0x01)
      {

         UCA0TXBUF = 0x00;
         SET_WIP;
         STROBE1_TOGGLE;
         while (byte_count == 1);

         SPIaddress = Temp_SPI_Read;
         FRAM_ptr = (unsigned short *)(FRAM_START+SPIaddress);
         STROBE1_TOGGLE;
         while (byte_count == 1);

         *FRAM_ptr = Temp_SPI_Read;
         UCA0TXBUF = Status_Reg;
         if (SPIaddress == 0x20) find_magic_number();
         else if (SPIaddress == 0x2E) find_magic_number();
         else if (SPIaddress == 0x2F) find_magic_number();
         else if (SPIaddress == 0x40) find_magic_number();
         else if (SPIaddress == 0x41) find_magic_number();
         else if (SPIaddress == 0xA0) find_magic_number();
         else if (SPIaddress == 0xAE) find_magic_number();
         else if (SPIaddress == 0xAF) find_magic_number();
         else if (SPIaddress == 0xC0) find_magic_number();
         else if (SPIaddress == 0xC1) find_magic_number();
         else if (SPIaddress == 0xE2) find_magic_number();
         else if (SPIaddress == 0xF4) find_magic_number();
         else if (SPIaddress == 0xF5) find_magic_number();

         CLR_WIP;
      }

      //=================================================================
      //=================================================================
      //=================================================================
      else

      {
         byte_count = 0;
         STROBE1_LOW;
      }
   }
}

#pragma vector=USCI_A0_VECTOR
__interrupt void USCI_A0_ISR(void)
{
   Temp_SPI_Read = UCA0RXBUF;
   byte_count ++;
   STROBE1_TOGGLE;
   if (Temp_SPI_Read == 0x03 && CHK_WIP)
   {
      UCA0TXBUF = Status_Reg;
      Temp_SPI_Read = 0;
   }

   __bic_SR_register_on_exit(CPUOFF);// Wake up to setup next TX
}

// Port 1 interrupt service routine
// Resets the SPI just in case
#pragma vector=PORT1_VECTOR
__interrupt void Port_1(void)
{
   UCA0CTL1 |= UCSWRST;
   UCA0CTL1 &= ~UCSWRST;
   UCA0IE |= UCRXIE;
}

While the code is much cleaner, the MSP430 is still not entering the ISR, even when the SPI master transmits information (I've checked this by toggling a GPIO pin on the MSP430).  The changes I've made should reduce the polling while loops, but I don't understand why the ISR would not respond.  Again, any further suggestions would be appreciated.

It looks as though you're catching the Chip Select via a P1 interrupt, but you aren't clearing the (appropriate) P1IFG bit. Once the /CS is signalled, you'll be looping (disabled) through Port_1 (ISR) forever. That would certainly produce your symptom.

Try adding "P1IFG=0; // crude but effective" to your P1 ISR.

Bruce McKenney47378 said:

Try adding "P1IFG=0; // crude but effective" to your P1 ISR.

I like the added comment to the line you suggested to add.  However, while it does make sense to keep the suggested line in the P1 Interrupt Service Routine, my program still does not enter the SPI Interrupt Service Routine.  The changed P1 Interrupt Service Routine is shown below.

// Port 1 interrupt service routine
// Resets the SPI just in case
#pragma vector=PORT1_VECTOR
__interrupt void Port_1(void)
{
   UCA0CTL1 |= UCSWRST;
   UCA0CTL1 &= ~UCSWRST;
   UCA0IE |= UCRXIE;
   P1IFG=0; // crude but effective
}

One thought that came to mind was whether or not the function "find_magic_number()" would cause this.  I know that it doesn't in ideal cases, but I'm trying to look at any cause to this issue.

My guess is that knowing what SYSTEM_init() does would be more topical.

Notably, I don't see anything in this code that enables interrupts (GIE). Where/when does that happen?

Bruce McKenney47378 said:

I don't see anything in this code that enables interrupts (GIE). Where/when does that happen?

Below is the setup of how I initialize my code.  

/* ======== CS_Init ========
* Initialize MSP430 Clock System
*/
void CS_Init(unsigned int clock_speed)
{
   //Writes password to enable CS register access
   CSCTL0_H = 0xA5;

   if (clock_speed == 24)
   {
      //Set max. DCO setting to 24MHz
      CSCTL1 |= DCOFSEL0 + DCOFSEL1 + DCORSEL;

      // Sets the MCLK source to DCOCLK,
      // the SMCLK source to DCOCLK,
      // and the ACLK source to VLOCLK
      CSCTL2 = SELM0 + SELM1 + SELS0 + SELS1 + SELA0;

      // Sets the MCLK clock divider to 0,
      // the SMCLK clock divider to 0,
      // and the ACLK clock divider to 0,
      CSCTL3 &= 0x00;

      // Enables conditional module requests for
      // MCLK, SMCLK, and ACLK
      CSCTL6 |= MCLKREQEN + SMCLKREQEN + ACLKREQEN;
   }
   else if (clock_speed == 8)
   {
      CSCTL1 |= DCOFSEL0 + DCOFSEL1; // Set max. DCO setting
      CSCTL2 = SELA_3 + SELS_3 + SELM_3; // set ACLK = MCLK = DCO
      CSCTL3 = DIVA_0 + DIVS_0 + DIVM_0; // set all dividers
   }

   // Lock Register
   CSCTL0_H = 0x01;
}

void GPIO_init(void)
{
   // Configure SPI Pins
   // Pin 1.5 set for Serial Clock Out (UCA0CLK)
   // Pin 1.4 set for SS (Slave Select) (UCA0STE)
   P1SEL1 |= SPI_CLK + SPI_SS;
   P1OUT &= ~(STROBE1 + STROBE2 + STROBE3 + STROBE4);
   P1DIR |= STROBE1 + STROBE2 + STROBE3 + STROBE4;

   // Terminate Unused GPIOs
   // P1.0 - P1.6 is unused
   P1OUT &= ~(BIT6 + BIT7);
   P1DIR &= ~(BIT6 + BIT7);
   P1REN |= (BIT3 + BIT6 + BIT7);

   // Configure SPI pins P2.0 and P2.1
   // Pin 2.0 set for Data Out (UCA0SIMO)
   // Pin 2.1 set for Data In (UCA0SOMI)
   P2SEL1 |= MOSI + MISO;

   // P2.3 - P2.7 is unused
   P2OUT &= ~(BIT3 + BIT4 + BIT5 + BIT6 + BIT7);
   P2DIR &= ~BIT3 + BIT4 + BIT5 + BIT6 + BIT7;
   P2REN |= (BIT3 + BIT4 + BIT5 + BIT6 + BIT7);

   // Enable LEDs for STROBE Pin
   // (Which is LED8 on the Experimenter Board)
   P3OUT &= ~(LED8 + LED7 + LED6 + LED5);
   P3DIR |= LED8 + LED7 + LED6 + LED5;

   // Enable LEDs for STROBE Pin
   // (Which is LED8 on the Experimenter Board)
   PJOUT &= ~(LED4 + LED3 + LED2 + LED1);
   PJDIR |= LED4 + LED3 + LED2 + LED1;
}

void SPI_init(void)
{
   UCA0CTLW0 |= UCSWRST; // **Put state machine in reset**
   // 4-pin, 8-bit SPI slave
   // Clock polarity high, MSB
   // 4-Pin SPI, w/ UCxSTE Active Low
   UCA0CTLW0 |= UCSYNC + UCMSB + UCMODE1 + UCCKPH;
   UCA0CTLW0 |= UCSSEL_2; // ACLK
   UCA0BR0 = 0x02; // /2
   UCA0BR1 = 0; //
   UCA0MCTLW = 0; // No modulation
   UCA0CTLW0 &= ~UCSWRST; // **Initialize USCI state machine**
   UCA0IE |= UCRXIE; // Enable USCI_A0 RX interrupt
   UpCntr = 0x90;
}

void SYSTEM_init(void)
{
   /* Stop watchdog timer from timing out during initial start-up. */
   WDTCTL = WDTPW | WDTHOLD;

   CS_Init(24);

   // Turn off temp sensor
   REFCTL0 |= REFTCOFF;
   REFCTL0 &= ~REFON;

   GPIO_init();
   SPI_init();

   // Configure MPU
   MPUCTL0 = MPUPW; // Write PWD to access MPU registers
   //Add any changes to the MPU here
   MPUCTL0 = MPUPW+MPUENA; // Enable MPU protection

   __bis_SR_register(LPM0_bits + GIE); // Enter LPM0, enable interrupts
}

As you can see, the interrupts are enabled at the end of the SYSTEM_init function.  I assumed that this would be working, since it does work for the majority of the time.  My only wonder is whether or not the interrupts would be disabled at any time while the program is running.

1) Are you watching the STROBE1 pin with a scope or an LED?

2) Are byte_count and Temp_SPI_Read declared "volatile"?

Bruce McKenney47378 said:

1) Are you watching the STROBE1 pin with a scope or an LED?

I'm watching the STROBE1 pin with a scope. With it, it is clear that the code is not going into the ISR properly.

Bruce McKenney47378 said:

2) Are byte_count and Temp_SPI_Read declared "volatile"?

Temp_SPI_Read is volatile, but byte_count is not.  I understand why the Temp_SPI_Read should be volatile (since it is holding the information to go into different states). I assumed that since byte_count is just a counter, there was no need.  I'll try changing it once I'm in front of my code again.

OK. (1)+(2) could have "caused" (through ambiguity) your immediate symptom, but neither by itself. On the other hand, you'll bump into (2) pretty soon.

I'll throw in this one also:

3) I don't see anything setting P1IE/P1IES, so your Port_1 ISR won't get called. You're setting RXIE in SPI_Init(), so this isn't your symptom either, but is something you'll eventually run into.

You mentioned that this "works most of the time"; I understood that this always fails (SPI ISR never called ever). In what cases does it succeed?

Bruce McKenney47378 said:

On the other hand, you'll bump into (2) pretty soon.

Alright, I've changed the byte_count variable to be volatile, and I still get the same result.

Bruce McKenney47378 said:

3) I don't see anything setting P1IE/P1IES, so your Port_1 ISR won't get called. You're setting RXIE in SPI_Init(), so this isn't your symptom either, but is something you'll eventually run into.

I'm not sure how else to set this other than use the following instructions at the end of the GPIO_init.

P1IES &= ~BIT4; // P1.4 Lo/Hi edge
P1IE = BIT4; // P1.4 interrupt enabled
P1IFG &= ~BIT4; // P1.4 IFG cleared

I want to make sure that this is set correctly so that it doesn't trigger when the clock (P1.5) is changing.  Instead, I want it so that when the SS_Pin (P1.4) is going high, the SPI can reset itself.  It was put as a precaution, but since my code is not working correctly, it may be useful to get this part up and running.

Bruce McKenney47378 said:

You mentioned that this "works most of the time"; I understood that this always fails (SPI ISR never called ever). In what cases does it succeed?

I'm sorry that I didn't go into further detail.  The SPI interrupt stops responding after about 2800 successful transactions.  Below is a more detailed example of the trace I have for the SPI transactions.

The first several transactions do work properly.  However, after a certain amount of transactions, the MSP430 stops responding, and it's due to the MSP430 not being able to go into the SPI ISR properly.