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

Add to nuttx apps a simple app to test spi slave driver. #2765

Merged

Conversation

FelipeMdeO
Copy link
Contributor

Summary

This app can be used to test spi slave driver. Using this app will be more fast to users test and verify spi slave driver.
For instance whats happened here: apache/nuttx#13855 (comment)

Impact

No impact, new feature.

Testing

This app was used to test lower half changes done in this PR: apache/nuttx#14420

@nuttxpr
Copy link

nuttxpr commented Oct 22, 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.

The PR summary is missing some key details. While it mentions testing the SPI slave driver, it doesn't explain how the app achieves this. Here's a breakdown of what's missing and how to improve the PR:

Summary:

  • Missing:
    • Functional part: Which part of the codebase is this app being added to? Is it a new directory under apps/, or is it extending an existing application?
    • Exact functionality: Be specific. Does the app send pre-defined SPI messages? Does it allow the user to input custom messages? Does it analyze the responses from the slave device?
  • Improvement:
    "This PR adds a new application under apps/testing/tools/spislave_test (or similar). The application provides a command-line interface for sending and receiving SPI data, allowing developers to test various aspects of the SPI slave driver, such as data transfer, interrupt handling, and error conditions."

Impact:

  • Inaccurate: While the app itself might be a new feature, it's likely interacting with existing code (the SPI driver). This interaction could have unforeseen impacts.
  • Missing:
    • Hardware: If the app requires specific hardware configurations (e.g., SPI pins), this needs to be documented.
    • Documentation: A new application will require documentation explaining how to use it.
  • Improvement:
    • "This PR adds a new application, so it has no direct impact on existing functionality. However, it relies on the SPI driver and might reveal hidden issues. This PR includes updated documentation for the application in the apps/testing/README.md file."

Testing:

  • Insufficient: Simply stating the app was used in another PR is not enough. The testing section should stand on its own.
  • Missing:
    • Build Hosts: List the operating systems and compilers used to build the application.
    • Target(s): Be specific about the target hardware and configurations used for testing (e.g., "STM32F4 Discovery board with the following SPI configuration...").
    • Logs: Provide snippets of the testing logs showing the app working as expected. Describe the test cases that were executed.
  • Improvement:
    Provide concrete examples of test commands used with the app and the expected output. For instance:
    "The following command was used to send a 16-byte message to the slave device: spislave_test -s /dev/spi0 -m 0x12 0x34 ... 0xab 0xcd. The slave device responded with 0xef 0x01 ..., confirming correct data transfer."

In conclusion, the PR does not fully meet the NuttX requirements. It needs significant improvements in clarity, completeness, and especially the testing section to be considered for merging.

Copy link
Contributor

@jerpelea jerpelea left a comment

Choose a reason for hiding this comment

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

please add a commit message that describes the app

@raiden00pl
Copy link
Member

please add this new example to documentation: https://github.com/apache/nuttx/tree/master/Documentation/applications/examples

so that it appears on the list here: https://nuttx.apache.org/docs/latest/applications/examples/index.html

Copy link

@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 also update the documentation as requested by @raiden00pl and add commit description as requested by @jerpelea :-)

This application continuously reads the file system of the spislv. Each received message will be written to the user in hexadecimal form, and the same received data will be sent back. In this way, the user can test if their spislv driver and hardware setup are working properly before proceeding further.

On a master device, using the SPI tool, when sending the message: spi exch -x 4 deadbeef

The slave device will output:
Slave: 4 Bytes read
 Value in hex form from /dev/spislv2: de ad be ef
Slave: Writing value back to /dev/spislv2
@FelipeMdeO
Copy link
Contributor Author

Hello @cederom, commit msg done and PR with documentation opened: apache/nuttx#14457

@FelipeMdeO FelipeMdeO requested a review from cederom October 22, 2024 13:26
Copy link

@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.

Perfect! Thank you @FelipeMdeO :-)

@cederom cederom merged commit f517b66 into apache:master Oct 22, 2024
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants