Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

riscv/common/espressif: Fix spi slave driver #14642

Conversation

FelipeMdeO
Copy link
Contributor

@FelipeMdeO FelipeMdeO commented Nov 4, 2024

Summary

In the current version, the ESP32-C6 does not behave correctly when sending data while operating in SPI Slave mode. This commit addresses the issue by ensuring proper data transmission. Additionally, Espressif's SPI controller uses a single buffer functioning as both TX and RX, which differs from other MCU manufacturers such as ST and Silicon Labs, as discussed and illustrated in the images below. With this commit, the SPI Slave behavior is expected to be appropriate on Espressif's RISC-V devices.

Impact

This change affects the operation of SPI Slave mode on Espressif's ESP32-C6 devices. Users utilizing SPI Slave configurations will be able to send data consistently and as expected, aligning with the behavior of SPI controllers from other MCU manufacturers. This improvement enhances interoperability and reliability of SPI communications in projects relying on these devices. Furthermore, the unified TX/RX buffer simplifies maintenance and ensures compatibility with future firmware and hardware updates.

For more details about compatibility, please take a look on #13855
You will see that the Espressif SPI controller has only one buffer to TX and RX data, so is necessary driver clear buffer if it have no data to be sent.
Below you can see behavior from stm32f4 mcu:
image

And below you can see behavior from efm32gg12 mcu:
image
image

Testing

To verify the effectiveness of this change, the spislv configuration available for the esp32c6-devkitm and esp32c6-devkitc kits was used. The tests involved sending data via SPI and observing that the master only receives the data being sent by the slave. The following environments and tools were utilized during testing:

Build Host: [Specify the operating system, CPU, compiler, etc.]
Verification Target: Espressif's RISC-V devices, specifically the esp32c6-devkitm and esp32c6-devkitc kits.
Testing Procedure:
    Configure the mentioned kits in SPI Slave mode using the spislv configuration.
    Send a sequence of data via SPI from the master.
    Verify on the master that only the data sent by the slave is received correctly.
Results: After the commit, the data sent by the slave was correctly received by the master as expected, without inconsistencies or data loss.

Build and runtime logs before and after the change were analyzed to ensure that the SPI Slave behavior meets the expected outcomes.

Se results below:
image

Below I fixed only 2 bytes as answer:
image
Now only two bytes are send.

@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Size: S The size of the change in this PR is small labels Nov 4, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 4, 2024

[Experimental Bot, please feedback here]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

This PR summary mostly meets the NuttX requirements but is missing some crucial details. Here's a breakdown and suggestions for improvement:

Strengths:

  • Clear Problem Description: The summary clearly explains the issue with ESP32-C6 SPI Slave mode.
  • Solution Explanation: It describes how the commit addresses the problem.
  • Visual Aids: The images effectively illustrate the difference in SPI buffer behavior.
  • Testing Explanation: It outlines the testing procedure and provides visual confirmation of the fix.

Weaknesses:

  • Missing Issue References: While it mentions an issue number, it needs a proper link to the NuttX issue tracker.
  • Incomplete Impact Assessment: The impact section lacks specifics. It mentions user impact and improved interoperability, but doesn't detail how users might need to adapt (if at all) or what the compatibility implications are. The "anything else to consider" is also left blank.
  • Insufficient Testing Details: While test results are shown, critical details are missing from the "Testing" section:
    • Build Host Details: The OS, CPU architecture, and compiler used for building are not specified.
    • Before/After Logs: Actual log output is missing. Screenshots are helpful, but full logs are essential for thorough review. The screenshots are also difficult to interpret without context. Explain what is being shown.
    • Specific Test Cases: Describe the exact data sent and received.

