danielappiagyei-bc commented on code in PR #11020:
URL: https://github.com/apache/nuttx/pull/11020#discussion_r1372064314
##########
arch/arm/src/imxrt/imxrt_serial.c:
##########
@@ -1373,6 +1373,7 @@ static inline void imxrt_serialout(struct imxrt_uart_s
*priv,
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
+ DEBUGASSERT(dmaresidual <= RXDMA_BUFFER_SIZE);
Review Comment:
> The rxfifo is a ring buffer. Written to by the DMA and read from by code.
N.B imxrt_dma_nextrx is only used on the RX side to check if data has been
added by the DMA.
>
> The DMA reads a byte from IMXRT_LPUART_DATA_OFFSET and writes a byte it to
priv->rxfifo adding 1 to to the destination address register and decrementing
the count register. (That starts at RXDMA_BUFFER_SIZE. ) EDMA_CONFIG_LOOPDEST
causes the destination address register to be reloaded with priv->rxfifo at
count = 0 and the count is reloaded.
Agree with everything here and my DMA RX TCD setup is the exact same as what
the code snippet you posted. I have not altered it and am on nuttx 12.
> Therefore it can not write outside the buffer.
I think I said writes were possible outside the buffer in the original issue
I wrote but I was wrong, you are correct here :)
> But the nagging question is how do you see 0..
See the logic in the
[imxrt_dmach_getcount()](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/imxrt_edma.c#L1235)
function that is called by
[imxrt_dma_nextrx()](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/imxrt_serial.c#L1375).
In `imxrt_edma.c` L1249, `remaining` is the value that will be returned and
it is initialized to 0. The number of remaining bytes is only calculated if the
CHANNEL DONE flag (`EDMA_TCD_CSR_DONE`) _isn't_ set. Otherwise, `remaining` is
returned unmodified with the initial value of 0. I've added an `else
{DEBUGPANIC();}` and placed a breakpoint there that does get hit reliably. I
know we configured DMA to operate in a circular fashion but the user manual for
the IMXRT1064 doesn't say that the `EDMA_TCD_CSR_DONE` flag _won't_ be set when
using the circular buffer configuration.
From the reg description:
> The value of this field is reset to 0 by the hardware (_when the channel
is activated_) or by software.
Emphasis mine. The flag does eventually get cleared but I'm not sure whether
it's the DMA hardware clearing it or software (the
[imxrt_dmach_interrupt()](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/imxrt_edma.c#L544)
function in `imxrt_edma.c`. I'm assuming that even though the channel is used
in a circular buffer fashion, it is only "activated" once, so
`imxrt_dmach_interrupt()` must be responsible for clearing it.
> What are your config settings for DMA and LPUART?
I should have included this first, my apologies. I've got a much higher baud
rate than you:
```
#LPUART3
CONFIG_IMXRT_LPUART3=y
CONFIG_LPUART3_BAUD=460800
CONFIG_LPUART3_SERIAL_CONSOLE=n
CONFIG_LPUART3_SERIALDRIVER=y
CONFIG_LPUART3_TXDMA=n
CONFIG_LPUART3_TXBUFSIZE=128
CONFIG_LPUART3_RXDMA=y
# serial RX buffer SMI reads from
CONFIG_LPUART3_RXBUFSIZE=512
```
I also:
- hard-coded the value of `CONFIG_IMXRT_SERIAL_RXDMA_BUFFER_SIZE` in
`imxrt_serial.c` to 256 instead of 32 (hardcoded because it's not a kconfig
(yet))
- Made the UART require 32 consecutive idle characters before setting the
IDLE flag in the LPUART STATUS register (a feature I'll be bringing to nuttx
soon). To do so, I added this line to the bottom of
[imxrt_lpuart_configure](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/imxrt_lowputc.c#L363):
```
modreg32(LPUART_CTRL_IDLECFG_32, LPUART_CTRL_IDLECFG_MASK, base +
IMXRT_LPUART_CTRL_OFFSET);
```
Everything else is whatever defaults kconfig uses.
> imxrt_dma_nextrx returns unsigned int. So the DEBUGASSERT ignores the
dmaresidual > 0 term.
An unsigned int can still be 0 so im not sure why the compiler would
optimize that out. Regardless, `!= 0` displays intent better and works just as
fine
> Will trap the condition. I have a break on the assert firing. But it has
never. 20 minute in.....
If it is indeed a race condition and it takes me 15+ minutes at my much
higher baud rate of 400k I figure it would take even longer for your rate
When I comment out my fix:
```
static int imxrt_dma_nextrx(struct imxrt_uart_s *priv)
{
int dmaresidual = imxrt_dmach_getcount(priv->rxdma);
int nextrx = RXDMA_BUFFER_SIZE - dmaresidual;
/* if index is past end of buffer, reset to 0*/
// if (nextrx == RXDMA_BUFFER_SIZE)
// {
// nextrx = 0;
// }
return nextrx;
}
```
I get CRC errors once again, randing from every 5 seconds to every 100+
seconds
> I could image a case were the count is sampled before the reload but I
have not seen it.
> Even if it did, the net effect is imxrt_dma_nextrx returns 32. But the
only way the return from imxrt_dma_nextrx is used is to compare it with the
priv->rxdmanext to determine if data is in the ring.
> Glitch of 32 would just say there is data, the next read, will see the
truth. So the assert !=0 should stay out.
In
[this](https://github.com/apache/nuttx/blob/master/arch/arm/src/imxrt/imxrt_serial.c#L2356)
comparison, what happens when `rxdmanext` is `0` and `nextrx` is (incorrectly)
`RXDMA_BUFFER_SIZE` instead of `0`. We're comparing the next head of the
circular buffer to the tail. Normally with a circular buffer, when the next
head == the tail, we wouldn't read the data. In this case, we do read it when
we shouldn't: `c = priv->rxfifo[priv->rxdmanext];`
This is the incorrect character being read, which would cause the CRC errors
im seeing, right?
I'm sending at least 230 bytes every 10ms, will update you with solid
numbers when i have a chance
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
