0

I'm programming a SPI communication with an external RF chip. The microcontroller is the model PIC24FJ64GA102 from Microchip.

I want to use the enhanced buffer mode of the SPI.

Problem: Get the received bytes out of the receive buffer.

Used SPI function:

void SPI1_get(uint8_t* data, uint16_t length) {
    uint16_t i = 0, l = length;
    uint8_t dummy;
    while (length > 0) {
        while (SPI1STATbits.SPITBF || SPI1STATbits.SPIRBF) {
            dummy = SPI1STAT;
        }
        do {
            SPI1BUF = 0xff;
        } while (SPI1STATbits.SPIRBF == 0 && --length > 0);
        do {
            while (SPI1STATbits.SRMPT == 0) {
            }
            data[i] = SPI1BUF;
            ++i;
        } while (i < l && SPI1STATbits.SRXMPT != 1);
    }
}

Here the calls:

uint8_t cmd[2]; cmd[0] = length; cmd[1] = address;
SPI1_put(cmd,2); // e.g: 0x02, 0x01
SPI1_get(buf,2); // e.g: 0x05, 0x01 (received data)

The communication is just fine, checked with an oscilloscope and SPI decoding module. The data on the SPI bus are like in the comment above: sent 0x02 0x01 0xff 0xff, received 0x00 0x00 0x05 0x01, but the function above does not correctly retrieve the data out of the receive buffer. I've already tested a lot of constellations of checking flags and interrupts but in the end the best result I can get is: 0x00 0x01 (only the last byte is correct).

Also already checked the errata sheet, where two SPI problems are mentioned which do not (should not) affect my code.

What the hell am I doing wrong?!

As requested here the SPI1_put() function:

void SPI1_put(uint8_t* data, uint16_t length) {
    uint16_t i = 0;
    uint8_t dummy;
    for (; i < length; ++i) {
        while (SPI1STATbits.SPITBF)
            ; // maybe change to (_SPI1BEC == 7) ?
        SPI1BUF = data[i];
        dummy = SPI1BUF; //dummy read
    }
}

[latest edit: 2015-02-05]

So today I was able to spend some more time on this particular issue, and came up with a port of ElderBug's suggestion, also taking care of the bugs mentioned in the errata sheet:

uint8_t out_buf[128];
uint8_t in_buf[128];

void SPI1_com(uint8_t* out, uint8_t* in, uint16_t out_len, uint16_t in_len) {
    uint16_t len = out_len + in_len;
    uint16_t sent = 0, recv = 0, i = 0;
//  while (!SPI1STATbits.SRXMPT)
    sent = SPI1BUF; // empty buffer
    sent = SPI1BUF; // empty buffer
    sent = 0;
    if (out != out_buf && out != 0)
        memcpy(out_buf, out, out_len);
    while (sent < len && recv < len) {
        if (SPI1STATbits.SPIBEC != 7 && sent < len) {
            SPI1BUF = out_buf[sent++];
        }
        if (!SPI1STATbits.SRXMPT && recv < len) {
            in_buf[recv] = SPI1BUF, recv++;
        }
    }
    if (in != 0) {
        for (i = 0; i < in_len; ++i) {
            in[i] = in_buf[out_len + i];
        }
//      memcpy(in, in_buf + out_len, in_len);
    }
    for (i = 0; i < len; ++i) {
        out_buf[i] = 0xff;
        in_buf[i] = 0xff;
    }
}

This code basically works. With an exception which I was not able to work around:

The communication works as follows:

  • 1byte: r/w-bit+length
  • 1byte: address
  • 1-127byte: data

So, when I read 1 byte from the slave chip, meaning sending 3 bytes (rw+len, address, dummybyte), the data I have in my in_buf buffer is 0xFF. Now a weird thing: When I just read one more byte, without changing anything (just one more dummy byte on the bus), the 1st byte of my in_buf has got the correct data. But this seems not to work always, because my program still stucks at some points.

I'm left sitting here with a lot of question marks.

Weird thing the 2nd: Reading 8 bytes, data correct in my buffer until the last byte, last byte is 0xFF, should be 0x00. Wtf?

PS: Already filed a ticket at Microchip for support in this issue.

qwc
  • 243
  • 1
  • 3
  • 12
  • Could you post the code for `SPI1_put` ? The problem can also come from there. – ElderBug Jan 22 '15 at 09:16
  • Just a wild guess: in `SPI1_put` function, shouldn't the `for` loop's step section have `i++` instead of `++i`? Because now you never process the 0th byte of the buffer. – Eimantas Jan 22 '15 at 10:42
  • That's just not correct. In a for loop it doesn't matter if your counter is incremented with `c++` or `++c`. And as I wrote in my question, the data on the bus itself (measured with a scope) is completely correct. The problem here is to get the received data out of the **SPI enhanced buffer**, not the communication itself. – qwc Jan 22 '15 at 11:10

1 Answers1

2

There is a problem in your SPI1_put.

In your SPI1_put function, you read SPI1BUF right after starting the transfer, and thus you read when the transfer is not complete (correct me if I'm wrong, but reading when the FIFO is empty doesn't block and just return the previous byte). The function should be something like :

void SPI1_put(uint8_t* data, uint16_t length) {
    uint16_t sent = 0;
    uint16_t rec = 0;
    uint8_t dummy;
    while(1)
    {
        if( !SPI1STATbits.SPITBF && sent < length )
            SPI1BUF = data[sent++];
        if( !SPI1STATbits.SRXMPT && rec < length )
            dummy = SPI1BUF, rec++;
        if( sent == length && rec == length )
            break;
    }
}

About the SPI1_get, I think it's fine, but I'm not sure. The thing is, it should look very similar to SPI1_put, because SPI is really symmetric in how it works :

void SPI1_get(uint8_t* data, uint16_t length) {
    uint16_t sent = 0;
    uint16_t rec = 0;
    while(1)
    {
        if( !SPI1STATbits.SPITBF && sent < length )
            SPI1BUF = 0xff, sent++;
        if( !SPI1STATbits.SRXMPT && rec < length )
            data[rec++] = SPI1BUF;
        if( sent == length && rec == length )
            break;
    }
}

Overall, since you only do synchronous calls, the enhanced buffer is not really useful. You could have simpler functions with the same results just by doing 'send, wait, receive' for each byte.

EDIT after your update :

Your new code looks correct, except for the condition in the while that you did wrong. Here is the corrected code with some modifications with comments that may improve it (removed the buffer copies) :

// No additional buffers

void SPI1_com(uint8_t* out, uint8_t* in, uint16_t out_len, uint16_t in_len) {
    uint16_t len = out_len + in_len;
    uint16_t sent = 0, recv = 0, i;
    uint16_t dummy;


    // After the call, SPI comms should have ended and RX FIFO should be empty.
    // If SPI1STATbits.SRXMPT is not 1 here,
    // it means there is a problem in the code. Report it somehow for debug ?
    // if( !SPI1STATbits.SRXMPT )
    //     error!

    // This loop is harmless but shouldnt be needed
    //while (!SPI1STATbits.SRXMPT)
        //dummy = SPI1BUF; // empty buffer

    i = 0;
    while (sent < len || recv < len) {
        if (SPI1STATbits.SPIBEC != 7 && sent < len) {
            // Here we are out of the buffer when sent>=out_len,
            // but it's ok because random bytes are fine here
            SPI1BUF = out[sent++];
        }
        if (!SPI1STATbits.SRXMPT && recv < len) {
            if( recv < out_len )
                // Before out_len, discard the data
                dummy = SPI1BUF;
            else
                // After out_len, store the data in in[]
                in[i++] = SPI1BUF;
            recv++;
        }
    }

}

Does this correct any bug ? Also, this may sound stupid, but are you calling that correctly ? I don't know what you did, but for your 1 byte read, it should be SPI1_com(two_byte_buffer,one_byte_buffer,2,1);.

One more point : it seems that you always try to empty the RX FIFO at the beginning. Is there actually something in the FIFO ? SPI1STATbits.SRXMPT MUST be 1 without even trying to empty. If it isn't, there IS a bug somewhere in software. Do you have any other code using the SPI ?

