msp430 single interrupt disabling with non atomic operation

After all, checking for 0 in the IV register might be the correct solution.

I suppose that the event is not lost. 

CCIE to 0 causes the IV register to be 0, but CCIFG is still 1.

Next time CCIE comes up to 1, the interrupt will be called again, this time the IV register will have the correct value.

I agree with both your explanation and action.

But I am very surprised. According to the UG, arbitration and granting of interrupts should happen only after the last MCLK cycle of a CPU instruction (and before the first MCLK cycle of the next CPU instruction). Which member of the MSP family are you using? Which silicon revision?

Good to read that you agree.

My chip is of the x5xx family (UG slau208). Hardware revision (read at 0x1a06) is 0x12. I don't know if this is supposed to be BCD coded, in that case I have the correct errata sheet (for Rev C).

I found a rev E of the errata, but I can't seem to be able to download it.

You did not indicate which chip of the F5xx family you are using. Thus the silicon revision and Errata rev are off no help to others.

I wrote a little test program to trap the problem you were talking about. I do not have a F5xx chip. With what I have, I found no problem. You may need to edit this code depending on the chip you use.

//
//*** It is important that you set up MCLK=SMCLK (come from exactly the same source)
//*** You also may need to change the Timer register names to match the chip
//*** Use the debugger to run the code and stop (break) it after a short while
//*** At that point, the static variables "total" and "late" should be 20 and ~10 respectively
//
#include <msp430.h>
#define TRUE (0xFF)
#define FALSE (0)
int wait, total, late;
char got_one, missed;
void main(void)
{
  WDTCTL = WDTPW+WDTHOLD;
  __enable_interrupt();
  total = late = 0;
  for (wait = 0; wait < 20; wait++)
  {
    got_one = missed = FALSE;
    TACCR0 = wait+200;
    TACTL = TASSEL_2+MC_1+TACLR+TAIE;
    __delay_cycles(210);
    TACTL &= ~TAIE;
    missed = !(got_one);
    TACTL |= TAIE;
    __delay_cycles(10);
    TACTL = TACLR;
  }
  while (1);     // trap at the end of test, should have total=20, late=~10 at this point
}

#pragma vector = TIMERA1_VECTOR
__interrupt void taisr(void)
{
  if (TAIV == 0) while (1);    // trap at strange TAIV, this never happens to me
  got_one = TRUE;
  total++;
  if (missed) late++;
}

I tried to trap for your problem on a FR5739. In this new test, TimerA clock is asynchronous and much faster than MCLK. Still no problem.

What is the chip that you found this problem? What is the MCLK used? What is the clock for the TimerA?

#include <msp430.h>
#define TRUE (0xFF)
#define FALSE (0)
int wait, total, late;
char got_one, missed;
void main(void)
{
  WDTCTL = WDTPW+WDTHOLD; 
  CSCTL0_H = 0xA5;                   // unlock
  CSCTL1 = 0;                        // DCO = 5.33 MHz
  CSCTL2 = SELA_3 + SELS_3 + SELM_1; // ACLK = SMCLK <= DCO; MCLK <= VLO
  CSCTL3 = DIVA_5 + DIVS_5 + DIVM_1; // ACLK = SMCLK = DCO/32; MCLK = VLO/1
  PJOUT = 0;
  PJDIR = 15;                        // LED1, LED2, LED3, LED4
  __enable_interrupt();
  total = late = 0;
  for (wait = 0; wait < 200; wait++)
  {
    got_one = missed = FALSE;
    TA0CCR1 = wait+1;
    TA0CCTL1 = CCIE;
    TA0CTL = TASSEL_2+ID_3+MC_2+TACLR;
    __delay_cycles(22);
    TA0CCTL1 &= ~CCIE;
    missed = !(got_one);
    TA0CCTL1 |= CCIE;
    __delay_cycles(2);
    TA0CTL = TACLR;
  }
  PJOUT = 15;                  // LED1, LED2, LED3, LED4
  while (1);                   // trap at the end of test
}

#pragma vector = TIMER0_A1_VECTOR
__interrupt void taisr(void)
{
  if (TA0IV == 0) while (1);    // trap at strange TAIV
  got_one = TRUE;
  total++;
  if (missed) late++;
}

Sorry I was not generous with details.

