Skip to content

add lin status changes the format of the reported frame and add the …#14222

Closed
wangxiaoxin1 wants to merge 5 commits intoapache:masterfrom
wangxiaoxin1:master
Closed

add lin status changes the format of the reported frame and add the …#14222
wangxiaoxin1 wants to merge 5 commits intoapache:masterfrom
wangxiaoxin1:master

Conversation

@wangxiaoxin1
Copy link
Contributor

…macros that lin needs

Signed-off-by: wangxiaoxin wangxiaoxin@xiaomi.com
(cherry picked from commit a58d19e)

Note: Please adhere to Contributing Guidelines.

Summary

Add status information of status reporting frame

Impact

only for lin module

Testing

Cortex-M7 test pass

…acros that lin needs

Signed-off-by: wangxiaoxin <wangxiaoxin@xiaomi.com>
(cherry picked from commit a58d19e)
@github-actions github-actions bot added the Size: S The size of the change in this PR is small label Oct 14, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 14, 2024

[Experimental Bot, please feedback here]

The PR as presented does not meet the NuttX requirements. Here's why and what's missing:

  • Insufficient Summary:
    • Vague description: "Add status information of status reporting frame" is too general. What specific status information is added? What is a "status reporting frame" in this context?
    • Missing details: Which functional part of the code is changed? How exactly does the change work?
  • Incomplete Impact Assessment:
    • The impact section only mentions "only for lin module."
    • It needs to address all impact points with specifics, even if the answer is NO. For example, explicitly stating "No impact on build process" is better than leaving it to interpretation.
  • Absent Testing Details:
    • No build host information: Which OS, CPU architecture, and compiler version were used for testing?
    • Vague target information: "Cortex-M7 test pass" is not specific enough. Which board and configuration were used?
    • Missing logs: The PR template explicitly requests "Testing logs before change" and "Testing logs after change." These logs provide concrete evidence of the issue and the fix.

To improve the PR:

  1. Expand the Summary:
    • Clearly state the purpose of the change.
    • Identify the specific files and functions modified.
    • Explain the technical details of the implementation.
  2. Complete the Impact Assessment:
    • Address all impact points listed in the template, even if the impact is "NO."
    • Provide specific details and justification for each impact point.
  3. Provide Thorough Testing Information:
    • List the build host environment: OS, CPU, compiler version.
    • Specify the target(s): Architecture, board, configuration.
    • Include relevant testing logs from before and after the change.

By providing comprehensive information, you make it easier for maintainers to understand, review, and merge your PR, ultimately benefiting the NuttX project as a whole.

@xiaoxiang781216
Copy link
Contributor

@wangxiaoxin1 please remove the merge patch and the intermediate change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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