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

examples:spislave_test: Improve example #2820

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

FelipeMdeO
Copy link
Contributor

Improve example, now it can receive data from user

Summary

Spislv example was improved. Now it can receive data from user. It follow spitool features standard:
This changes was done in parallel at apache/nuttx#14642. Both are tested together .

Impact

No Impact

Testing

This changes was done in parallel at apache/nuttx#14642. Both are tested together .

@nuttxpr
Copy link

nuttxpr commented Nov 4, 2024

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a summary and testing information, it lacks crucial details. Here's a breakdown of the missing information:

  • Summary: While it explains the "what," it lacks sufficient detail on the "how." It mentions following spitool features but doesn't explain which features or how they were implemented. The reference to a parallel PR is helpful, but the description should stand on its own. What specific files were changed in the spislv example?

  • Impact: Simply stating "No Impact" is insufficient. Each impact category should be addressed individually, even if the answer is "NO." For example:

    • Impact on user: NO
    • Impact on build: NO
    • ...and so on for all categories. This demonstrates that each aspect has been considered.
  • Testing: While it mentions testing with a parallel PR, the logs are missing. "Both are tested together" isn't enough. Provide specific logs showing the behavior before and after the change in this PR. Include the specific build host and target details. What OS, architecture, board, and configuration were used for testing?

In short: The PR needs more details in the Summary, explicitly address each Impact category, and include actual testing logs with specific host/target information. Relying on a parallel PR for context isn't sufficient for a thorough review.

@FelipeMdeO FelipeMdeO force-pushed the implove-spi-slave-example branch from 29361b4 to 7531949 Compare November 4, 2024 22:41
@github-actions github-actions bot added Size: L and removed Size: M labels Nov 4, 2024
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.

@acassis
Copy link
Contributor

acassis commented Nov 11, 2024

@FelipeMdeO the coding style issues are just Long Lines, you can use the tools/checkpatch.sh -f path/to/file.c/h to show the lines with issues

@cederom
Copy link

cederom commented Jan 30, 2025

ping :-)

@FelipeMdeO FelipeMdeO force-pushed the implove-spi-slave-example branch from 7531949 to b48fd82 Compare February 16, 2025 23:18
@FelipeMdeO FelipeMdeO requested a review from cederom February 16, 2025 23:20
@FelipeMdeO
Copy link
Contributor Author

Hello @cederom, sorry for delay. Code style issues solved.

@cederom
Copy link

cederom commented Feb 16, 2025

Thanks @FelipeMdeO for the updates :-) Please update git commit:

  • subject: examples/spislave_test: Add user data receive. or something like that :-)
  • Add git commit -s signature :-)

@FelipeMdeO FelipeMdeO force-pushed the implove-spi-slave-example branch 2 times, most recently from 423b884 to f899f67 Compare February 16, 2025 23:46
@FelipeMdeO
Copy link
Contributor Author

Hello @acassis , done.

Improve example, now it can receive commands / data from user;
Fixed indentation issues.

Signed-off-by: Felipe Moura <[email protected]>
@FelipeMdeO FelipeMdeO force-pushed the implove-spi-slave-example branch from f899f67 to 37ad23e Compare February 16, 2025 23:58
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 :-)

@cederom cederom merged commit 62894b9 into apache:master Feb 17, 2025
37 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