I could reproduce the situation on a msp430f5308, in the development module MSP-TS430RGC64B ( http://www.ti.com/tool/msp-ts430rgc64b ). See code below.

Is the hardware version binary coded decimal? Does 0x12 mean revision C? That would feel a bit strange, but I can think of no better explanation.

The timer is in capture mode, interrupt on capture event. MCLK left as default in my example below.

The input clock is SMCLK, clock divided by 2, continuous mode.

Capture happens on both edges, source is a pin (CCIxA), SCS.

Some comments on your code:

- you seem to clear the timer at every loop execution (TACLR). Was that really intended?

- are delays needed between enabling and disabling interrupts? I tried adding them, it only reduced the frequency at which the LED toggling occured.

Here is my code that triggers TAIV == 0. The LED of the development module toggles every time TAIV is 0. This happens irregularly, approximately 3 times per second.

In my test case, I put a square wave [0..3.3] V @ 10 Hz on P1.3 / TA0.3 as the capture signal.

When TA0IV is 0, the return address is after the instruction that disables interrupts.

#include <stdbool.h>

#include <stdint.h>

#include "msp430f5308.h"

#define LED_PORT_SEL    P1SEL

#define LED_PORT_DIR    P1DIR

#define LED_PORT_OUT    P1OUT

#define LED_PORT_PIN    BIT0

#define CAPTURE_SEL     P1SEL

#define CAPTURE_DIR     P1DIR

#define CAPTURE_PIN     BIT4

static void timer_init(void);

static void timer_enable(const bool value);

static void led_init(void);

static void led_toggle(void);

static __interrupt void timer_0_isr_1(void);

int main (void)

{

    WDTCTL = WDTPW + WDTHOLD;

    timer_init();

    led_init();

    __enable_interrupt();

    while (true) {

        timer_enable(true);

        timer_enable(false);

    }

}

#pragma vector=TIMER0_A1_VECTOR

static __interrupt void timer_0_isr_1(void)

{

    // TA0IV is reset (or set to the next value in priority) upon read.

    // The highest priority interrupt's flag is also reset automatically.

    if (TA0IV == 0) {

        // Strange value for TA0IV!

        led_toggle();

    }

}

static void timer_init(void)

{

    TA0CTL = TASSEL__SMCLK  // Source clock.

        | ID__2             // Clock divided by two.

        | MC__CONTINUOUS    // Continuous mode to 0xFFFF.

        | TACLR;            // Reset.

    CAPTURE_SEL |= CAPTURE_PIN;

    CAPTURE_DIR &= ~CAPTURE_PIN;

    TA0CCTL3 = CM_3 // CM_3: capture on both edges.

        | CCIS_0    // CCIS_0: CCIxA.

        | SCS       // SCS: synchronous capture.

        | CAP       // CAP: capture mode.

        | CCIE;     // CCIE: interrupts enabled.

}

static void timer_enable(const bool value)

{

    if (value) {

        TA0CCTL3 |= CCIE;

    } else {

        TA0CCTL3 &= ~CCIE;

    }

}

static void led_init(void)

{

    LED_PORT_SEL &= ~LED_PORT_PIN;

    LED_PORT_DIR |= LED_PORT_PIN;

    LED_PORT_OUT |= LED_PORT_PIN;

}

static void led_toggle(void)
{

    LED_PORT_OUT ^= LED_PORT_PIN;
}

That's how much I could reduce it, I'll see if I can trigger the problem in other than capture mode.

With the function timer_enable() above replaced with the following, the LED does not toggle at all:

static void timer_enable(const bool value)

{

    if (value) {

        TA0CCTL3 |= CCIE;

    } else {

        __disable_interrupt();

        TA0CCTL3 &= ~CCIE;

        __enable_interrupt();

    }

}

(What's up with code formatting on this site? I don't want to shift+Enter on all lines after I paste code!)

I could reproduce the problem on a msp430f5308 timer in up mode (interrupt on compare).

Here is the modified code:

#include <stdbool.h>
#include <stdint.h>
#include "msp430f5308.h"

#define LED_PORT_SEL    P1SEL
#define LED_PORT_DIR    P1DIR
#define LED_PORT_OUT    P1OUT
#define LED_PORT_PIN    BIT0

#define DEBUG_DIR       P1DIR
#define DEBUG_PORT      P1OUT
#define DEBUG_PIN       BIT4

static void timer_init(void);
static void timer_enable(const bool value);
static void led_init(void);
static void led_toggle(void);
static __interrupt void timer_0_isr_1(void);

int main (void)
{
    WDTCTL = WDTPW + WDTHOLD;
    timer_init();
    led_init();
    __enable_interrupt();

    while (true) {
        timer_enable(true);
        timer_enable(false);
    }
}

#pragma vector=TIMER0_A1_VECTOR
static __interrupt void timer_0_isr_1(void)
{
    // TA0IV is reset (or set to the next value in priority) upon read.
    // The highest priority interrupt's flag is also reset automatically.
    uint16_t vector = TA0IV;
    if (vector == 0) {
        // Strange value for TA0IV!
        led_toggle();
    } else if (vector == TA0IV_TA0CCR3) {
        DEBUG_PORT ^= DEBUG_PIN;
    }
}

static void timer_init(void)
{
    TA0CTL = TASSEL__SMCLK  // Source clock.
        | MC__UP            // Up to TA0CCR0.
        | TACLR;            // Reset.

    TA0CCR0 = 20000;        // Up mode limit.

    DEBUG_DIR |= DEBUG_PIN; // Output pin toggle on every timer interrupt.
    TA0CCR3 = TA0CCR0 - 1;  // Timer interrupt just before reaching max.
    TA0CCTL3 = CCIE;     // CCIE: interrupts enabled.
}

static void timer_enable(const bool value)
{
    if (value) {
        TA0CCTL3 |= CCIE;
    } else {
        TA0CCTL3 &= ~CCIE;
    }
}

static void led_init(void)
{
    LED_PORT_SEL &= ~LED_PORT_PIN;
    LED_PORT_DIR |= LED_PORT_PIN;
    LED_PORT_OUT |= LED_PORT_PIN;
}

static void led_toggle(void)
{
    LED_PORT_OUT ^= LED_PORT_PIN;
}

Your tests have definitely identified an abnormal behavior of Timer interrupt in the MSP430F5308 you used.

#1. An interrupt must only be granted after a CPU instruction is finished and before the next instruction is started.

#2. To grant a specific mask-able interrupt, (a) the GIE must be 1, (b) the IFG of that specific interrupt must be 1, and (c) the IE of that specific interrupt must also be 1.

In your F5308, I speculate (just a hunch without any proof) that rule 1 above is actually not violated. Also, the decision to grant the interrupt is made according to rule #2. However this decision is made a little too early -- before the current CPU instruction is finished. Thus after that CPU instruction is finished, a bogus interrupt is immediately granted.

I wish TI Engineers are as diligent as you.

That was a nice thing to say, thank you.

And thank you for taking the time to read and analyze what I write here.

Actually, the code examples I have seen from TI deal with case 0 for IV registers in the ISR. My guess is one of the three:
- TI's engineers are aware of the problem, and covered it in their examples.
- TI's engineers writing example code follow good coding practice and implement all valid values for the IV registers (the datasheet states that 0 is a valid value, although it does not make sense in the related ISR), and therefore never discovered the problem.
- Any mix of the above. Some of TI's engineers know about it, some don't.

I agree with your explanation, the decision to jump to ISR is made too early. I wonder though if that was not intentional, I suppose that it simplifies the chip's logic quite a bit.

If it was intentional, then the big mistake is not mentioning it in the datasheet.

It would be very interesting to know if other chips show the same behavior, or if it is specific to F530x / F5310.

I have not verified the topic on my 5438s (I mean, I din't even try), but with previous experience it sounds logical.

Both cited rules are in conflict with the MSP structure. The keyword is pipelining.

Even on 1x family MSPs, the users guide tells you to add a nop after an instruction that sets GIE before you actually enter the atomic operation it shall protect.

The reason is that if the target of an instruction is a processor register, the next isntruction is fetched in the same cycle that writes the result to the register.On 5x family, this pipelining has been extended to even MOV and BIT operations with non-register targets. (I guess it has something to do with some structural changes, including the flash fetch cache etc.)
So the next operation is already being queued when the GIE bit hasn't been set yet. If now an interrupt comes, it is granted, but first the already fetched instruction is executed.
This "violates" the first rule.

About the second rule, well, if an interrupt is granted, it will be executed. Even if the last clock cycle of the last instruction before the already granted interrupt is executed will clear the interrupt cause.
The other option would have been to disable pipelining, so the decision whether the grant an interrupt can be made exactly between two instructions. But this would slow down the processor significantly, adding otherwise unnecessary cycles to many instructions. The other option would have been to discard already fetched but not yet executed instructions when an interrupt is granted. This would have been easy for multi-cycle instructions, but for those which execute in one cycle (actually 2, but pipelined, so nly one effective cycle) this would have been a 'little' bit more difficult.

However, the IFG bits can be checked within the ISR (or the IV register), and also, what if the interrupt had happened a tick before? Or what if the IFG bit is reset due to a hardware event? USCI I2C UCNACKIFG is cleared by hardware when a start or stop condition is received.
So if using the IV register, always check for no interrupt.

It shouldn't be  aproblem anyway: Either you use the switch(even_in_range(x,y))  style to check the IV. Then the '0' case is handled automatically (as default, if you don't add your own '0' case), or you check for specific IV values, then the '0' reading is not tested and nothing is executed.
The only thing you shouldn't do is reading the IV register and then assume that it has only the expected value.

old_cow_yellow said:

However this decision is made a little too early -- before the current CPU instruction is finished.

IMHO, doubling the speed of register/register instructions (and speeding up many others by a significant amount) justifies the additional effort of checking for a bogus interrupt. (It's not really bogus - the IFG and IE bits were set - they just aren't anymore. And as I said, it can happen by hardware too)

Gauthier ��stervall said:

5.5.1.5 of the MSP430x5xx family specification says that such an operation has:
        length: 3
        No of cycles: 5

It is not clear for me what different things are achieved in these different five cycles.

I understand that some of these 5 steps should be:

1. Read instruction (1st word, the instruction opcode)
2. Read operand (2nd word, #0x10)
3. read target address (3rd word, TA0CCTL3 address)
4. read target value (TA0CCTL3 read)
5. perform operation and write back the result (TA0CCTL3 write)

One cylce for each data bus operation. You cannot be faster on a unified memory structure.

On some operands, however, the source operand is NOT stored as a separate word, but rather par tof the instruction. This is true for values that can be fetched from the constant generator. In this case, the operand is a register access and has one word and one cycle less. However, 0x10 isn't one of them (0x00, 0x01, 0x02, 0x04, 0x08, 0xff and 0xffff are - this includes the GIE bit, so setting/clearing it is a one-word instruction with register/register operation)
The compiler/assembler will automatically pick this smaller variant if possible, transparent to the user.