Forum Rule: Always post complete source code & details to reproduce any issue!
Results 1 to 9 of 9

Thread: SPI transfer16 bug

  1. #1
    Junior Member
    Join Date
    Jul 2020
    Posts
    2

    SPI transfer16 bug

    Hello,

    while trying to address an external eeprom module from a Teensy 4.0 I noticed some strange behaviour of the SPI library functions transfer/transfer16:
    The SPI interface seems to get stuck with a 16 bit frame size after two consecutive calls to transfer16. Following calls to transfer
    now clock out 16 bits instead of the expected 8.

    I managed to reproduce this behaviour with the following code snipplet:
    Code:
    SPI.beginTransaction(SPISettings(115200, MSBFIRST, SPI_MODE3));
    digitalWrite(21, LOW); 
    
    SPI.transfer16(0x00FF);
    SPI.transfer16(0xFF00);
    SPI.transfer(0xAA);
    
    digitalWrite(21, HIGH);
    SPI.endTransaction();
    See the extra 0x00 before sending 0xAA:
    Click image for larger version. 

Name:	transfer16_x2.jpg 
Views:	8 
Size:	77.5 KB 
ID:	21210

    Commening out one of the calls to SPI.transfer16 I get the expected behaviour:
    Click image for larger version. 

Name:	transfer16.jpg 
Views:	4 
Size:	70.8 KB 
ID:	21211


    After digging around in the SPI code and the chip manual it seems transfer16 fails to properly reset the TCR register value after modifying the FRAMESZ bits.
    If I understand correctly this is caused by a problem with reading this register as described in 48.4.1.15.2 of the manual:
    The register write of the first transfer16 call is still stuck on the FIFO when the second call reads TCR so the FRAMESZ bits are still set to 15.
    So the second call incorrectly resets the FRAMESZ bits to 15 instead of 7.

    I am unsure about a possible fix though:
    • Use the strategies suggested in the manual to avoid the read problem
    • Reuse the TCR value from SPISettings


    I hope someone with a bit more experience can weigh in on this

  2. #2
    Senior Member+ defragster's Avatar
    Join Date
    Feb 2015
    Posts
    12,232
    When @KurtE or Paul sees this there may be an answer - it seems this was recently discovered and addressed - there may be a PULL REQUEST {pending or merged} on github CORES for Paul.

    edit>: Quick check no PR appears for this? And not finding the thread it seemed I saw

  3. #3
    Senior Member+ KurtE's Avatar
    Join Date
    Jan 2014
    Posts
    7,476
    Confirmed I am seeing it with your example...

    Working on fix.

  4. #4
    Senior Member+ defragster's Avatar
    Join Date
    Feb 2015
    Posts
    12,232
    Cool. Seem to recall now the other transfer16() issue was on T_LC display driver or something.

    Good luck with a fix.

  5. #5
    Senior Member+ KurtE's Avatar
    Join Date
    Jan 2014
    Posts
    7,476
    @Paul @Mithradir, @defragster and others, Again I know the issue, I know ways to fix it, but not sure the best way to go...

    The current code for transfer16:
    Code:
    	uint16_t transfer16(uint16_t data) {
    		uint32_t tcr = port().TCR;
    		port().TCR = (tcr & 0xfffff000) | LPSPI_TCR_FRAMESZ(15);  // turn on 16 bit mode 
    		port().TDR = data;		// output 16 bit data.
    		while ((port().RSR & LPSPI_RSR_RXEMPTY)) ;	// wait while the RSR fifo is empty...
    		port().TCR = tcr;	// restore back
    		return port().RDR;
    	}
    So let me unwind the two 16 bit calls... With looking at the contents of the TCR register in between.
    Code:
      SPI.transfer16(0x00FF);
      uint32_t tcr1 = IMXRT_LPSPI4_S.TCR;
      SPI.transfer16(0xFF00);
    Actually lets just look at the start and end of the two calls with my assignment in between
    The line: uint32_t tcr = port().TCR;
    tcr = c0000007 the important part is that last 7 which says we are outputting 8 bits.

    We give it a new value: port().TCR = (tcr & 0xfffff000) | LPSPI_TCR_FRAMESZ(15);
    SO now logically TCR is c000000f

    We then output our data a 16 bits. We then try to assign TCR back to 8 bits so we output: IMXRT_LPSPI4_S.TCR = c0000007
    and we return the data.

    Now we enter it again (and or we look at the current value of TCR...

    So here the interesting question suppose you do:
    Code:
        IMXRT_LPSPI4_S.TCR = 0xc000000f;
        IMXRT_LPSPI4_S.TDR = 0x00ff;
        IMXRT_LPSPI4_S.TCR = 0xc0000007;
       uint32_t tcr1 = IMXRT_LPSPI4_S.TCR;
       Serial.println(tcr1, HEX);
    What would you guess would print out for tcr1?

    It will actually mostly likely print out as: c000000f

    Why? Because assigning a value to TCR does not acutely assign a value to TCR, instead it is put on the OUTPUT Queue to be processed when appropriate and until that time if you query TCR it will give you actual in use version of TCR...

    Or as shown in the Reference Manual:
    47.4.1.15.2 Function
    Writes to either the Transmit Command Register or Transmit Data Register will push the
    data into the transmit FIFO, in the order that the data are written. Command Register
    writes will be tagged and cause the command register to update, after that entry reaches
    the top of the FIFO. This allows changes to the command word and the transmit data
    itself to be interleaved. Changing the command word will cause all subsequent SPI bus
    transfers to be performed using the new command word.
    • In master mode, writing a new command word does not initiate a new transfer,
    unless TXMSK is set. Transfers are initiated by transmit data in the transmit FIFO, or
    by a new command word (with TXMSK set). Hardware will clear TXMSK when the
    LPSPI_PCS negates.
    So when you do two transfer16s in a row the second one will see that it is still Physically 16 bits and restore it to that, which is obviously not what is wanted.

    I ran into similar issues when doing some of the display drivers. Tried several work around approaches and saw to solutions.

    a) Every time you call transfer you set the TCR to 8(7). Every time you do a transfer16 you set to 16... Should work, lots more output to the queue than needed...

    b) do like I do in the display drivers. I have a member variable as part of the class, which is my logical state for TCR. And when I call transfer I say am I in 8 bit mode, Yes continue, No set to 8 bit mode. Likewise for Transfer16. except 16 instead of 8. This minimizes a lot of these going back and forth.

    There are a few different problems, more with b) Is suppose someone other code changes the state of things and I believe I am in one state but actually we are in another... I solved this in drivers by when you call something like beginTransaction, the internal state is set to a state which either transfer or transfer16 will cause a new value to loaded into TCR...

    Another problem which I saw is suppose you have a sketch that wants to talk to an SPI device in 6 bit transfers. Your code could update TCR to the appropriate setting and then transfer would typically leave it alone. This updated approach would have us undo that.

    Thoughts?

  6. #6
    Senior Member+ defragster's Avatar
    Join Date
    Feb 2015
    Posts
    12,232
    Is "a) Every time you call transfer you set ..." just an internal no transmit operation?

    If so that may be faster than compares or tests or state storage. The only thing keeping it queued is data send in progress?

    How long does:
    loop ( 100 times ) {
    set TCR8(7)
    set TCR16(15)
    }
    take when no data is queued? Assuming that doesn't cycle any data? { "In master mode, writing a new command word does not initiate a new transfer, unless TXMSK is set." }

  7. #7
    Senior Member+ KurtE's Avatar
    Join Date
    Jan 2014
    Posts
    7,476
    Quick update, I put up a version in the branch: https://github.com/KurtE/SPI/tree/T4...transfer16_fix

    that tries to maintain the state of was last transfer 8 or 16 bits and if we are transferring the same size than don't need to update TCR.

    This also implies that if the user calls transfer16 and then plays with TDR register and assumes that TCR is setup for 8 bits... Could be an issue. But I doubt that anyone does this.

    Anyway would be good if we played a bit with this version and make sure it fixes your problem and does not create new ones.

    Kurt

  8. #8
    Senior Member+ KurtE's Avatar
    Join Date
    Jan 2014
    Posts
    7,476
    Forgot to show an update logic analyzer output...

    Click image for larger version. 

