[BUG] SPI Slave driver issue

[FelipeMdeO]

Description / Steps to reproduce the issue

Hello All.

I am Facing some issues during slave driver development.
Here I can report and talk about change done in drivers/spi/spi_slave_driver.c. PR is: #10727

This PR do bind during file open and unbind during file close. The issues is that the bind is necessary to configure configure slave controller, so, in current master, all spi transaction when file is closed will be lost.

Before this PR all spi message is saved in lower half buffer, when we do read in top half, we get data from lower half, now lower half only get spi data while top half is waiting for.

@Donny9 and @gustavonihei let's talk about this PR, please?
@acassis Can you help me to check about expected behavior, Do we need save information after driver initialization or only during open execution?

On which OS does this issue occur?

[Linux]

What is the version of your OS?

Ubuntu 24.0

NuttX Version

master

Issue Architecture

[all]

Issue Area

[Drivers]

Verification

• I have verified before submitting the report.

[acassis]

@FelipeMdeO I think you are right, the modification seems suspicious! This is issue when someone adds a driver and don't add a testing application!

@xiaoxiang781216 could you please help and ask your team to review?

[acassis]

@xiaoxiang781216 is there some example using SPI Master <---> SPI Slave communication (maybe using RPMSG) ? It should be something useful to validate the driver and could help Felipe to reach his goal

[xiaoxiang781216]

@xiaoxiang781216 is there some example using SPI Master <---> SPI Slave communication (maybe using RPMSG) ? It should be something useful to validate the driver and could help Felipe to reach his goal

Yes, we made rpmsg over spi master/slave recently, but it make the significant change to rptun driver framework., so it need more time to prepare the patch.

For spi slave change, @Donny9 will take a look.

[Donny9]

@FelipeMdeO hello～
The purpose of this PR modification is to disable interrupts and the SPI slave controller when the application has not opened the SPI slave device, in order to conserve power consumption.

Before the device is opened by the user application, any data sent by the host is of no concern to the user application.
Even without this PR, if the host continuously sends data, the rxbuffer may overflow, resulting in data loss.
So, if the application does care about the data, it should open the device and use the poll interface to monitor the underlying data, thus preventing data loss.

Therefore, I recommend opening the device node to prevent data loss.
If you do not want to do so, you can, in your board code, initialize your SPI slave code while enabling interrupts by default. This will ensure that when the host sends data, it is received by the SPI slave lower half driver, and when you read it, the data is copied from the lower half to the top half.

[FelipeMdeO]

@Donny9 , Thank you very much for your explanation.

I understand the implementation now and I agree, it makes sense, initialize function configure spi peripheral, open enable and close disable it.

Donny, I cannot do it work yet, can help me to understand other point, please?
In the PR: #10782
We have the change below:

but:
priv->rx_length = MIN(buflen, sizeof(priv->rx_buffer));

I think that:
while (priv->rx_length == 0) always will be false. What did I miss?

[Donny9]

@Donny9 , Thank you very much for your explanation.

I understand the implementation now and I agree, it makes sense, initialize function configure spi peripheral, open enable and close disable it.

Donny, I cannot do it work yet, can help me to understand other point, please? In the PR: #10782 We have the change below:

but: priv->rx_length = MIN(buflen, sizeof(priv->rx_buffer));

I think that: while (priv->rx_length == 0) always will be false. What did I miss?

@FelipeMdeO Hello:
The spi_slave_read function is used by an application to read data from the top half. It first checks if there is any data available (by verifying if priv->rx_length is not equal to 0):

If there is data available, it directly copies the data from priv->rx_buffer to the user's buffer using memcpy.
However, if there is no data available, it needs to invoke SPIS_CTRLR_QPOLL to fetch data from the lower half.

The SPIS_CTRLR_QPOLL function will call SPIS_DEV_RECEIVE to push data from the lower half into the priv->rx_buffer of the top half and updates priv->rx_length accordingly. Once this is done, the while loop in spi_slave_read will exit , and memcpy data, finish read~

[acassis]

@Donny9 all drivers on NuttX needs some example application to prove it is working and to help to test. At this moment we don't know if the BUG is on ESP32-C6 or in the SPI Slave driver.

Could you please submit a simple/minimal apps/testing/spislave/ example just to receive data from other board (using spitool) and transmit back to the board?

[xiaoxiang781216]

@Donny9 all drivers on NuttX needs some example application to prove it is working and to help to test. At this moment we don't know if the BUG is on ESP32-C6 or in the SPI Slave driver.

Could you please submit a simple/minimal apps/testing/spislave/ example just to receive data from other board (using spitool) and transmit back to the board?

@acassis we will provide a new rpmsg transport on top of spi master and slave, after that you can test the basic communication through rpmsg ping command for both spi master/slave from nsh.

[acassis]

@xiaoxiang781216 sounds good thank you, but as you said this rpmsg will delay to come, so maybe a simple sample code is enough for let @FelipeMdeO to test it

[Donny9]

@Donny9 all drivers on NuttX needs some example application to prove it is working and to help to test. At this moment we don't know if the BUG is on ESP32-C6 or in the SPI Slave driver.

Could you please submit a simple/minimal apps/testing/spislave/ example just to receive data from other board (using spitool) and transmit back to the board?

okay, i will submit later.

[FelipeMdeO]

Hi @Donny9,

Sorry to insist, but I still haven't figured out how the read function works.

Look:

Let's consider that I'm trying to get 30 bytes, so priv->rx_length = 30;
while (priv->rx_length == 0) will be false, so we jump for:

now, read_bytes = 30;
But priv->rx_buffer isn't updated, because it is necessary call SPIS_CTRLR_QPOLL(priv->ctrlr); what does not happen.

You explaned:
"However, if there is no data available, it needs to invoke SPIS_CTRLR_QPOLL to fetch data from the lower half."
I understand priv->rx_length always will different of zero, maybe the mistake is because we are doing sizeof instead just get value:
priv->rx_length = MIN(buflen, **sizeof**(priv->rx_buffer));

What do you think?

[acassis]

@Donny9 I think @FelipeMdeO is right, there is a logic error here:

  priv->rx_length = MIN(buflen, sizeof(priv->rx_buffer));
  ret = nxmutex_lock(&priv->lock);
  if (ret < 0)
    {
      spierr("Failed to get exclusive access: %d\n", ret);
      return ret;
    }

  while (priv->rx_length == 0)

The MIN(buflen, sizeof(priv->rx_buffer)); never will return 0, unless the user ask to read 0 bytes, what makes no sense either.

Please review the code or submit a minimum example showing that it will work.

[Donny9]

@Donny9 I think @FelipeMdeO is right, there is a logic error here:

  priv->rx_length = MIN(buflen, sizeof(priv->rx_buffer));
  ret = nxmutex_lock(&priv->lock);
  if (ret < 0)
    {
      spierr("Failed to get exclusive access: %d\n", ret);
      return ret;
    }

  while (priv->rx_length == 0)

The MIN(buflen, sizeof(priv->rx_buffer)); never will return 0, unless the user ask to read 0 bytes, what makes no sense either.

Please review the code or submit a minimum example showing that it will work.

@acassis @FelipeMdeO You are right, sorry, We have reverted this PR（#8769) internally, but it hasn't been committed yet. It's true that it would cause the read function to stop working. Revert PR: #13337, @FelipeMdeO you can try with this PR.