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

Merged

Conversation

eren-terzioglu
Copy link
Contributor

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

Summary

Related to apache/nuttx-apps#2911 and apache/nuttx-apps#2925

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.

tools/Unix.mk Outdated Show resolved Hide resolved
@eren-terzioglu
Copy link
Contributor Author

Thanks @xiaoxiang781216, updated.

@github-actions github-actions bot added Size: XS The size of the change in this PR is very small and removed Size: S The size of the change in this PR is small labels Dec 30, 2024
tools/Unix.mk Outdated Show resolved Hide resolved
@github-actions github-actions bot added Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces. and removed Size: XS The size of the change in this PR is very small labels Dec 30, 2024
tools/Unix.mk Outdated Show resolved Hide resolved
Copy link
Contributor

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

This seems really nice and desired set of features! But it seems too quickly getting out of hand proposing too many changes at once.. lets slow down a bit please and get better feedback from the community so the best possible version lands into master :-)

The Christmas and New Year break is over so we should get more hands on deck now :-)

HAPPY NEW YEAR FOLKS :-)

@cederom
Copy link
Contributor

cederom commented Jan 2, 2025

I would suggest renaming debug_info target to sysinfo or dump_sysinfo or sysinfo_dump (or similar) because we operate on sysinfo (i.e. sysinfo.h / tools/parse_sysinfo.py / tools/host_sysinfo.py) not really anything debug related (in terms of On-Chip-Debug) right?

@cederom
Copy link
Contributor

cederom commented Jan 2, 2025

@eren-terzioglu this is really amazing and welcome job, big thank you for the contributions!! Let's invite the community to the discussion :-)

Especially may come handy in the Independent Distributed NuttX Build and Runtime Test Farm created by @lupyuen as an extra source of information for the Dashboard? And similar projects? How can we include it into a standard build and runtime process? :-)

But maybe not everyone would want to share information about their build environment to the public.. so maybe partial anonymization should be possible here?

Also we should remember not to include possibility to execute fetched content..

Also it seems the documentation part may need a review and update with new functionalities and examples? :-)

https://nuttx.apache.org/docs/latest/applications/system/nxdiag/index.html

Thank you :-)

@cederom cederom requested a review from patacongo January 3, 2025 02:27
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 include the summary in the commit log message

@eren-terzioglu
Copy link
Contributor Author

I would suggest renaming debug_info target to sysinfo or dump_sysinfo or sysinfo_dump (or similar) because we operate on sysinfo (i.e. sysinfo.h / tools/parse_sysinfo.py / tools/host_sysinfo.py) not really anything debug related (in terms of On-Chip-Debug) right?

If you don't count some information about chip (MAC address, CHIP id, ...) we are not doing something related on chip.

@tmedicci
Copy link
Contributor

tmedicci commented Jan 3, 2025

the problem is that it's not good to put the arch/chip specific code in a common script.

My suggestion is to decouple the host_sysinfo.py from chip-specific. Let's split Espressif-related functions to tools/espressif/chip_info.py (and similarly for other vendors) and, based on .config file, we can import it to host_sysino.py internally. It'd be like a plugin that imports chip-specific information. Instead of using the --espressif argument, we can use --enable-target-info to enable/disable such set of chip-specific information.

I would suggest renaming debug_info target to sysinfo

I agree!

@cederom
Copy link
Contributor

cederom commented Jan 5, 2025

the problem is that it's not good to put the arch/chip specific code in a common script.

My suggestion is to decouple the host_sysinfo.py from chip-specific. Let's split Espressif-related functions to tools/espressif/chip_info.py (and similarly for other vendors) and, based on .config file, we can import it to host_sysino.py internally. It'd be like a plugin that imports chip-specific information. Instead of using the --espressif argument, we can use --enable-target-info to enable/disable such set of chip-specific information.

I would suggest renaming debug_info target to sysinfo

I agree!

Sounds nice.. if it does not complicate things at this point or can be implemented step by step when other blocks are ready? :-)

I also like clear decoupling of "host" (where NuttX is built) and "target" (where NuttX is running) :-)

@patacongo
Copy link
Contributor

I would suggest renaming debug_info target to sysinfo or dump_sysinfo or sysinfo_dump (or similar) because we operate on sysinfo (i.e. sysinfo.h / tools/parse_sysinfo.py / tools/host_sysinfo.py) not really anything debug related (in terms of On-Chip-Debug) right?

sysinfo() would collide with a Linux API of that name: https://man7.org/linux/man-pages/man2/sysinfo.2.html If it is not compatible that this might lead to naming issues in the future.

@cederom
Copy link
Contributor

cederom commented Jan 5, 2025

@cederom: I would suggest renaming debug_info target to sysinfo or dump_sysinfo or sysinfo_dump (or similar) because we operate on sysinfo (i.e. sysinfo.h / tools/parse_sysinfo.py / tools/host_sysinfo.py) not really anything debug related (in terms of On-Chip-Debug) right?

@patacongo: sysinfo() would collide with a Linux API of that name: https://man7.org/linux/man-pages/man2/sysinfo.2.html If it is not compatible that this might lead to naming issues in the future.

Thank you Greg! Good catch :-)

Maybe something like this?

  • tools/nxdiag/host_info_dump.py.
  • tools/nxdiag/host_info_parse.py.
  • tools/nxdiag/target_info_dump.py.
  • tools/nxdiag/target_info_parse.py.

My main concern was not to use debug term for system information as we may use it for the real debug stuff (in terms of On-Chip-Debug) one day. Its common nowadays to misuse "debug" for "logging" etc. I know the system information will be used to debug a problem, but debug has precise specific meaning (i.e. control CPU registers and program execution, control memory, access bus peripherals, etc). Also "system information" is a bit misleading because it does not clearly state if it is about host or target information :-P

@eren-terzioglu eren-terzioglu force-pushed the feature/add_debug_info branch from f2a2395 to c6db817 Compare January 6, 2025 09:05
@github-actions github-actions bot added Area: Documentation Improvements or additions to documentation Arch: risc-v Issues related to the RISC-V (32-bit or 64-bit) architecture labels Jan 6, 2025
Copy link
Contributor

@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.

Perfect! Thank you @eren-terzioglu !! :-)

tools/Unix.mk Outdated Show resolved Hide resolved
@eren-terzioglu eren-terzioglu force-pushed the feature/add_debug_info branch from c6db817 to 55a13bc Compare January 6, 2025 15:09
Nxdiag examples scripts  modified to print system information
without building and flashing nxdiag application to get report
about system without reflash or reconfigure.
@xiaoxiang781216 xiaoxiang781216 merged commit 6eabe35 into apache:master Jan 6, 2025
28 checks passed
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 Area: Build system Area: Documentation Improvements or additions to documentation Area: Tooling Size: XL The size of the change in this PR is very large. Consider breaking down the PR into smaller pieces.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants