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

Coredump restore application isolate with board_crashdump #2871

Merged
merged 4 commits into from
Nov 29, 2024

Conversation

jasonbu
Copy link
Contributor

@jasonbu jasonbu commented Nov 29, 2024

Summary

pervious coredump is depend on board_crashdump enabled,
but for some use case, we need to not open crashdump and open coredump restore.
for example AMP case.

also we already finished the blkout stream seek support, should make coredump infomation seek only depend on seek API.

should work with
apache/nuttx#14997
if want using coredump to blk/mtd -> reboot -> restore to filesystem -> adb/ymodem/.. this kind of workflow.

Impact

the coredump infomation position has changed, we now able to put more data in coredump section.
we now able to close board_crash dump and open crash dump restore.

Testing

CI test, local arm-v8m board.

@nuttxpr
Copy link

nuttxpr commented Nov 29, 2024

[Experimental Bot, please feedback here]

This PR does not fully meet the NuttX requirements as documented. While it provides some information, it lacks crucial details and formatting. Here's a breakdown:

Missing/Insufficient Information:

  • Summary: While the rationale is explained, the how is missing. What code was changed and how does it decouple coredump from board_crashdump? How does it leverage the blkout seek support? Be specific.
  • Impact:
    • User Impact: While it mentions a change in the coredump information position, it doesn't explain what this means for the user. Will existing tools need to be updated? How will users access the extra data?
    • Build Impact: Not addressed. Did adding this functionality change any build flags, dependencies, or configurations?
    • Hardware Impact: Not specifically addressed. While an ARM-v8m board is mentioned in testing, does this change affect all architectures, or only specific ones? Are there any hardware limitations?
    • Documentation Impact: Not addressed. Does this require updates to the NuttX documentation? If so, were these updates provided in the PR?
    • Security Impact: Not addressed. Changing how core dumps are handled could have security implications. This needs to be considered and explicitly stated (even if the impact is "NONE").
    • Compatibility Impact: Not addressed. Does this break any existing functionality or tools that rely on the old coredump behavior?
  • Testing:
    • Build Host(s): Insufficient. OS and compiler versions are missing. Example: "Linux, Ubuntu 20.04, GCC 9.4.0"
    • Target(s): Insufficient. Be more specific than "arm-v8m board." Provide the exact board name and configuration. Example: "STM32F429I-DISC1, default configuration"
    • Logs: Completely missing. "CI test" is not sufficient. Provide actual log output demonstrating the functionality before and after the change, specifically showing how the core dump is handled in both scenarios (with and without board_crashdump).

How to Improve the PR:

  1. Elaborate on the "How" in the Summary: Explain the code changes. Did you introduce a new API? Modify an existing one? Provide a brief, technical overview of the implementation.
  2. Address all Impact points: Even if the answer is "NO," explicitly state it and briefly explain why. If "YES," provide detailed explanations.
  3. Provide Specific Testing Information: Include OS, compiler versions for the build host. Specify the exact target board and configuration.
  4. Include Actual Test Logs: Show the output before and after the change, focusing on the core dump behavior. This is crucial for reviewers to verify the functionality.
  5. Consider Documentation: If changes are needed, include them in the PR or create a separate issue/PR for documentation updates.

By addressing these points, the PR will be much clearer, easier to review, and more likely to be accepted.

@anchao anchao merged commit 22d5d1d into apache:master Nov 29, 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