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

{bp-16085} arch/esp32s3: The MISO, MOSI and C/S are optional #16115

Merged
merged 1 commit into from
Apr 2, 2025

Conversation

jerpelea
Copy link
Contributor

@jerpelea jerpelea commented Apr 2, 2025

Summary

The MISO or MOSI pin is optional.
The C/S pin is unnecessary when ESP32S3_SPI_UDCS is enabled.

Impact

RELEASE

Testing

CI

The MISO or MOSI pin is optional.
The C/S pin is unnecessary when `ESP32S3_SPI_UDCS` is enabled.

Signed-off-by: wangjianyu3 <[email protected]>
@github-actions github-actions bot added Arch: xtensa Issues related to the Xtensa architecture Size: M The size of the change in this PR is medium labels Apr 2, 2025
@nuttxpr
Copy link

nuttxpr commented Apr 2, 2025

[Experimental Bot, please feedback here]

No, this PR does not fully meet the NuttX requirements. While it provides some information, it lacks crucial details.

Here's why:

  • Insufficient Summary: "The MISO or MOSI pin is optional. The C/S pin is unnecessary when ESP32S3_SPI_UDCS is enabled." This only describes what changed, not why. What problem does this solve? Is this a bug fix, a new feature, or an optimization? Which driver is affected? How exactly does making these pins optional and unnecessary work within the driver code? Links to related issues are missing.

  • Incomplete Impact Assessment: "RELEASE" is not a valid answer. Each impact category (user, build, hardware, documentation, security, compatibility) requires a "YES" or "NO" answer, followed by a description if the answer is "YES." For example, even if the answer to "Impact on user" is "NO," the PR submitter should explicitly state "NO." Marking "RELEASE" suggests this change affects a release, but doesn't explain how.

  • Inadequate Testing Information: "CI" is insufficient. While Continuous Integration testing is valuable, the PR should still include specific details about local testing. The template requests information about the build host and target(s), along with "before" and "after" testing logs. Simply stating "CI" doesn't provide enough evidence that the changes were thoroughly tested. It leaves reviewers guessing about the testing environment and the results.

The PR author needs to provide more specific information for each section to meet the NuttX requirements.

@lupyuen lupyuen merged commit 4e8c2ea into apache:releases/12.9 Apr 2, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: xtensa Issues related to the Xtensa architecture 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