ElderBug
  • 5,926
  • 16
  • 25
  • The old version of my software does the 'send, wait, receive' cycle for each byte. But the send/receive part has got a wait delay, which I hope to eliminate through the buffer, because that are ~1.5µs for each byte transfered. – qwc Jan 22 '15 at 11:29
  • @qwc What is the link speed ? 1.5µs is good if it is < 10MHz. – ElderBug Jan 22 '15 at 12:02
  • The link speed is 8Mhz. The device has to save time and energy, so the goal is to eliminate that 1.5µs delay between SPI bytes. The slave device doesn't care about any delay according to manual and experts of the manufacturer. – qwc Jan 22 '15 at 12:25
  • @qwc Oh, 1.5µs delay between bytes is indeed not optimal (i thought it was 1.5µs per byte overall). Nevertheless, a well done 'send, wait, receive' (waiting on the right status bit) should not induce delay (because `SP1BUF` act as a 1 byte buffer), but enhanced buffer is good too. I didn't test the code I posted, so tell me if you still have problems. – ElderBug Jan 22 '15 at 12:34
  • I did not use your code, but found a partial solution, see my question. When it's not partial anymore I'll post an answer... ;) Hope I'll get that working soon... – qwc Jan 22 '15 at 12:59
  • @qwc The solution you posted still has problems. In the 'put' function, you don't actually wait for the byte, you just read all there is to read. For the last byte, the FIFO is still empty and you exit without receiving. In the 'get', you should not empty the FIFO at the start, because if you have to, it means something is wrong elsewhere. The code I posted is only an example, but if I didn't make any mistake, it should cover all the cases and use maximal buffering (even with length > 8). – ElderBug Jan 22 '15 at 13:23
  • Briefly tested your code, but the watchdog came, when used unchanged. Have to work on another bug for the rest of the day, so I'll leave that branch for some time where it is now. – qwc Jan 22 '15 at 14:25
  • qwc: try DMA, you have two bits delay between bytes there. 1 bit per byte in 16-bit mode. – Marco van de Voort Jan 26 '15 at 15:23
  • @MarcovandeVoort There shouldn't be any delay if buffering is used correctly. DMA could be a good idea (although a bit overkill for synchronous function), but his PIC doesn't have DMA, so it's out of the question. – ElderBug Jan 26 '15 at 15:50
  • IIRC there is some overhead due to the nested looping. (I use dspic33e, the free compiler and 16 MHz spi clock btw), but for short sequences you can do better by having a special case of stuffing the fifo (preferably in mode16) and then reading back. Since you leave the FIFO empty at the end of a routine like this that is fairly safe for synchronous apps. – Marco van de Voort Jan 26 '15 at 20:23
  • Used your suggestion @ElderBug and optimized it a bit, regarding bugs from the errata sheet. But I still have weird bugs. – qwc Feb 05 '15 at 12:54
  • @qwc Re-updated my answer, because I suddenly found a fatal bug. – ElderBug Feb 05 '15 at 15:55
  • Oh my f*** god!!!!!!!! How the f*** could I overlook THAT! I am god damn ashamed of this. I've become blind for this obvious bug. Sorry for that, from the bottom of my heart! – qwc Feb 06 '15 at 07:57
  • Regarding your concern about emptying the receive buffer, please look at the errata sheet of my PIC24 type: http://ww1.microchip.com/downloads/en/DeviceDoc/80000486h.pdf – qwc Feb 06 '15 at 08:16
  • @qwc Yeah I looked at the errata and the function shouldn't be affected, as it only happens when the PIC wake up with SPI already enabled. The proposed workaround is just to disable SPI before sleeping. – ElderBug Feb 06 '15 at 08:21
  • @qwc Oh, and I don't know if you used the code I reposted, but the `recv++` should be at the end (edited). – ElderBug Feb 06 '15 at 08:24
  • I seldom use provided code directly. So I reworked my code after reading your suggestion and recognized that little glitch immediately. Sadly I did not recognize the main problem of a simple logical OR....... – qwc Feb 06 '15 at 08:50
  • @qwc If you use your code with buffers, note that at the beginning you test `out != out_buf`, but at the end, your clearing loop may break everything if `out == out_buf` or `in == in_buf`. Not necessarily a problem, see yourself if it is. – ElderBug Feb 06 '15 at 08:57