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

Feature/esp efuse support #15186

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

eren-terzioglu
Copy link
Contributor

@eren-terzioglu eren-terzioglu commented Dec 13, 2024

Summary

efuse support added to read and write efuses through application code
virtual efuse (efuse simulation) support to test efuse operation codes without damaging the hardware.

  • esp32[c3|c6|h2]: Add efuse simulation support
  • esp32[c3|c6|h2]: Add efuse support

Impact

ESP32-C3, ESP32-C6, ESP32-H2

Testing

Configrations used with CONFIG_ESPRESSIF_EFUSE option and additionaly CONFIG_ESPRESSIF_EFUSE_VIRTUAL option enabled:

esp32c3-generic:nsh
esp32c6-devkitc:nsh
esp32h2-devkit:nsh

Sample app used to fetch a sample efuse.

@eren-terzioglu eren-terzioglu changed the title Feature/esp efuse sim support Feature/esp efuse support Dec 13, 2024
@github-actions github-actions bot added Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture Arch: xtensa Issues related to the Xtensa architecture Board: risc-v Size: L The size of the change in this PR is large labels Dec 13, 2024
@eren-terzioglu eren-terzioglu marked this pull request as ready for review December 13, 2024 16:57
@nuttxpr
Copy link

nuttxpr commented Dec 13, 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.

Yes, this PR appears to meet the basic NuttX requirements, but it's missing some crucial details. Here's a breakdown of what's good and what needs improvement:

Strengths:

  • Clear Summary: The summary explains the "what" and "why" of the changes. The inclusion of related issues would strengthen this further.
  • Impact Section Mostly Complete: Addresses most impact areas, although the answers are very brief and need more detail.
  • Testing Information Provided: Includes target configurations and mentions a sample app.

Weaknesses:

  • Missing Issue References: The summary mentions adding efuse support. Are there corresponding issues in either the NuttX or NuttX-Apps repositories? If so, link them.
  • Impact Section Lacks Detail: While the impacted architectures are listed, the descriptions are insufficient. For example:
    • User Impact: How will the user interact with the new EFUSE features? New syscalls? Configuration options? This needs to be explicit.
    • Build Impact: Does enabling EFUSE support require any special build flags or dependencies?
    • Hardware Impact: This is just a list of architectures. Does this change touch any specific peripherals or drivers?
    • Documentation Impact: If documentation updates are required, has this PR included them, or is a separate PR planned? Be specific about which documentation needs updating.
    • Security Impact: EFuses often have security implications. This section requires a more thorough analysis. Even if there's no direct security impact, explicitly stating that and the reasoning behind it is important.
    • Compatibility Impact: Same as Security Impact. A more detailed analysis is needed.
  • Testing Logs Missing: The template clearly asks for "Testing logs before change" and "Testing logs after change." These are absent. Provide actual log output demonstrating the functionality and the fix/improvement. Just stating that a sample app was used isn't sufficient proof.
  • Build Host Information Missing: What system was used to build NuttX? This is critical for reproducibility.
  • "Anything else to consider?" Unanswered: This prompts for unusual situations or potential side effects. Even if the answer is "nothing," it should be stated.

Recommendation:

Revise the PR description to provide the missing details. Focus on expanding the Impact and Testing sections to fully address the requirements. A more complete PR description will greatly expedite the review process.

Copy link
Contributor

@acassis acassis left a comment

Choose a reason for hiding this comment

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

@eren-terzioglu please update boards Documentation/ to include efuse board profile, include usage example.

Please update also the Peripheral Support table.

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 Arch: xtensa Issues related to the Xtensa architecture Board: risc-v Size: L The size of the change in this PR is large
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants