nrf-uarte problems with uart_irq_tx_disable() in handler

[pabigot]

The CDC_ACM sample uses an idiom for interrupt-driven UART that involves a loop that is repeated as long as an interrupt is pending.

This idiom does not work for nordic,nrf-uarte. (It works for nordic,nrf-uart; I have not tested other platforms.) The failure is due to the invocation of uart_irq_tx_disable() within the application-provided handler. For UARTE this merely sets a flag disable_tx_irq, which is only inspected and acted upon in the FLIH that invokes the application-provided handler.

Consequently though the code asked that transmit interrupts be disabled, when it loops back around uart_irq_is_pending() returns true, as does uart_irq_tx_ready(), which merely causes another attempt to disable transmit that is again ignored.

The UART API is incompletely defined with respect to uart_irq_update() and its relation to the pending and ready checks: it says it must be invoked once per handler invocation, but does not address whether it needs to be invoked again if the handler implements a loop on uart_irq_is_pending().

If the loop solution in CDC-ACM is legal code, then the Nordic UARTE driver should be fixed. A possible solution is to change the condition for uarte_nrfx_irq_tx_ready_complete():

diff --git a/drivers/serial/uart_nrfx_uarte.c b/drivers/serial/uart_nrfx_uarte.c
index 7631c4356c..105c0915c1 100644
--- a/drivers/serial/uart_nrfx_uarte.c
+++ b/drivers/serial/uart_nrfx_uarte.c
@@ -1077,6 +1077,7 @@ static int uarte_nrfx_irq_tx_ready_complete(struct device *dev)
         * what would be the source of interrupt.
         */
        return nrf_uarte_event_check(uarte, NRF_UARTE_EVENT_ENDTX) &&
+               !get_dev_data(dev)->int_driven->disable_tx_irq &&
               nrf_uarte_int_enable_check(uarte, NRF_UARTE_INT_ENDTX_MASK);
 }

This solves the problem in my code, but may have side effects that break other uses.

If the loop solution in the CDC-ACM examples is not legal code then it should be revised to avoid demonstrating a non-portable use of the interrupt-driven UART API.

[marc-cpdesign]

I ran into this a short while back too, but had forgotten about it after moving to uarte ... I agree about the UART API being a bit unclear, but it seemed that doing
while (uart_irq_update() && uart_irq_is_pending()) { ...handle loop.. } is done in other tests and examples... (This seems sensible to me also otherwise the int handler would just be called immediately after again if there is an int pending)

My fix was to modify uart_irq_update() so that it handled the disable_tx_irq flag:

 /** Interrupt driven interrupt update function */
 static int uarte_nrfx_irq_update(struct device *dev)
 {
	struct uarte_nrfx_data *data = get_dev_data(dev);
	NRF_UARTE_Type *uarte = get_uarte_instance(dev);

	if (data->int_driven->disable_tx_irq &&
	    nrf_uarte_event_check(uarte, NRF_UARTE_EVENT_ENDTX)) {
		nrf_uarte_int_disable(uarte, NRF_UARTE_INT_ENDTX_MASK);

		/* If there is nothing to send, driver will save an energy
		 * when TX is stopped.
		 */
		nrf_uarte_task_trigger(uarte, NRF_UARTE_TASK_STOPTX);

		data->int_driven->disable_tx_irq = false;
	}

 	return 1;
 }

(this disable code is copy/pasted from another part of the file, it should probably be in a static helper func)