Name:	screenshot.jpg 
Views:	11 
Size:	68.2 KB 
ID:	21214

    Just an FYI - Most of the time now I use Saleae's new Alpha/Beta which has lots of new interesting stuff:
    I believe you can download from their forum area: https://discuss.saleae.com/

    Currently I just wait for their version to tell me there is a new version...

  9. #9
    Junior Member
    Join Date
    Jul 2020
    Posts
    2
    Thanks for the quick feedback. @KurtE I'll try out your changes when I find some time this weekend.

    I think the case where the TCR is changed by code outside of the SPI lib can only be handled transparently when reading TCR is guaranteed to produce the latest value.
    Looking at 48.4.1.15.2 this is only the case after TXCOUNT in FSR reads 0:
    Avoid register reading problems: Reading the Transmit Command Register will return
    the current state of the command register. Reading the Transmit Command Register at the
    same time that the Transmit Command Register is loaded from the transmit FIFO, can
    return an incorrect Transmit Command Register value. It is recommended:
    to either read the Transmit Command Register when the transmit FIFO is empty,
    • or to read the Transmit Command Register more than once and then compare the
    returned values.
    But I think the SPI library should only be responsible for properly transferring data in 8/16 bit mode. In my opinion the user should expect issues when fiddling directly with chip hardware while also using a library to access the same functionality.

Posting Permissions

  • You may not post new threads
  • You may not post replies
  • You may not post attachments
  • You may not edit your posts
  •