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

boards/arm/rp2040/common: Add weak_function to SPI common logic #15894

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

keever50
Copy link

@keever50 keever50 commented Feb 22, 2025

Summary

Board logic change.
This PR adds weak_function attributes to the RP2040 common SPI board logic.
This allows board developers to override and extend the SPI board logic.

Impact

This allows board developers to add custom SPI logic such as adding additional chip select pins.
Adding new SPI devices such as displays or custom SPI devices like external boards is now possible.

External custom boards will have the biggest impact, as these are typically not pushed.

Testing

This has been tested by building the code on linux with raspberrypi-pico:nsh config.
I also tested it on a custom board and it works.
However, my knowledge is not very big about compiler compatibility when weak_function is used.

Please let me know if there are issues.

@github-actions github-actions bot added Board: arm Size: S The size of the change in this PR is small labels Feb 22, 2025
@keever50 keever50 force-pushed the rp2040_spi_weak_functions branch from 420cfb9 to 93e0db1 Compare February 22, 2025 23:03
@nuttxpr
Copy link

nuttxpr commented Feb 22, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides a good starting point, it lacks crucial details.

Missing/Insufficient Information:

  • Summary: While it mentions the "what," it's missing a strong "why." Why is this change necessary? What specific problem does it solve beyond a general desire for extensibility? A related NuttX issue would greatly strengthen this section.
  • Impact: The impact section is too vague.
    • User Impact: While it mentions custom boards, it doesn't specify how users would adapt. Would they need to change configurations? Write new drivers? Clearer examples are needed.
    • Build Impact: Does adding weak_function change the build process in any way? Even if the answer is "NO," explicitly state it.
    • Hardware Impact: Specify the affected architecture (ARM) and board (Raspberry Pi Pico, and any others). "Custom board" is not specific enough. List the specific hardware changes if any.
    • Documentation Impact: Does this require documentation updates? If so, have they been included in the PR?
    • Security Impact: Explicitly state "NO" if there's no security impact.
    • Compatibility Impact: Explicitly state "NO" or explain potential backward compatibility issues. Consider the impact on existing boards using the RP2040 SPI.
  • Testing:
    • Build Host(s): Be specific about the Linux distribution used, the compiler version (e.g., GCC 12.2.0), and any relevant build tools.
    • Target(s): Be precise about the target configurations tested. "raspberrypi-pico:nsh config" isn't precise enough. Specify the full configuration name (e.g., configs/raspberrypi-pico/nsh). For the custom board, provide details about its configuration.
    • Logs: The "Testing logs" sections are empty. Include relevant log output demonstrating the functionality before and after the change. Show how the new weak functions are being overridden and what the results are. Demonstrate that existing functionality is not broken.

Recommendations:

  1. Clarify the "why": Explain the specific problem this solves. Provide a concrete example of how the lack of this feature hindered development before. Link a related NuttX issue if one exists.
  2. Detailed Impact Assessment: Be explicit about all impact categories, even if the answer is "NO." Provide specific details and examples where necessary.
  3. Thorough Testing Information: Provide complete details about the build host and target environments. Include relevant log output demonstrating the change's functionality and the absence of regressions. Test and document the behavior when the weak functions are not overridden to ensure backward compatibility.
  4. Compiler Compatibility: Research and address the compiler compatibility concerns regarding weak_function. Document any limitations or required compiler versions.

By addressing these points, the PR will better meet the NuttX requirements and be more likely to be accepted.

@lupyuen
Copy link
Member

lupyuen commented Feb 24, 2025

Please fill in the Commit Description (copy from the PR Summary). And remember to sign-off with git commit -s. Thanks! :-)

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

@keever50 keever50 force-pushed the rp2040_spi_weak_functions branch from 93e0db1 to c094891 Compare February 24, 2025 09:37
@keever50
Copy link
Author

Okay! Signed and added a message

@keever50
Copy link
Author

I realized i copied the entire PR message and should have been only summery i believe. Hope this isnt a big issue.

@lupyuen
Copy link
Member

lupyuen commented Feb 24, 2025

Sorry could you run git commit -s to sign off? Thanks :-)

@keever50 keever50 force-pushed the rp2040_spi_weak_functions branch 2 times, most recently from 7fa681b to 70748c1 Compare February 24, 2025 09:43
@keever50
Copy link
Author

Signed

@lupyuen
Copy link
Member

lupyuen commented Feb 24, 2025

Hmmm I don't see the "Signed-off-by". Could you do git push? Thanks!

@keever50
Copy link
Author

@lupyuen By just running commit -s i get "nothing to commit, working tree clean". I ran git commit --amend --no-edit -S which should in theory add a sign?

Summary
Board logic change.
This PR adds weak_function attributes to the RP2040 common SPI board logic.
This allows board developers to override and extend the SPI board logic.

Signed-off-by: Kevin Witteveen (MartiniMarter) <[email protected]>
@keever50
Copy link
Author

ahhh i see signed off now! However now i have issues pushing it because it does not let me enter my password anymore. Ill get back to this soon

@keever50 keever50 force-pushed the rp2040_spi_weak_functions branch from 70748c1 to b44d759 Compare February 24, 2025 09:54
@keever50
Copy link
Author

oh okay do the same thing twice and it works.

@lupyuen
Copy link
Member

lupyuen commented Feb 24, 2025

Looks great, thanks! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Board: arm Size: S The size of the change in this PR is small
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants