SPI read and write multiple bytes of varying lengths with MCP2515

[BendaEng]

Hi, 

Recently I was able to get a single read and write SPI operation to work properly with a custom nRF52832 (BL652) with SPI connection to MCP2515 (CAN SPI) using the nrf_drv_spi.c/h. (Solved through the following: https://devzone.nordicsemi.com/f/nordic-q-a/52046/nrf52832-spi-with-mcp2515-nrf_drv_spi_transfer-seems-to-send-properly-but-not-receiving-in-all-cases). I'm now trying to get multiple read and writing of bytes (of varying lengths and multiple different buffers) to work. 

I wrote the following code sections to modify a .cpp library (https://github.com/Seeed-Studio/CAN_BUS_Shield/blob/master/mcp_can.cpp) function of mcp2515_readRegisterS and  mcp2515_setRegisterS to try utilize the mcp2515 auto increment of address-pointer and repeated reading/writing from/to a register. 

However, I'm not sure if this is the proper way to implement and it doesn't seem to be giving back proper data. Since the previous method wrote multiple separate times (and wasn't working in my single read/write) I've attempted to append the arrays together for each send. The commented code is how it was done in the .cpp library. 

1. mcp2515_readRegisterS

/*********************************************************************************************************
** Function name:           mcp2515_readRegisterS
** Descriptions:            read registerS
*********************************************************************************************************/

uint8_t mcp2515_readRegisterS(const uint8_t address, uint8_t values[], const uint8_t n)
{
    //byte i;
    uint8_t i; 
      
    uint8_t m_tx_buf[] = {MCP_READ, address};
    //uint8_t m_tx_buf[] = {address};
    uint8_t m_rx_buf[n]; 
    uint8_t m_tx_length = sizeof(m_tx_buf);
    uint8_t m_rx_length = sizeof(m_rx_buf);

    memset(m_rx_buf, 0, m_rx_length);
        
    spi_xfer_done = false;

    APP_ERROR_CHECK(nrf_drv_spi_transfer(&can_spi, m_tx_buf, m_tx_length, m_rx_buf, m_rx_length));

     while (!spi_xfer_done)
        {
            __WFE();
        }  

    //spi_readwrite(MCP_READ);
    //spi_readwrite(address);
    // mcp2515 has auto-increment of address-pointer
    for (i=0; i<n-2 && i<CAN_MAX_CHAR_IN_MESSAGE-2; i++)
    {
      //values[i] = spi_read();
      return values[i] = m_rx_buf[i+2];
    }

    /* NOT SURE IF CORRECT YET*/
}

***As a test I used the following code using mcp2515_readRegisterS:

   // Enter main loop.
    for (;;)
    {
        uint8_t values[5]; 
        mcp2515_readRegisterS(MCP_READ_STATUS, values, 5);
        
        for(uint8_t i = 0; i<5; i++)
          {
            NRF_LOG_INFO("Data Received %d 0x%x", i, values[i]);
          }
          nrf_delay_ms(10);

        idle_state_handle();
    }

This was the output: 

<info> app: Data Received 0 0x0
<info> app: Data Received 1 0x7
<info> app: Data Received 2 0x10
<info> app: Data Received 3 0x3
<info> app: Data Received 4 0x3
<info> app: Transfer completed.

Note: This doesn't seem correct, since I'm expecting the data to be repeated for 5 bytes. Maybe this is the address auto-incremented return? However, I have tried to just send the "address" (see commended //uint8_t m_tx_buf[] = {address}...in this case an instruction of MCP_READ_STATUS), but the results were the same. 

2. mcp2515_setRegisterS

/*********************************************************************************************************
** Function name:           mcp2515_setRegisterS
** Descriptions:            set registerS
*********************************************************************************************************/

void mcp2515_setRegisterS(const uint8_t address, const uint8_t values[], const uint8_t n)
{
    uint8_t i;

    uint8_t tx_buf[2] = {MCP_WRITE, address};
    uint8_t* m_tx_buf = malloc(2+n * sizeof(uint8_t));

    memcpy(m_tx_buf, tx_buf, 2 * sizeof(uint8_t)); //copy 2 uint8_t from tx_buf to m_tx_buf
    memcpy(m_tx_buf + n, values, n * sizeof(uint8_t)); //copy n uint8_t from values to m_tx_buf
        
    uint8_t m_tx_length = sizeof(m_tx_buf);

    spi_xfer_done = false;

    APP_ERROR_CHECK(nrf_drv_spi_transfer(&can_spi, m_tx_buf, m_tx_length, NULL, 0));

    while (!spi_xfer_done)
      {
          __WFE();
      }  

    //uint8_t i;
    //spi_readwrite(MCP_WRITE);
    //spi_readwrite(address);
    //for (i=0; i<n; i++)
      //{
        //spi_readwrite(values[i]);
      //}

    /* NOT SURE IF CORRECT YET*/
}

The read and set Registers seem like they should work, but maybe I'm missing something in how auto incremented/repeated SPI operations should function.

The other way I'm trying to implement multiple byte reading/writing is with mcp2515_read_canMsg (not modified yet) and mcp2515_write_canMsg. These functions in the .cpp library contained many more steps that readRegisterS and setRegisterS.

The write_canMsg function at it's core does the following in order. 

1. Write the 1 value of "load_address"

2. Write the 4 values of "tbufdata[]" sequentially

3. Write the 1 value of "dlc"

4. Write the 'len' values of "buf[]"

I've attempted to modify in the following by appending the arrays. Is that the proper way to treat multiple writing operations that happen sequentially in nRF52832? Or is there a more defined way to approach this? See the following code. 

/*********************************************************************************************************
** Function name:           mcp2515_write_canMsg
** Descriptions:            write msg
**                          Note! There is no check for right address!
*********************************************************************************************************/

void mcp2515_write_canMsg(const uint8_t buffer_sidh_addr, unsigned long id, uint8_t ext, uint8_t rtrBit, uint8_t len, volatile const uint8_t *buf)
{
  uint8_t load_addr=txSidhToTxLoad(buffer_sidh_addr);
  uint8_t tbufdata[4];
  uint8_t dlc = len | ( rtrBit ? MCP_RTR_MASK : 0 ) ;
  uint8_t i;

  mcp2515_id_to_buf(ext,id,tbufdata);

    uint8_t* m_tx_buf = malloc(6+len * sizeof(uint8_t));

    memcpy(m_tx_buf, &load_addr, 1 * sizeof(uint8_t)); //copy 1 uint8_t from load_addr to m_tx_buf[0]
    memcpy(m_tx_buf + 1, tbufdata, 4 * sizeof(uint8_t)); //copy 4 uint8_t from tbufdata to m_tx_buf[1] -> m_tx_buf[4]
    memcpy(m_tx_buf + 5, &dlc, 1 * sizeof(uint8_t)); //copy 1 uint8_t from dlc to m_tx_buf[5]
    memcpy(m_tx_buf + 6, &buf, len * sizeof(uint8_t)); //copy n uint8_t from values to m_tx_buf[6] -> m_tx_buf[len]
        
    uint8_t m_tx_length = sizeof(m_tx_buf);

    spi_xfer_done = false;

    APP_ERROR_CHECK(nrf_drv_spi_transfer(&can_spi, m_tx_buf, m_tx_length, NULL, 0));

    while (!spi_xfer_done)
      {
          __WFE();
      }  

    /* NOT SURE IF CORRECT YET - the following is the previous .cpp code */
  /*
  spi_readwrite(load_addr);
  for (i = 0; i < 4; i++) spi_write(tbufdata[i]);
  spi_write(dlc);
  for (i = 0; i < len && i<CAN_MAX_CHAR_IN_MESSAGE; i++) spi_write(buf[i]);
*/

  mcp2515_start_transmit( buffer_sidh_addr );
  

}

The read_canMsg function at it's core does the following in order: 

1. Write "buffer_load_addr"

2. Read 4 values into "tbufdata[]"

3. Read 1 value into "pMsgSize"

4. Read 'len' values into "buf[]"

I haven't started to modify this yet, but if the process from the readRegisterS is correct I imagine it would be similar. But, how would the nrf_spi_drv handle reading all these sequentially? Would setting the size of the rx_buf for the size of the values work? (i.e. uint8_t* m_rx_buf = malloc(6+len * sizeof(uint8_t)); And then read the appropriate expected locations of the data into the tbufdata[], pMsgSize, and buf[] be the way to approach this? See the following un-modified way the read_canMsg was done in the .cpp library. 

/*********************************************************************************************************
** Function name:           mcp2515_read_canMsg
** Descriptions:            read message
*********************************************************************************************************/

void mcp2515_read_canMsg( const uint8_t buffer_load_addr, volatile unsigned long *id, volatile uint8_t *ext, volatile uint8_t *rtrBit, volatile uint8_t *len, volatile uint8_t *buf)        // read can msg 
{
  
  uint8_t tbufdata[4];
  byte i;

  //MCP2515_SELECT();
  spi_readwrite(buffer_load_addr);
  // mcp2515 has auto-increment of address-pointer
  for (i = 0; i < 4; i++) tbufdata[i] = spi_read();

  *id = (tbufdata[MCP_SIDH] << 3) + (tbufdata[MCP_SIDL] >> 5);
  *ext = 0;
  if ( (tbufdata[MCP_SIDL] & MCP_TXB_EXIDE_M) ==  MCP_TXB_EXIDE_M )
  {
    // extended id                  
    *id = (*id << 2) + (tbufdata[MCP_SIDL] & 0x03);
    *id = (*id << 8) + tbufdata[MCP_EID8];
    *id = (*id << 8) + tbufdata[MCP_EID0];
    *ext = 1;
  }

  byte pMsgSize = spi_read();
  *len = pMsgSize & MCP_DLC_MASK;
  *rtrBit = (pMsgSize & MCP_RTR_MASK) ? 1 : 0;
  for (i = 0; i < *len && i<CAN_MAX_CHAR_IN_MESSAGE; i++) {
    buf[i] = spi_read();
  }

  MCP2515_UNSELECT();

}

What would be the correct way to do all this?

Thanks!

[hmolesworth]

I think READ and READ_RX_BUFFER both auto-increment starting at the supplied address, but READ_STATUS does not, it simply repeats the status byte for verification and error checking. I only had time for a quick look, and I see the return statement is misplaced which means you were looking at random data on the stack after the first byte:

Code above:
    for (i=0; i<n-2 && i<CAN_MAX_CHAR_IN_MESSAGE-2; i++)
    {
      return values[i] = m_rx_buf[i+2];
    }
Should be:
    for (i=0; i<n-2 && i<CAN_MAX_CHAR_IN_MESSAGE-2; i++)
    {
      values[i] = m_rx_buf[i+2];
    }
    return;

I assume MCP_READ is 0x03? Also I am not sure all compilers would handle sizeof() correctly when used on a passed parameter but that could be easily verified with a breakpoint

[BendaEng]

That was part of it, the return was misplaced. When ran the data received is the following (for READ_STATUS): 

<info> app: Data Received 0 0x0
<info> app: Data Received 1 0x0
<info> app: Data Received 2 0x0
<info> app: Data Received 3 0x3
<info> app: Data Received 4 0x3
<info> app: Transfer completed.

It seems like the sizeof() is being handled properly. I changed the values to how much I want to read and it doesn't make a difference. 

uint8_t m_tx_buf[] = {address};
    uint8_t m_rx_buf[n]; 
    uint8_t m_tx_length = 1; //sizeof(m_tx_buf);
    uint8_t m_rx_length = 5; //sizeof(m_rx_buf);

    memset(m_rx_buf, 0, m_rx_length);
        
    spi_xfer_done = false;

    APP_ERROR_CHECK(nrf_drv_spi_transfer(&can_spi, m_tx_buf, m_tx_length, m_rx_buf, m_rx_length));

     while (!spi_xfer_done)
        {
            __WFE();
        }  

Another interesting thing to note: When I change the '2' to '3' in the for loop I get the following output (the 0x10 corresponds to what I received previously: 

<info> app: Data Received 0 0x0
<info> app: Data Received 1 0x0
<info> app: Data Received 2 0x10
<info> app: Data Received 3 0x3
<info> app: Data Received 4 0x3
<info> app: Transfer completed.

for (i=0; i<n-3 && i<CAN_MAX_CHAR_IN_MESSAGE-3; i++)
    {
      //values[i] = spi_read();
      values[i] = m_rx_buf[i+3];
    }
    return;

Thanks!