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

tools/Unix.mk: Add debug_info target to print nxdiag output #15304

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

Conversation

eren-terzioglu
Copy link
Contributor

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

Summary

Related to apache/nuttx-apps#2911

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/Unix.mk: Add debug_info target to print nxdiag output

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 mostly meet the NuttX requirements, but some sections need more detail.

Here's a breakdown of what's good and what needs improvement:

Good:

  • Clear Summary: The summary explains the "why," "what," and "how" of the change. The link to the related NuttX Apps PR is also helpful.
  • Impact - Common Layer Change: This identifies the general area affected.
  • Testing - Specific Configuration: Mentioning esp32c6-devkitc:nsh and make debug_info gives some context.

Needs Improvement:

  • Impact: The Impact section needs significantly more detail. While "Common layer change" is a start, explicitly answer all the questions with "YES" or "NO" and provide descriptions where necessary. Even if the answer is "NO," state it explicitly. For example:

    • Is new feature added? YES (debug_info target)
    • Is existing feature changed? NO
    • Impact on user... YES (Users can now run make debug_info to get system information.)
    • Impact on build... YES (New debug_info target added to the build system.)
    • Impact on hardware... NO
    • Impact on documentation... YES (Documentation should be updated to explain the new debug_info target and its usage.)
    • Impact on security... NO (Assuming the sysinfo.h file doesn't contain sensitive data unintentionally.)
    • Impact on compatibility... NO (Assuming no breaking changes.)
  • Testing: The Testing section is the weakest. "Testing logs before change" and "Testing logs after change" are required. Show the actual output of make debug_info before the change (which would likely be an error or nothing) and after the change (showing the parsed sysinfo.h information). This demonstrates that the new functionality works as expected. Also include details about your build host environment.

Example of Improved Testing Section:

I confirm that changes are verified on local setup and works as intended:
* Build Host(s): macOS Ventura 13.5, Intel Core i7, GCC 12.2
* Target(s): esp32c6-devkitc:nsh (simulated)

Testing logs before change:

make debug_info
make: *** No rule to make target 'debug_info'. Stop.


Testing logs after change:

make debug_info
Parsing sysinfo.h...
System Information:
NuttX Version: v11.0.0
Build Date: 2024-07-24 10:00:00
ARCH: sim
BOARD: esp32c6-devkitc
CONFIG: nsh
... other sysinfo data ...


By providing this level of detail, reviewers can more easily understand the changes and verify their correctness, leading to a smoother review process and a higher quality contribution.

# debug_info: Parse nxdiag example output file (sysinfo.h) and print

debug_info: checkpython3
@if [[ ! -f "$(CONFIG_APPS_DIR)/system/nxdiag/sysinfo.h" ]]; then \
Copy link
Contributor

Choose a reason for hiding this comment

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

but why generate apps specific info from nuttx build script? it's better to generate this info inside system/nxdiag's scripts.

Copy link
Contributor Author

@eren-terzioglu eren-terzioglu Dec 22, 2024

Choose a reason for hiding this comment

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

It is not only apps specific info, it gives info from build system.I used it because it was already implemented for fetching system information (e.g Compilation flags, Configuration(defconfig), Toolchain info, ...).
Also I tried to implement target in apps/system/nxdiag scripts but it didn't work.

Copy link
Contributor

Choose a reason for hiding this comment

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

what's problem do you hit if the action move into nxdiag?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Build system Area: Tooling 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