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

dump more information when CONFIG_MM_DUMP_DETAILS_ON_FAILURE enabled, flush log before coredump #14094

Merged
merged 3 commits into from
Oct 12, 2024

Conversation

jasonbu
Copy link
Contributor

@jasonbu jasonbu commented Oct 11, 2024

Summary

dump more information when CONFIG_MM_DUMP_DETAILS_ON_FAILURE enabled,
flush log before coredump.

Impact

more log when do diagnostic in mass-production projects.

Testing

CI-test. cortex-m33 board.

@github-actions github-actions bot added Area: Memory Management Memory Management issues Area: OS Components OS Components issues Size: S The size of the change in this PR is small labels Oct 11, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 11, 2024

[Experimental Bot, please feedback here]

This Pull Request seems to mostly meet the NuttX PR requirements, but could use some improvements for clarity and completeness.

Strengths:

  • Summary: Clearly states the "what" of the change: More information will be dumped on memory manager failures if a config flag is enabled.
  • Testing: Specifies CI testing was done, along with the architecture and board used.

Areas for Improvement:

  • Summary:
    • Why: Explain the reason behind needing more detailed dumps. What kind of diagnostics does this aid in mass-production?
    • How: Be more specific about the "more information" being added. What data will be included in the dump that wasn't there before?
    • Flush Log: Briefly explain why flushing the log before a coredump is important.
    • Issues: Link any related NuttX issues if this PR addresses them.
  • Impact:
    • New/Changed Feature: While technically correct, be more explicit. State if this is a bug fix, improvement to an existing feature (dumping), or a completely new feature.
    • User Impact: Will users need to enable the CONFIG_MM_DUMP_DETAILS_ON_FAILURE flag to see the change? If so, mention it.
    • Build Impact: Unlikely, but if the added logging has any potential impact on build times (especially for resource-constrained targets), mention it.
    • Other Impacts: All marked as "NO" without explanation. While likely accurate, consider adding a brief "N/A" or a sentence like "No other anticipated impacts" for completeness.
  • Testing:
    • Build Host: Provide details about the CI environment (OS, compiler used for the host).
    • Logs: Instead of just stating "testing logs", either provide snippets of the relevant log sections or explain what differences were observed in the logs before and after the change.

In short, be as specific and detailed as possible while remaining concise. This helps reviewers understand the PR's value and potential ramifications quickly.

anjiahao1 and others added 3 commits October 11, 2024 21:49
When coredump to mtd, it maybe cost lots of time, do flush syslog can
make user access all log when coredump processing, should be better.

Signed-off-by: buxiasen <[email protected]>
@xiaoxiang781216 xiaoxiang781216 merged commit ca45ad6 into apache:master Oct 12, 2024
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Memory Management Memory Management issues Area: OS Components OS Components issues 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