Recommendations for Improvement:

  • Add Issue Link: Include a direct link to the related NuttX issue (e.g., https://github.com/apache/nuttx/issues/13855).

  • Elaborate on Impact:

    • User Impact: If there's no user impact, state "NO" explicitly. If there is an impact, explain what users need to do differently. For example, do they need to change configuration settings?
    • Build Impact: Specify "NO" or detail any changes to the build process.
    • Hardware Impact: Clearly state which architectures/boards/drivers are affected. You mention ESP32-C6, but be explicit.
    • Documentation Impact: If documentation needs updating, either state that an update is provided in the PR or explain what documentation needs to be changed.
    • Security Impact: Explicitly state "NO" if there's no security impact.
    • Compatibility Impact: While you link to an issue, summarize the key compatibility aspects in the PR itself. Explain backward/forward compatibility in detail.
    • Anything Else: If there's truly nothing else to consider, state "None."
  • Improve Testing Details:

    • Build Host: Provide complete details (OS, CPU, compiler version). Example: Build Host: Linux (Ubuntu 22.04), x86_64, GCC 11.2.0
    • Target: Be specific. Example: Target: esp32c6-devkitm (sim), esp32c6-devkitc (hardware)
    • Logs: Replace the screenshots with text-based logs showing the SPI communication before and after the change. Explain what each part of the log represents.
    • Test Data: Provide the exact data sequence sent by the master and the expected/received data from the slave. For example: "Master sent: 0x12 0x34 0x56. Slave responded (before): 0x00 0x00 0x00 0x00. Slave responded (after): 0x12 0x34."

By addressing these points, the PR will be much clearer, easier to review, and more likely to be accepted.

Fix defconfigs and documentation
@FelipeMdeO FelipeMdeO force-pushed the riscv-esp32c6-fix-spi-slave-tx-issue branch from a678368 to 9a670fc Compare November 4, 2024 17:45
@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Board: risc-v Size: M The size of the change in this PR is medium and removed Size: S The size of the change in this PR is small labels Nov 4, 2024
@FelipeMdeO
Copy link
Contributor Author

Please @eren-terzioglu and @tmedicci, take a look.

@tmedicci
Copy link
Contributor

tmedicci commented Nov 4, 2024

Hi @FelipeMdeO , thanks for submitting! I have some questions:

  1. Why the master device receives (in the first transmission) 09 0a 0b 0c? Is this expected?
  2. What would happen if you write more bytes in the slave's transmit buffer than the number of bytes to be received? For instance, suppose you write 01 02 03 04 05 06 and the master device sends 4 bytes. I'd expect that the answer would be 01 02 03 04 as the response and 05 06 to be transmitted in the next exchange. Is that true?

@FelipeMdeO
Copy link
Contributor Author

Hi @FelipeMdeO , thanks for submitting! I have some questions:

1. Why the master device receives (in the first transmission) `09 0a 0b 0c`? Is this expected?

2. What would happen if you write more bytes in the slave's transmit buffer than the number of bytes to be received? For instance, suppose you write `01 02 03 04 05 06` and the master device sends 4 bytes. I'd expect that the answer would be `01 02 03 04` as the response and `05 06` to be transmitted in the next exchange. Is that true?
  1. Hello @tmedicci , sorry, I generate images without "reload" master app, so screen have information from old test. Data in top of second image is related bottom of first image. Also, I change spi slave example to send always a know data, see PR examples:spislave_test: Improve example nuttx-apps#2819 ( I will fix PR issues as soon as possible ).
  2. I changed spislv example (I think will be better change this example to receive data to send as parameter ;] ) to always queue 8 bytes, look results :
    image

@FelipeMdeO
Copy link
Contributor Author

I updated the example to allow user choose data to send:
apache/nuttx-apps#2820

Copy link
Contributor

@cederom cederom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @FelipeMdeO :-)

Please remember that you can copy-paste the terminal output.

Use the markdown to paste code / runtime example like this :-)

Text is smaller than picture it works on text browsers etc.

@xiaoxiang781216 xiaoxiang781216 merged commit 0fad2ee into apache:master Nov 5, 2024
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Area: Documentation Improvements or additions to documentation Board: risc-v Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants