drivers: serial: nrfx_uarte: Refactoring poll_out #28961

nordic-krch · 2020-10-06T12:12:53Z

PR contains refactoring of poll_out and test which validates that uart_poll_out, uart_tx and uart_fifo_fill are resilient to preemptions.

Patch is fixing existing issues:
- potential bytes dropping with flow control enabled (#26722)
- busywaiting for ENDTX (asynchronous API) - poor performance
- potential bytes dropping during preemption
- potential uart_tx returning -EBUSY when interrupted poll_out

In general, poll_out implementation is now common for all APIs and relies on the fact that ENDTX and TXSTOPPED events are cleared atomically only before starting new transfer so new poll out can be started if TXSTOPPED (or ENDTX) is set.

Added option (enabled by default) to use PPI to connect ENDTX with STOPTX. This way there is no need for interrupt on ENDTX to stop TX but at the cost of 1 ppi channel (thus option to disable it).

Test is doing following:

poll out in a loop from main thread
poll out from high priority interrupt with sleep between each byte
poll out from k_timer timeout handler (interrupt context)
uart_tx/uart_irq_tx_enable called from another thread

From each context data is sent. Data increments from 0 to 0xf and has context_id on 4 MSB bits. This way receiver is able to detect if any byte is losed from any context.

Additionally, enabled nordic-nrf,uarte as default compatible for uart0 on all nordic boards since UARTE driver can be considered "better" than UART after this change.

Fixes #26722.

pabigot

I've only gone through the first commit, the main one will take more time.

Has it always been understood that it's safe to use a serial device with the polled API when it's also configured for one of the interrupt-based APIs? I think that's what this provides....

pabigot · 2020-10-06T14:09:28Z

tests/drivers/uart/uart_mix_fifo_poll/boards/nrf52840dk_nrf52840.overlay

+	status = "okay";
+	tx-pin = <6>;
+	rx-pin = <8>;
+	rts-pin = <0>;


Is this right? Deleting the property is supposed to be the way to mark that it's not supported. This would seem to assign the XL1 pin to be both RTS and CTS.

By inspection using -1 as the pin ordinal should also have the desired effect. 0xffffffff (the value of NRF_UART_PSEL_DISCONNECTED should also.

yeah, it's a copy paste from test uart_async_api. Will clean it up.

pabigot · 2020-10-06T14:15:25Z

tests/drivers/uart/uart_mix_fifo_poll/testcase.yaml

@@ -0,0 +1,56 @@
+tests:


This should probably have a README explaining how it's supposed to work. Does it not require an external loopback connection to short RX/TX CTS/RTS?

nordic-krch · 2020-10-07T05:50:17Z

@pabigot

Has it always been understood that it's safe to use a serial device with the polled API when it's also configured for one of the interrupt-based APIs?

poll_out is used by printk from any context and at the same type by the logger/shell. Shell works with interrupt driven api. So uart was requested from multiple contexts by both api's.

Mierunski · 2020-10-07T09:05:22Z

drivers/serial/uart_nrfx_uarte.c

+	}
+
+	tx_start(uarte, buf, len);
+
+out:


Suggested change

}

tx_start(uarte, buf, len);

out:

} else {

tx_start(uarte, buf, len);

}

Mierunski · 2020-10-07T09:08:10Z

drivers/serial/uart_nrfx_uarte.c

@@ -511,14 +577,23 @@ static int uarte_nrfx_tx(const struct device *dev, const uint8_t *buf,
 	}

 	data->async->tx_buf = buf;


I think it can be moved right before tx_start at line 591

Mierunski · 2020-10-07T10:39:43Z

drivers/serial/uart_nrfx_uarte.c

+
+	tx_start(uarte, data->int_driven->tx_buffer, len);
+
+out:


Also can be replaced by else

Mierunski · 2020-10-07T11:00:19Z

tests/drivers/uart/uart_mix_fifo_poll/src/main.c

