You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by "TimJTi (via GitHub)" <gi...@apache.org> on 2024/10/11 14:23:11 UTC
[PR] SAMA5D2. Fix DEBUGASSERT triggered in sam_updhs.c [nuttx]
TimJTi opened a new pull request, #14128:
URL: https://github.com/apache/nuttx/pull/14128
## 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SAMA5D2. Fix DEBUGASSERT triggered in sam_updhs.c [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 commented on code in PR #14128:
URL: https://github.com/apache/nuttx/pull/14128#discussion_r1797058905
##########
arch/arm/src/sama5/sam_udphs.c:
##########
@@ -2495,8 +2495,13 @@ static void sam_dma_interrupt(struct sam_usbdev_s *priv, int epno)
* BUFF_COUNT should not be zero. BUFF_COUNT was set to the
* 'inflight' count when the DMA started so the difference will
* give us the actual size of the transfer.
+ *
+ * Get the request from the head of the endpoint request queue first.
*/
+ privreq = sam_rqpeek(&privep->reqq);
Review Comment:
ditto
##########
arch/arm/src/sama5/sam_udphs.c:
##########
@@ -2439,7 +2434,12 @@ static void sam_dma_interrupt(struct sam_usbdev_s *priv, int epno)
}
else if (privep->epstate == UDPHS_EPSTATE_RECEIVING)
{
- /* privreg->inflight holds the total transfer size */
+ /* privreg->inflight holds the total transfer size
+ * Get the request from the head of the endpoint request queue
+ */
+
+ privreq = sam_rqpeek(&privep->reqq);
Review Comment:
why add so many spaces before =
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SAMA5D2. Fix DEBUGASSERT triggered in sam_updhs.c [nuttx]
Posted by "TimJTi (via GitHub)" <gi...@apache.org>.
TimJTi commented on code in PR #14128:
URL: https://github.com/apache/nuttx/pull/14128#discussion_r1797060720
##########
arch/arm/src/sama5/sam_udphs.c:
##########
@@ -2439,7 +2434,12 @@ static void sam_dma_interrupt(struct sam_usbdev_s *priv, int epno)
}
else if (privep->epstate == UDPHS_EPSTATE_RECEIVING)
{
- /* privreg->inflight holds the total transfer size */
+ /* privreg->inflight holds the total transfer size
+ * Get the request from the head of the endpoint request queue
+ */
+
+ privreq = sam_rqpeek(&privep->reqq);
Review Comment:
I was matching the style of the following code. Happy to change if preferred?
##########
arch/arm/src/sama5/sam_udphs.c:
##########
@@ -2495,8 +2495,13 @@ static void sam_dma_interrupt(struct sam_usbdev_s *priv, int epno)
* BUFF_COUNT should not be zero. BUFF_COUNT was set to the
* 'inflight' count when the DMA started so the difference will
* give us the actual size of the transfer.
+ *
+ * Get the request from the head of the endpoint request queue first.
*/
+ privreq = sam_rqpeek(&privep->reqq);
Review Comment:
Ditto!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SAMA5D2. Fix DEBUGASSERT triggered in sam_updhs.c [nuttx]
Posted by "xiaoxiang781216 (via GitHub)" <gi...@apache.org>.
xiaoxiang781216 merged PR #14128:
URL: https://github.com/apache/nuttx/pull/14128
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] SAMA5D2. Fix DEBUGASSERT triggered in sam_updhs.c [nuttx]
Posted by "nuttxpr (via GitHub)" <gi...@apache.org>.
nuttxpr commented on PR #14128:
URL: https://github.com/apache/nuttx/pull/14128#issuecomment-2407533632
[**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues)
__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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org