#596 STM32 USARTv1: incorrect txend2_cb callback behavior

[Yuriy Cherniavsky]

In current implementation (after fix [#586]) txend2_cb callback call immediately after return from uart_lld_serve_tx_end_irq routine, before last data byte actually send.
I have noticed, this because in this callback, I signal on semaphore, and another task on receiving this signal close uart periphery. As result last data byte send incorrect (in my case it send 0xFF instead of 0x5A).

This is tiny dirty patch which fix this issue:

diff --git a/chibios/os/hal/platforms/STM32/USARTv1/uart_lld.c b/chibios/os/hal/platforms/STM32/USARTv1/uart_lld.c
index 5f8dada..eb4bc14 100644
--- a/chibios/os/hal/platforms/STM32/USARTv1/uart_lld.c
+++ b/chibios/os/hal/platforms/STM32/USARTv1/uart_lld.c
@@ -295,8 +295,10 @@ static void uart_lld_serve_tx_end_irq(UARTDriver *uartp, uint32_t flags) {
   /* Only enable TC interrupt if there's a callback attached to it.
      We have to do it here, rather than earlier, because TC flag is set
      until transmission starts.*/

-  if (uartp->config->txend2_cb != NULL)
+  if (uartp->config->txend2_cb != NULL) {
+    uartp->usart->SR = ~USART_SR_TC;
     uartp->usart->CR1 |= USART_CR1_TCIE;
+  }

   /* A callback is generated, if enabled, after a completed transfer.*/
   uartp->txstate = UART_TX_COMPLETE;

But I think next patch is more clear implement txend2_cb callback functional, by set TCIE exactly on send start (instead on send finish):

diff --git a/chibios/os/hal/platforms/STM32/USARTv1/uart_lld.c b/chibios/os/hal/platforms/STM32/USARTv1/uart_lld.c
index 5f8dada..bf5944e 100644
--- a/chibios/os/hal/platforms/STM32/USARTv1/uart_lld.c
+++ b/chibios/os/hal/platforms/STM32/USARTv1/uart_lld.c
@@ -292,12 +292,6 @@ static void uart_lld_serve_tx_end_irq(UARTDriver *uartp, uint32_t flags) {

   dmaStreamDisable(uartp->dmatx);


-  /* Only enable TC interrupt if there's a callback attached to it.
-     We have to do it here, rather than earlier, because TC flag is set
-     until transmission starts.*/
-  if (uartp->config->txend2_cb != NULL)
-    uartp->usart->CR1 |= USART_CR1_TCIE;
-
   /* A callback is generated, if enabled, after a completed transfer.*/
   uartp->txstate = UART_TX_COMPLETE;
   if (uartp->config->txend1_cb != NULL)
@@ -766,6 +760,15 @@ void uart_lld_start_send(UARTDriver *uartp, size_t n, const void *txbuf) {
   dmaStreamSetMode(uartp->dmatx, uartp->dmamode    | STM32_DMA_CR_DIR_M2P |
                                  STM32_DMA_CR_MINC | STM32_DMA_CR_TCIE);
   dmaStreamEnable(uartp->dmatx);
+
+  /* Only enable TC interrupt if there's a callback attached to it.
+     We have to do it here, rather than earlier, because TC flag is set
+     until transmission starts.
+     Also we need to clear TC flag which could be set before. */
+  if (uartp->config->txend2_cb != NULL) {
+    uartp->usart->SR = ~USART_SR_TC;
+    uartp->usart->CR1 |= USART_CR1_TCIE;
+  }
 }

 /**

[Giovanni Di Sirio]

Hi,

I merged the second fix but I moved it before enabling the DMA, could you give it a try? I also applied the same change to the USARTv2 driver, I imagine it is affected as well.

Giovanni

[Yuriy Cherniavsky]

Hi Giovanni,

I have tested the second fix with moving it before enabling the DMA (could not find the place where you merged it, so moved it by myself), and it is work as expected.

Yuriy.

P.S.
To be clear - tested code:

diff --git a/chibios/os/hal/platforms/STM32/USARTv1/uart_lld.c b/chibios/os/hal/platforms/STM32/USARTv1/uart_lld.c
index 5f8dada..7a5034d 100644
--- a/chibios/os/hal/platforms/STM32/USARTv1/uart_lld.c
+++ b/chibios/os/hal/platforms/STM32/USARTv1/uart_lld.c
@@ -292,12 +292,6 @@ static void uart_lld_serve_tx_end_irq(UARTDriver *uartp, uint32_t flags) {

   dmaStreamDisable(uartp->dmatx);


-  /* Only enable TC interrupt if there's a callback attached to it.
-     We have to do it here, rather than earlier, because TC flag is set
-     until transmission starts.*/
-  if (uartp->config->txend2_cb != NULL)
-    uartp->usart->CR1 |= USART_CR1_TCIE;
-
   /* A callback is generated, if enabled, after a completed transfer.*/
   uartp->txstate = UART_TX_COMPLETE;
   if (uartp->config->txend1_cb != NULL)
@@ -760,6 +754,15 @@ void uart_lld_stop(UARTDriver *uartp) {
  */
 void uart_lld_start_send(UARTDriver *uartp, size_t n, const void *txbuf) {


+  /* Only enable TC interrupt if there's a callback attached to it.
+     We have to do it here, rather than earlier, because TC flag is set
+     until transmission starts.
+     Also we need to clear TC flag which could be set before. */
+  if (uartp->config->txend2_cb != NULL) {
+    uartp->usart->SR = ~USART_SR_TC;
+    uartp->usart->CR1 |= USART_CR1_TCIE;
+  }
+
   /* TX DMA channel preparation and start.*/
   dmaStreamSetMemory0(uartp->dmatx, txbuf);
   dmaStreamSetTransactionSize(uartp->dmatx, n);

[Yuriy Cherniavsky]

Hi Giovanni,

Recently I saw you commit "Fixed bug #596." in stable_2.6.x branch and need to say that it is incorrect. May be it is some meagre issue.
You sad that you "merged the second fix but I moved it before enabling the DMA", but in result commit in function uart_lld_serve_tx_end_irq was not removed block:

  /* Only enable TC interrupt if there's a callback attached to it.
     We have to do it here, rather than earlier, because TC flag is set
     until transmission starts.*/
  if (uartp->config->txend2_cb != NULL)
    uartp->usart->CR1 |= USART_CR1_TCIE;

and added block in function uart_lld_start_send was not moved before enabling the DMA.

Really strange commit :). And if function uart_lld_start_send work properly in any case, current implementation in function uart_lld_serve_tx_end_irq cause odd callback call.

[Yuriy Cherniavsky]

Sorry for importunity about final fixing of this issue.

If my previous comment wasn't clear, maybe I could provide more explanation about it?

[Giovanni Di Sirio]

Hi,

I planned to return on this during the weekend, if you could add info or a patch it would be better. I thought to have implemented your fix already (enabling the IRQ at beginning of transaction).

Giovanni

[Yuriy Cherniavsky]

I have reviewed you commits for this issue and understand, that you simply forgot to delete code block, which I mention in my comment about strange commit, in USARTv1 and delete it, as expected, in USARTv2 :).
So the final patch is really simple:

diff --git a/os/hal/platforms/STM32/USARTv1/uart_lld.c b/os/hal/platforms/STM32/USARTv1/uart_lld.c
index 07a8633..3c799c1 100644
--- a/os/hal/platforms/STM32/USARTv1/uart_lld.c
+++ b/os/hal/platforms/STM32/USARTv1/uart_lld.c
@@ -292,12 +292,6 @@ static void uart_lld_serve_tx_end_irq(UARTDriver *uartp, uint32_t flags) {

   dmaStreamDisable(uartp->dmatx);


-  /* Only enable TC interrupt if there's a callback attached to it.
-     We have to do it here, rather than earlier, because TC flag is set
-     until transmission starts.*/
-  if (uartp->config->txend2_cb != NULL)
-    uartp->usart->CR1 |= USART_CR1_TCIE;
-
   /* A callback is generated, if enabled, after a completed transfer.*/
   uartp->txstate = UART_TX_COMPLETE;
   if (uartp->config->txend1_cb != NULL)

[Giovanni Di Sirio]

Hi,

I just removed that part of code.

Giovanni

[Yuriy Cherniavsky]

Giovanni, thanks.
See this "More on bug #596." commit in SourceForge, but, for some reason, it isn't available on GitHub yet. Waiting for GitHub ...