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

arm/qemuv7a: Modify link script #14352

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

arm/qemuv7a: Modify link script #14352

wants to merge 1 commit into from

Conversation

W-M-R
Copy link
Contributor

@W-M-R W-M-R commented Oct 16, 2024

Summary

  1. Modify to map to the specified memory, CONFIG_FLASH_START and CONFIG_RAM_START
  2. Add explanation README.TXT

Impact

All configurations under qemev7a have been specified with CONFIG_FLASH_START CONFIG_FLASH_SIZE, CONFIG_RAM-START, and CONFIG_RAM_SIZE

Testing

No

1. Designate specialized VNet FLASH-START and VNet RAM-START
2. Add explanation README.TXT

Signed-off-by: wangming9 <[email protected]>
@github-actions github-actions bot added Board: arm Size: M The size of the change in this PR is medium labels Oct 16, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 16, 2024

[Experimental Bot, please feedback here]

No, this PR does not meet the NuttX requirements.

Missing Information:

  • Summary:
    • Lacks a clear explanation of why the change is necessary. Is it a bug fix, performance improvement, or new feature?
    • Doesn't specify what functional part of the code is being changed. Mention specific files or modules.
    • How does the mapping to CONFIG_FLASH_START and CONFIG_RAM_START actually work? What code is modified?
    • Are there related NuttX issues?
  • Impact:
    • While it states all configurations are impacted, it's unclear what the user-facing consequences are. Will applications behave differently? Are there potential compatibility issues?
    • Will the build process change for any boards? Provide specifics if yes.
  • Testing:
    • Unacceptable: "No" is not sufficient. You must provide testing evidence.
    • List the specific host operating systems, compilers, target architectures, and boards used for testing.
    • Include relevant log snippets demonstrating the problem before the change and the correct behavior after the change.

Recommendations:

  1. Expand the Summary: Clearly articulate the problem or need this PR addresses. Provide more context on the code being modified and the mechanics of the change.
  2. Detail Impact: Go beyond stating configurations are affected. Explain the consequences for users, build processes, and potential compatibility concerns.
  3. Provide Thorough Testing Information: Testing is crucial! List all host and target environments, and include log snippets showing before/after behavior.

Without these improvements, it's difficult for maintainers to assess the PR's validity and safety.

@GUIDINGLI
Copy link
Contributor

Hold for fix the knsh error

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.

Please don't include README.txt into boards/. All README.txt were moved and converted to ReStructuredText into Documentation/ long time ago!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Board: arm Size: M The size of the change in this PR is medium
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants