Incorrect interrupt handling in MSP430 (X2 CPU) USCI ISR

[jns-]

Michiel Konstapel reported this issue back in 2009. However, his fix newer made it into the repository.

On MSP430F2x devices USCIAx and USCIBx modules share the same interrupt vector. But the ISR doesn't take this into account. Problems occur if code of one module is interrupt driven an the other one is polling flags.

Michiel gives a good explanation under which circumstances things can go wrong:

http://mail.millennium.berkeley.edu/pipermail/tinyos-devel/2009-December/004200.html

The ISR should not only test if interrupt flags are set but also make sure that the interrupts are actually enabled, before servicing the request.

Michiel suggests to test enable bits in RX ISR in the following way:

TOSH_SIGNAL(USCIAB1RX_VECTOR) {
  uint8_t temp;
  if ((UC1IFG & UCA1RXIFG) && (UC1IE & UCA1RXIE)){
    temp = UCA1RXBUF;
    signal UsciA.rxDone(temp);
  }
  if ((UC1IFG & UCB1RXIFG) && (UC1IE & UCB1RXIE)){
    temp = UCB1RXBUF;
    signal UsciB.rxDone(temp);
  }
}

Even more surprising is the TX ISR. I don't understand why the RX flags are tested. I think this is not necessary and could potentially signal a txDone() to the wrong component. If UCA1TXBUF and UCA1RXBUF are full (UCA1TXIFG cleared, UCA1RXIFG set) and UCB1TXIFG invokes the ISR UsciA.txDone() and UsciB.txDone() are signaled.

TOSH_SIGNAL(USCIAB1TX_VECTOR) {
  if ((UC1IFG & UCA1TXIFG) | (UC1IFG & UCA1RXIFG)){
    signal UsciA.txDone();
  }
  if ((UC1IFG & UCB1TXIFG) | (UC1IFG & UCB1RXIFG)){
    signal UsciB.txDone();
  }
}

Moreover, Michiel's findings also apply to the TX ISR and should be changed to something like:

TOSH_SIGNAL(USCIAB1TX_VECTOR) {
  if ((UC1IFG & UCA1TXIFG) && (UC1IE & UCA1TXIE)){
    signal UsciA.txDone();
  }
  if ((UC1IFG & UCB1TXIFG) && (UC1IE & UCB1TXIE)){
    signal UsciB.txDone();
  }
}

[alignan]

I didn't receive an assignment notification, sorry for noticing late, I'll take a look next week.

[jns-]

The proposed solution solves only part of the problem and introduces new issues, i.e. it breaks interrupt handling in I2C mode. I put together some information about MSP430x2xx USCI interrupt flag registers and the corresponding mapping to interrupt vector addresses (see http://decentlab.com/pastebin/msp430x2xx_USCI_Interrupts.pdf). Depending on the USCI_Bx mode (SPI or I2C) UCBxRXIF request two different ISRs. Therefore the ISRs have to check which mode the USCI_Bx actually is in.

I rewrote the HplMsp430UsciAB1RawInterruptsP module to take this behavior into account (see http://decentlab.com/pastebin/HplMsp430UsciAB1RawInterruptsP.nc). I am not sure if this solution covers everything or has any side effects. Moreover, I did no extensive testing but at least it brought I2C back to life.

[cire831]

One of the problems with the x2 cpus is the weird interrupt structure. What vectors for the usci do what changes depending on what mode the usci is in.

I've written an overview of the differences between the x2 and x5 uscis in https://github.com/tp-freeforall/prod/blob/tp-master/tos/chips/msp430/02_Serial

[jns-]

The behavior for USCI_Ax and USCI_Bx in SPI mode is consistent. One vector for USCIABxRXIF and one for USCIABxTXIF. Things become really wired if USCI_Bx is switched to I2C. In this mode there is only one vector for USCIABxRXIF and USCIABxTXIF. However, this could be handled by the ISRs in a way as proposed.

On 2013-04-17 10:52, Eric Decker wrote:

One of the problems with the x2 cpus is the weird interrupt structure. What vectors for the usci do what changes depending on what mode the usci is in.

I've written an overview of the differences between the x2 and x5 uscis in https://github.com/tp-freeforall/prod/blob/tp-master/tos/chips/msp430/02_Serial

— Reply to this email directly or view it on GitHub https://github.com/tinyos/tinyos-main/issues/172#issuecomment-16494211.

[alignan]

Can you please specify which toolchain are you using for tests? I actually noticed that TestIntADXL345 seems to be broken using msp430-47 and msp430-46 (4.6.3 version), but with old msp430-z1 (3.2.3 version) works OK with both current code and the suggested fix.

[jns-]

I am using msp430-gcc (GCC) 4.6.3 20120301 (mspgcc LTS 20120406 unpatched).

The old code works ok as long as no interrupt flag polling is used. Therefore, the enable flags should be checked before signaling events in order to make sure the interrupt is actually enabled.

Checking whether the USCI is in SPI or I2C mode is possibly unnecessary but does not harm.

On 2013-04-18 15:35, Antonio Lignan wrote:

Can you please specify which toolchain are you using for tests? I actually noticed that TestIntADXL345 https://github.com/tinyos/tinyos-main/tree/master/apps/tests/z1/Accelerometer/TestIntADXL345 seems to be broken using msp430-47 and msp430-46 (4.6.3 version), but with old msp430-z1 (3.2.3 version) works OK with both current code and the suggested fix.

— Reply to this email directly or view it on GitHub https://github.com/tinyos/tinyos-main/issues/172#issuecomment-16576567.

[alignan]

OK, as suggested does no harm, I'll be pushing your fix soon.

[jns-]

The same changes also apply for USCIA/B0. Could you also update tos/chips/msp430/x2xxx/usci/HplMsp430UsciAB0RawInterruptsP.nc