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]

Try correcting this error and see if that helps:

void mcp2515_spi_transfer(uint8_t p_tx_data, uint8_t p_rx_data, const uint16_t len)
{
    // Reset rx buffer and transfer done flag
    memset(&p_rx_data, 0, len);
    
    NRF_LOG_INFO("tx data %x\n", p_tx_data);

    APP_ERROR_CHECK(nrf_drv_spi_transfer(&can_spi, &p_tx_data, len, &p_rx_data, len));

Should be

void mcp2515_spi_transfer(uint8_t *p_tx_data, uint8_t *p_rx_data, const uint16_t len)
{
    // Reset rx buffer and transfer done flag
    memset(p_rx_data, 0, len);
    
    NRF_LOG_INFO("tx data %x\n", p_tx_data);

    APP_ERROR_CHECK(nrf_drv_spi_transfer(&can_spi, p_tx_data, len, p_rx_data, len));

In the first case you are using the address of a byte parameter on the stack, not the address of the buffers you require.

You might also try increasing the drive of the SCK output pin to H0H1, just until you have everything working. This must be done after initialising the SPI which sets that output pin to S0S1. If the CAN chip is off-board maybe long leads or capacitance issues are corrupting transfers, although unlikely at 250kHz