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

add lin error flags to lin.h #14125

Merged
merged 1 commit into from
Oct 12, 2024
Merged

Conversation

wangxiaoxin1
Copy link
Contributor

@wangxiaoxin1 wangxiaoxin1 commented Oct 11, 2024

Summary

To add specific error types about the lin module, first determine the error frame flag, classify the error in data[0], and indicate the specific error in data[1~4]. When there is a problem with the lin module communication, it is convenient to locate the problem in time.

Impact

Changes will change the format of lin error frames

Testing

It has been verified in multiple development versions

Signed-off-by: wangxiaoxin <[email protected]>
@github-actions github-actions bot added the Size: S The size of the change in this PR is small label Oct 11, 2024
@nuttxpr
Copy link

nuttxpr commented Oct 11, 2024

[Experimental Bot, please feedback here]

The PR summary is reasonably informative, but let's break down whether it meets all the NuttX requirements and where it could improve:

Summary:

  • ✅ Why? Clearly states the goal is better error reporting in the LIN module.
  • ✅ What? Specifies that the error frame format is being modified.
  • ✅ How? Provides a high-level overview of the new format using data[0], data[1~4].
  • ❌ Issue References: Missing! These are crucial for linking to bug reports or feature requests.
  • ➕ Could be more specific:
    • What exactly are the new error types? List a few examples.
    • What was the previous error handling like? This helps reviewers understand the improvement.

Impact:

  • ✅ New Feature? Implied, but stating "This PR adds..." is clearer.
  • ❌ User Impact? While it changes internal error frames, does this affect how user applications handle LIN errors? Be explicit.
  • ❌ Build Impact? Unlikely, but stating "NO" explicitly is better.
  • ✅ Hardware Impact: "NO" is sufficient if true.
  • ❓ Documentation Impact: Does this new error format need to be documented somewhere?
  • ✅ Security Impact: "NO" is fine if no vulnerabilities are introduced.
  • ❓ Compatibility Impact: Does this break existing applications relying on the old error format?
  • ➕ Anything else? This section is for unusual considerations not covered elsewhere.

Testing:

  • ❌ Build Hosts: Provide specifics: OS, compiler versions used for testing.
  • ❌ Targets: Be very specific: "Multiple development versions" is too vague. List the exact architectures and boards.
  • ❌ Logs: Absolutely require before/after logs demonstrating the change in error reporting.

Verdict:

The PR shows promise but does NOT fully meet NuttX requirements. The missing issue references, vague testing, and incomplete impact assessment need to be addressed before it's ready for review.

@xiaoxiang781216 xiaoxiang781216 merged commit 1b8c17f into apache:master Oct 12, 2024
35 of 37 checks passed
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.

3 participants