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/add debug info #2911

Merged
merged 2 commits into from
Dec 21, 2024

Conversation

eren-terzioglu
Copy link
Contributor

Summary

Related to apache/nuttx#15304

Add debug_info target support to parse and print sysinfo.h file which is output file of nxdiag application without flashing it to the device or even enabling SYSTEM_NXDIAG option. We can use this feature to have more information about user system environment when we are investigating issue. Users only need to run make debug_info on their problematic code.

  • tools: Add debug_info target to diagnostic system

Impact

Common layer change

Testing

esp32c6-devkitc:nsh config selected and then run make debug_info command

@nuttxpr
Copy link

nuttxpr commented Dec 20, 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 is missing some crucial information. Here's a breakdown of what's good and what needs improvement:

Strengths:

  • Clear Summary: The summary provides a good overview of the change, its purpose, and how it works. The link to the related PR is also helpful.
  • Impact Section Started: Addressing the impact is started, but needs significant expansion.
  • Testing Information Started: Mentioning the target and command used is a good start.

Weaknesses:

  • Incomplete Impact Assessment: The impact section is far too brief. Every "NO" should be justified. Every "YES" needs a detailed explanation. For example:
    • Impact on user: Even if the user doesn't need to adapt, can they use this new feature? How? Document it.
    • Impact on build: Does adding this target affect build times? Add dependencies? Require new tools?
    • Impact on hardware: While it might not change hardware behavior, does it increase code size? Does it require more RAM during the build process?
    • Impact on documentation: If this is a user-facing feature, it absolutely needs documentation. State where the documentation will be added/updated.
    • Security, Compatibility: Explicitly stating "NO" with a brief justification is better than leaving it blank. E.g., "Impact on security: NO. This change only adds a diagnostic tool and does not affect the security of the system itself."
  • Insufficient Testing Information: "esp32c6-devkitc:nsh" and make debug_info is a start, but insufficient. Provide:
    • Build Host Details: OS, CPU architecture, compiler version.
    • Example sysinfo.h file: Include a before and after sysinfo.h (or snippets) to demonstrate the tool's output. This proves the tool functions correctly.
    • make debug_info output: Show the actual output of the command. Does it print anything to the console? Where is the parsed information displayed?
    • "Testing logs before change": What was the previous behavior? What problem did this PR solve? Show how the lack of this tool made debugging difficult.

In short: The PR description is a good starting point, but needs significantly more detail in the Impact and Testing sections to be considered complete and ready for review. Providing concrete examples and justifications for each point will greatly increase the likelihood of quick and positive feedback.

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 @eren-terzioglu :-)

@xiaoxiang781216 xiaoxiang781216 merged commit 82ee1ff into apache:master Dec 21, 2024
25 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.

4 participants