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

SAMA5D2. Fix DEBUGASSERT triggered in sam_updhs.c #14128

Merged

Conversation

TimJTi
Copy link
Contributor

@TimJTi TimJTi commented Oct 11, 2024

Summary

When trying CDC/NCM I found that a debug assert was triggered. Initially I commented out the offending DEBUGASSERT and found the code worked just fine.

I don't know why the assert was placed there - perhaps with other uses of this driver there would always be data in the receive queue - so I moved the assignment to the places that actually needed and used the variable.

I have checked that not only CDC/NCM (now) works but also that RNDIS still behaves as before, so it seems like a "safe" fix.

Impact

None expected, but maybe reviewers with USB and DMA knowledge can comment.

Testing

Custom board with SAMA5D27C-D1G, using either CDC/NCM or RNDIS connectivity.

@github-actions github-actions bot added Arch: arm Issues related to ARM (32-bit) architecture 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]

Fill In The Commit Message: This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary.

The PR summary is missing some key information required by the NuttX PR guidelines. Here's a breakdown:

Missing Information:

  • What functional part of the code is being changed? Be specific. Is it the USB driver? The CDC/NCM driver? A DMA driver?
  • How does the change exactly work? Simply saying "moved the assignment" is not descriptive enough. Explain why the assert was likely there, what conditions would cause it to trigger, and how moving the assignment addresses the root problem.
  • Related Issues: Are there any related issues on the NuttX or NuttX Apps repositories? Even if not, state that explicitly.

"Impact" Section Concerns:

  • Too many "None expected": While it's possible there's no user-facing impact, you can't assume this about aspects like build processes or hardware compatibility. Instead of saying "None Expected", be explicit if something is not applicable (e.g., "Build: N/A"). If you're unsure, say so!
  • DMA Expertise Needed: You acknowledge needing reviewers with DMA knowledge. Highlight this explicitly in the "Impact" section to attract the right reviewers.

Testing:

  • Insufficient Detail: "Custom board" is too vague. Provide:
    • The exact board name (if it's a common one) or a link to its schematic/documentation.
    • The NuttX configuration you used (e.g., sama5d27c_som1_ek_defconfig).
  • No Logs: The template asks for logs before and after the change. Provide these, even if they're simple.

How to Improve:

  1. Be Specific: Don't assume reviewers understand the context. Explain the problem and your solution in detail.
  2. Address All Points: Even if something doesn't apply, acknowledge it in your PR.
  3. Provide Evidence: Back up your claims about testing and impact with logs and clear explanations.

By addressing these points, you'll make it much easier for maintainers to review and merge your PR.

@xiaoxiang781216 xiaoxiang781216 merged commit 3027be7 into apache:master Oct 12, 2024
24 checks passed
@TimJTi TimJTi deleted the SAMA5D2.-Fix-DEBUGASSERT-in-sam_updhs.c branch October 12, 2024 12:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Arch: arm Issues related to ARM (32-bit) architecture 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