• State Resolved
  • Locked Locked
  • Replies 7 replies
  • Subscribers 79 subscribers
  • Views 1080 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.

MSP430 MSPDRIVERLIB USCI masterReceiveMultiByteFinish-related functions use wrong operation order

Petre Rodan
Prodigy 150 points

Other Parts Discussed in Thread: MSP430F5529, CC430F5137, MSP430WARE, MSP430F5510


good morning everyone.

I took a pause in datasheet parsing only to find another bug, this time in the MSP430 DRIVERLIB.

bug description:
the USCI_B_I2C_masterReceiveMultiByteFinish() and USCI_B_I2C_masterReceiveMultiByteFinishWithTimeout() functions from DRIVERLIB/MSP430F5xx_6xx/usci_b_i2c.c v2.91.13.01 perform a read from the OFS_UCBxRXBUF register before the UCRXIFG flag is asserted. the consequences are that all the read operations that use those two functions will provide pure garbage.

proposed bug fix for the first function (patch):

--- driverlib/MSP430F5xx_6xx/usci_b_i2c.c 2020-02-27 20:48:53.000000000 +0200
+++ atlas430/driverlib/MSP430F5xx_6xx/usci_b_i2c.c 2021-12-05 17:33:18.264094116 +0200
@@ -446,16 +446,16 @@ uint8_t USCI_B_I2C_masterReceiveMultiByt
//Send stop condition.
HWREG8(baseAddress + OFS_UCBxCTL1) |= UCTXSTP;

- //Capture data from receive buffer after setting stop bit due to
- //MSP430 I2C critical timing.
- receiveData = HWREG8(baseAddress + OFS_UCBxRXBUF);
-
//Wait for Stop to finish
while (HWREG8(baseAddress + OFS_UCBxCTL1) & UCTXSTP);

//Wait for RX buffer
while (!(HWREG8(baseAddress + OFS_UCBxIFG) & UCRXIFG));

+ //Capture data from receive buffer after setting stop bit due to
+ //MSP430 I2C critical timing.
+ receiveData = HWREG8(baseAddress + OFS_UCBxRXBUF);
+
return receiveData;
}

to note is that this function appears 4 more times in this driverlib archive for different families and eusci/usci flavors and in all other places the read operation happens AFTER the IFG is checked.

unit test performed between a msp430f5529/cc430f5137 and a Cypress FM24V10 external i2c fram chip:

8 bytes written, 8 bytes read, repeated once.

Legend:
"i2c write" simply writes a buffer containing "0xa1, 0xa2, 0xa3, 0xa4, 0xa5, 0xa6, 0xa7, 0xa8" to the external fram chip
"i2c read" simply attempts to read 8 bytes from the same address of the external fram chip, and the unpatched USCI_B_I2C_masterReceiveMultiByteFinish() is used when the last byte is being read.
"(prog)" what the buffer inside the firmware contains
"(wire)" what can be seen with a logic analyzer on the wire
"[foo]" explanation
in an ideal world the 'i2c read' must provide the exact same buffer as what has been written with 'i2c write' since the start address for both functions is the same.

i2c write (prog): 0xa1 0xa2 0xa3 0xa4 0xa5 0xa6 0xa7 0xa8 [ok]
i2c write (wire): 0xa1 0xa2 0xa3 0xa4 0xa5 0xa6 0xa7 0xa8 ACK [ok]

i2c read (wire): 0xa1 0xa2 0xa3 0xa4 0xa5 0xa6 0xa7 0xa8 NACK [ok]
i2c read (prog): 0xa1 0xa2 0xa3 0xa4 0xa5 0xa6 0xa7 0xa7 [last byte is read wrong]

i2c write (prog): 0xa1 0xa2 0xa3 0xa4 0xa5 0xa6 0xa7 0xa8 [ok]
i2c write (wire): 0xa1 0xa2 0xa3 0xa4 0xa5 0xa6 0xa7 0xa8 ACK [ok]

i2c read (wire): 0xa1 0xa2 0xa3 0xa4 0xa5 0xa6 0xa7 NACK [fail, a NAK is issued after the 7-th byte, not the 8-th one]
i2c read (prog): 0xa8 0xa1 0xa2 0xa3 0xa4 0xa5 0xa6 0xa6 [0xa8 is garbage from last read operation, remaining bytes are shifted]

TLDR: so without the patch the two functions provide garbage, with the patch tested on CC430F5137 and MSP430F5529 everything is great. please fix.

  • 0
    TI__Expert 7080 points

    Hi 

    Please download the newest MSP430WARE and use the latest drivelib.

    MSP430WARE_3.80.14.01

  • 0 in reply to Allen Gao
    Prodigy 150 points

    that version of msp430ware contains the exact version of driverlib this bug belongs to (v2.91.13.01)

  • 0
    Prodigy 150 points

    just tested with msp430f5510 with the exact same results - the driverlib needs to perform the read after checking the IFG.

    the code I'm using is on github, here: github.com/.../cypress_fm24
    it's heavily based on pre-processing scripts, makefiles and the linux msp430 toolchain with no ccs support sorry.

    the problematic function is getting used at this line: github.com/.../i2c.c

  • 0 in reply to Petre Rodan
    TI__Expert 7080 points

    Hi Petre,

    I will have a test and then reply to you

  • +1 in reply to Petre Rodan
    TI__Expert 7080 points

    Hi Petre,

    There is no problem with the example drivelib project to receive multibyte data.

    As the example code. 

    #if defined(__TI_COMPILER_VERSION__) || defined(__IAR_SYSTEMS_ICC__)
#pragma vector=USCI_B0_VECTOR
__interrupt
#elif defined(__GNUC__)
__attribute__((interrupt(USCI_B0_VECTOR)))
#endif
void USCI_B0_ISR (void)
{
    switch (__even_in_range(UCB0IV,12)){
        case USCI_I2C_UCRXIFG:
        {
            //Decrement RX byte counter
            receiveCount--;

            if (receiveCount){
                if (receiveCount == 1) {
                    //Initiate end of reception -> Receive byte with NAK
                    *receiveBufferPointer++ =
                        USCI_B_I2C_masterReceiveMultiByteFinish(
                        		USCI_B0_BASE
                            );
                }
                else {
                    //Keep receiving one byte at a time
                    *receiveBufferPointer++ = USCI_B_I2C_masterReceiveMultiByteNext(
                    		USCI_B0_BASE
                        );
                }
            }
            else {
                //Receive last byte
                *receiveBufferPointer = USCI_B_I2C_masterReceiveMultiByteNext(
                		USCI_B0_BASE
                    );
                __bic_SR_register_on_exit(LPM0_bits);
            }
            break;
        }
    }
}

    When run to the <receiveData = HWREG8(baseAddress + OFS_UCBxRXBUF);> It is in interrupt routine and the RX flag already set. That means master has received the data from slave. Then send the stop signal and receive the final byte to RXBUFFER. 

    <while (!(HWREG8(baseAddress + OFS_UCBxIFG) & UCRXIFG));> This is to confirm that the last byte has already received to BUFFER.

  • 0 in reply to Allen Gao
    Prodigy 150 points

    Allen, I do appreciate your quick replies, I understand that there are a lot of tickets coming in, people tend to go on vacation and someone needs to close them but I respectfully disagree with your input on this ticket.

    please be so kind and open an internal ticket addressed to the maintainer of the driverlib and include all my information.

    I understand your reasoning and all the registers involved and your code looks like it might work. But there is another usecase under which the function will not work, as proven above on three different msp430s. and as per the links I provided in my previous post, I'm using the masterReceiveMultiByteFinish() functions in a non-interrupt driven environment. that particular check for UCRXIFG makes little sense in an interrupt implementation, since the function is called after the byte was actually received and the interrupt handler called already. In your example code the IFG check is redundant, in a non-interrupt implementation however the check is mandatory and must be performed before reading the UCBxRXBUF register.

    so, to get back on track. masterReceiveMultiByteFinish() appears 5 times in driverlib - in 4 of those the UCRXIFG check is correctly performed BEFORE the read from UCBxRXBUF. this simple fact already should have prompted you that something is amiss, yet you say that in that 1 case where the order is inverted it's somehow 'good'. in all those 4 cases that function can be used perfectly in a non-interrupt driven enviromnent since reading UCBxRXBUF will be correctly delayed until UCRXIFG gets asserted. in that 1 case I opened this bug for someone forgot why the check exists.

  • 0 in reply to Allen Gao
    Prodigy 150 points

    I'm starting to believe that the non-interrupt implementation was not such a great idea, I will remove it.

    sorry for the noise.

**Attention** This is a public forum