+	static bool enable = true;
+	static uint8_t async_rx_buf[4];
+
+	if (async & !async_rx_enabled) {


Probably boolean operator instead of bitwise

nordic-krch · 2020-10-07T11:49:40Z

@Mierunski thanks for review, comments applied.

drivers/serial/uart_nrfx_uarte.c

anangl · 2020-10-08T07:44:30Z

drivers/serial/uart_nrfx_uarte.c

+			if (--safety_cnt == 0) {
+				LOG_ERR("Dropping byte in poll_out");
+				return;


With this still present, this PR does not really fix #26722. As stated there:

The expected behavior for hardware flow control used with uarte_nrfx_poll_out() is that it would block forever until the other side asserts CTS.

removed any timeout when hwfc is enable, in thread mode also removed a timeout since byte should eventually be sent (unless CTS is asserted)

drivers/serial/uart_nrfx_uarte.c

anangl · 2020-10-08T11:07:56Z

drivers/serial/uart_nrfx_uarte.c

+			if (data->async) {
 				uarte_nrfx_isr_async(dev);
 			}


Is that still needed, actually, now that the loop waits for certain hardware event?

we need to handle potential completion of tx transfer if we are in higher than UART irq priority context. During this completion AMOUNT register is read so it cannot be done after poll out transfer.

Okay, I see. Non-HW byte counting would also benefit from those calls in such scenario.
But when it comes to the AMOUNT register readout, it is done in txstopped_isr(), so we'll have a race condition here, as the TXSTOPPED event can occur between the calls to uarte_nrfx_isr_async() and is_tx_ready().

yes, noticed that yesterday and working on solution.

TXSTOPPED event can occur between the calls to uarte_nrfx_isr_async() and is_tx_ready().

it's not only this, if we are in lower priority interrupt then interrupt will preempt at any moment within uarte_nrfx_isr_async and the same other way round. Basically, we cannot have uarte_nrfx_isr_async there.

anangl · 2020-10-08T11:14:07Z

drivers/serial/uart_nrfx_uarte.c

+				NRFX_WAIT_FOR(is_tx_ready(dev),
+					      POLL_OUT_TIMEOUT_US, 1, res);


Why do we need this timeout? Is this possible that the event won't eventually appear in a scenario with the flow control disabled?

will remove

anangl · 2020-10-09T06:26:35Z

drivers/serial/uart_nrfx_uarte.c

+	if (nrf_uarte_int_enable_check(uarte, NRF_UARTE_INT_ENDTX_MASK) &&
+		nrf_uarte_event_check(uarte, NRF_UARTE_EVENT_ENDTX)) {
+		endtx_isr(dev);


This should be done only if (!get_dev_config(dev)->ppi_endtx). Also in uarte_nrfx_isr_async(), so perhaps it would be then good to move all those checks to endtx_isr?

but when ppi is used then ENDTX interrupt is never enabled so nrf_uarte_int_enable_check(uarte, NRF_UARTE_INT_ENDTX_MASK) is equivalent to !get_dev_config(dev)->ppi_endtx

Right, I missed that.

anangl · 2020-10-09T06:34:37Z

drivers/serial/uart_nrfx_uarte.c

+	if (nrf_uarte_int_enable_check(uarte, NRF_UARTE_INT_ENDTX_MASK) &&
+		nrf_uarte_event_check(uarte, NRF_UARTE_EVENT_ENDTX)) {
+		endtx_isr(dev);
+		return;


This return should be removed. The handler should exit here only when interrupt driven API is not used for the instance, but this case is handled in line 221.

nordic-krch · 2020-10-09T07:02:48Z

I've removed uarte_nrfx_isr_async() from poll_out. Instead i'm trying to capture TX amount register there. I used following approach: in uart_tx variable tx_amount is reset (set to -1), in poll_out if variable is reset, register is read. In TXSTOPPED handling if variable has value then this one is taken instead of register value. It should cover a case when poll_out happens during async TX. TXSTOPPED interrupt will be handled eventually because TXSTOPPED event is always set.

anangl · 2020-10-09T08:40:51Z

drivers/serial/uart_nrfx_uarte.c

@@ -62,7 +69,9 @@ struct uarte_async_cb {
 	 * transmission by uart_poll_out
 	 */
 	const uint8_t *volatile tx_buf;


This no longer needs to be volatile. Also the comment above became outdated.

anangl · 2020-10-09T08:42:15Z

drivers/serial/uart_nrfx_uarte.c

 #endif
-		lock = &data->poll_out_lock;


poll_out_lock should be also removed from the uarte_nrfx_data structure.

anangl · 2020-10-09T08:52:25Z

drivers/serial/uart_nrfx_uarte.c

+	size_t amount = (data->async->tx_amount >= 0) ?
+			data->async->tx_amount : nrf_uarte_tx_amount_get(uarte);


You need to lock interrupts for these operations, as uarte_nrfx_poll_out() might potentially get called in between.

good point, done.

anangl · 2020-10-09T08:56:38Z

drivers/serial/uart_nrfx_uarte.c

@@ -80,6 +89,7 @@ struct uarte_async_cb {
 		gppi_channel_t ppi;
 		uint32_t cnt;
 	} rx_cnt;
+	int tx_amount;


It should be volatile because it is read in the loop in uarte_nrfx_poll_out().

anangl

LGTM now.

anangl · 2020-10-09T09:54:50Z

tests/drivers/uart/uart_mix_fifo_poll/src/main.c

+
+static void process_byte(uint8_t b)
+{
+	int base = b/16;


b >> 4 would be more meaningful in this context.

anangl · 2020-10-09T09:55:52Z

tests/drivers/uart/uart_mix_fifo_poll/src/main.c

+	ok = ((b - src->prev) == 1) || (!b && (src->prev == 0x0F));
+
+	zassert_true(ok, "Unexpected byte received %x, prev:%x",
+			base | b, base | src->prev);


Looks like it should be (base << 4) in both cases.

anangl · 2020-10-09T09:56:48Z

tests/drivers/uart/uart_mix_fifo_poll/src/main.c

+
+	ok = ((b - src->prev) == 1) || (!b && (src->prev == 0x0F));
+
+	zassert_true(ok, "Unexpected byte received %x, prev:%x",


Suggested change

zassert_true(ok, "Unexpected byte received %x, prev:%x",

zassert_true(ok, "Unexpected byte received 0x%02x, prev: 0x%02x",

Added test which is calling uart_poll_out from various contexts and asynchronous/interrupt driven API. Test is validating that calls can be preempted at any moment and no data is lost. Signed-off-by: Krzysztof Chruscinski <[email protected]>

Refactoring poll_out to be ready for handling preemption. uart_tx and uart_fifo_fill modified so they are resilient to being preempted by uart_poll_out. Refactored uart_poll_out implementation to be common for interrupt driven API and asynchronous API. In both APIs active state is detected by evaluating state of ENDTX and TXSTOPPED events. If anyone is set it means that new transfer can be started. Patch is fixing existing issues: - potential bytes dropping with flow control enabled - busywaiting for ENDTX (asynchronous API) - poor performance - potential bytes dropping during preemption - potential uart_tx returning -EBUSY when interrupted poll_out Signed-off-by: Krzysztof Chruscinski <[email protected]>

Update nordic boards to use UARTE driver for UART0 by default. Signed-off-by: Krzysztof Chruscinski <[email protected]>

nordic-krch · 2020-10-09T10:27:26Z

@anangl thanks for review, your final comments applied.

nordic-krch · 2020-10-09T11:49:01Z

@carlescufi can you merge this?

nordic-krch requested review from anangl and Mierunski October 6, 2020 12:12

github-actions bot added area: Boards area: Devicetree area: Tests Issues related to a particular existing or missing test area: Timer Timer labels Oct 6, 2020

nordic-krch added platform: nRF Nordic nRFx area: UART Universal Asynchronous Receiver-Transmitter bug The issue is a bug, or the PR is fixing a bug labels Oct 6, 2020

anangl requested a review from pabigot October 6, 2020 12:15

nordic-krch force-pushed the fix_uart_nrf_poll branch 3 times, most recently from c888b9f to 74fac23 Compare October 6, 2020 13:41

pabigot reviewed Oct 6, 2020

View reviewed changes

Mierunski suggested changes Oct 7, 2020

View reviewed changes

nordic-krch force-pushed the fix_uart_nrf_poll branch from 74fac23 to 5363dfe Compare October 7, 2020 11:49

nordic-krch requested review from carlescufi, galak, ioannisg, lemrey, MaureenHelm and nashif as code owners October 7, 2020 11:49

nordic-krch force-pushed the fix_uart_nrf_poll branch from 5363dfe to c25e267 Compare October 7, 2020 13:39

Mierunski approved these changes Oct 8, 2020

View reviewed changes

anangl requested changes Oct 8, 2020

View reviewed changes

anangl reviewed Oct 8, 2020

View reviewed changes

drivers/serial/uart_nrfx_uarte.c Outdated Show resolved Hide resolved

drivers/serial/uart_nrfx_uarte.c Outdated Show resolved Hide resolved

nordic-krch force-pushed the fix_uart_nrf_poll branch from c25e267 to 6646502 Compare October 8, 2020 10:41

anangl reviewed Oct 8, 2020

View reviewed changes

anangl reviewed Oct 9, 2020

View reviewed changes

nordic-krch force-pushed the fix_uart_nrf_poll branch from 6646502 to c61a0fe Compare October 9, 2020 06:59

anangl reviewed Oct 9, 2020

View reviewed changes

nordic-krch force-pushed the fix_uart_nrf_poll branch 2 times, most recently from 7902cfd to d0e2463 Compare October 9, 2020 09:23

anangl approved these changes Oct 9, 2020

View reviewed changes

nordic-krch added 3 commits October 9, 2020 12:26

board: arm: nrf: Set uart0 compatible to nordic,nrf-uarte

72808d9

Update nordic boards to use UARTE driver for UART0 by default. Signed-off-by: Krzysztof Chruscinski <[email protected]>

nordic-krch force-pushed the fix_uart_nrf_poll branch from d0e2463 to 72808d9 Compare October 9, 2020 10:26

carlescufi merged commit a1a0ffd into zephyrproject-rtos:master Oct 14, 2020

nordic-krch mentioned this pull request Jan 27, 2021

Logging: Using asserts with LOG in high pri ISR context blocks output #30418

Closed

		@@ -511,14 +577,23 @@ static int uarte_nrfx_tx(const struct device dev, const uint8_t buf,
		}

		data->async->tx_buf = buf;

		NRFX_WAIT_FOR(is_tx_ready(dev),
		POLL_OUT_TIMEOUT_US, 1, res);

		size_t amount = (data->async->tx_amount >= 0) ?
		data->async->tx_amount : nrf_uarte_tx_amount_get(uarte);


		ok = ((b - src->prev) == 1) \|\| (!b && (src->prev == 0x0F));

		zassert_true(ok, "Unexpected byte received %x, prev:%x",

drivers: serial: nrfx_uarte: Refactoring poll_out #28961

drivers: serial: nrfx_uarte: Refactoring poll_out #28961

Conversation

nordic-krch commented Oct 6, 2020 • edited Loading

pabigot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordic-krch commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordic-krch commented Oct 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordic-krch commented Oct 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anangl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nordic-krch commented Oct 9, 2020

nordic-krch commented Oct 9, 2020

nordic-krch commented Oct 6, 2020 •

edited

Loading