drivers: nrf: Fix nrf uarte fifo_fill function. #11475

Mierunski · 2018-11-19T09:24:43Z

Calling fifo_fill function from uart_nrfx_uarte in the same
interrupt more than once, would break previous transmission.

Following fix adds checking if previous data was sent, and
if not, returns 0.

Calling fifo_fill function from uart_nrfx_uarte in the same interrupt more than once, would break previous transmission. Following fix adds checking if previous data was sent, and if not, returns 0. Signed-off-by: Mieszko Mierunski <[email protected]>

codecov-io · 2018-11-19T10:00:41Z

Codecov Report

Merging #11475 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master   #11475   +/-   ##
=======================================
  Coverage   48.37%   48.37%           
=======================================
  Files         270      270           
  Lines       42080    42080           
  Branches    10132    10132           
=======================================
  Hits        20358    20358           
  Misses      17645    17645           
  Partials     4077     4077

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 03c7d9b...507678a. Read the comment docs.

jakub-uC

Please include a case when user uses fifo fill and disable interrupts. In current implementation data will not be sent correctly.

New commit added.

jakub-uC · 2018-11-19T13:05:18Z

drivers/serial/uart_nrfx_uarte.c

@@ -70,6 +70,9 @@ static inline NRF_UARTE_Type *get_uarte_instance(struct device *dev)
 }

 #ifdef UARTE_INTERRUPT_DRIVEN
+
+volatile bool disable_tx_irq = false;


please make it multi instance ;)

anangl · 2018-11-19T13:20:42Z

drivers/serial/uart_nrfx_uarte.c

@@ -70,6 +71,8 @@ static inline NRF_UARTE_Type *get_uarte_instance(struct device *dev)
 }

 #ifdef UARTE_INTERRUPT_DRIVEN
+
+


Undesirable empty lines.

anangl · 2018-11-19T13:21:29Z

drivers/serial/uart_nrfx_uarte.c

 }

 /** Interrupt driven transfer ready function */
 static int uarte_nrfx_irq_tx_ready_complete(struct device *dev)
 {
 	NRF_UARTE_Type *uarte = get_uarte_instance(dev);

-	return nrf_uarte_event_check(uarte, NRF_UARTE_EVENT_ENDTX);
+	return nrf_uarte_event_check(uarte, NRF_UARTE_EVENT_ENDTX)
+	       && nrf_uarte_int_enable_check(uarte, NRF_UARTE_INT_ENDTX_MASK);


Why?
Would the returned status be incorrect without this second condition?

anangl · 2018-11-19T13:22:29Z

drivers/serial/uart_nrfx_uarte.c

-	nrf_uarte_task_trigger(uarte, NRF_UARTE_TASK_STOPTX);
+	struct uarte_nrfx_data *data = get_dev_data(dev);
+	/* TX IRQ will be disabled after current transmission is finished */
+	data->disable_tx_irq = true;


You should also clear this flag in uarte_nrfx_irq_tx_enable.

anangl · 2018-11-19T13:25:02Z

drivers/serial/uart_nrfx_uarte.c

+	NRF_UARTE_Type *uarte = get_uarte_instance(dev);
+
+	if (data->disable_tx_irq
+	    && nrf_uarte_event_check(uarte, NRF_UARTE_EVENT_ENDTX)) {


Move the && operator to the end of the previous line to stay consistent with the rest of the code in this file.
Same applies to uarte_nrfx_irq_tx_ready_complete.

lemrey

This does not seem to fix our problem yet :/
I am investigating.

Make sure that TX interrupts are disabled after transmission is finished. Signed-off-by: Mieszko Mierunski <[email protected]>

lemrey

Works for me.

carlescufi · 2018-11-19T16:50:43Z

@anangl can you re-review?

evenl · 2018-11-20T07:37:08Z

drivers/serial/uart_nrfx_uarte.c

@@ -28,6 +28,7 @@ struct uarte_nrfx_data {
 	uart_irq_callback_user_data_t cb; /**< Callback function pointer */
 	void *cb_data; /**< Callback function arg */
 	u8_t *tx_buffer;
+	volatile bool disable_tx_irq;


I think the type should be a atomic_t type.

Only independent load and store operations are used which are itself atomic, there is no need for atomic type.

anangl · 2018-11-20T08:34:21Z

drivers/serial/uart_nrfx_uarte.c

+	/* ENDTX flag is always on so that ISR is called when we enable TX IRQ.
+	 * Because of that we have to explicitly check if ENDTX interrupt is
+	 * enabled, otherwise this function would always return true no matter
+	 * what would be the source of interrupt.


Sorry, I don't get it. This function is supposed to return true exactly when the NRF_UARTE_EVENT_ENDTX event is set. And it is not true that this event is always set. It is cleared before a transmission is started, see uarte_nrfx_fifo_fill.

For the application it is a problem if
after disabling interrupts uart_irq_tx_ready() returns true.

For the application it is a problem if
after disabling interrupts uart_irq_tx_ready() returns true.

Then perhaps the application should be corrected.
The UART API does not describe such behavior as required or expected, does it?

Application assumes correctly that it is allowed to send data with uarte_nrfx_fifo_fill and ignore tx interrupt by disabling it. I am pretty sure this is not problem of an application.

When application is used with UART (not UARTE) this works as expected.

The UART API does not describe such behavior as required or expected, does it?

It does. If it doesn't that's bug. Indeed, uart_irq_tx_ready() should be gated on on interrupts being enabled. Well, I don't know if there're some apps which can be fooled otherwise, but we can't dismiss that. The issue, there's single ISR for both UART RX and TX (and for errors, about which a few people remember). So, ISR can be called by RX interrupt, and should not be wrongly doing TX actions if TX is effectively disabled (and if TX interrupt is disabled, then effectively TX is disabled).

I created an issue (#11535) for clarification of this.

This PR fixes current issues with nrf uarte driver acting differently than nrf uart driver. I agree that API has to be clarified and it should be discussed, but changes in this PR don't affect it and are needed now. Also there is ongoing work on new API #10820 which will eventually make current interrupt driven API obsolete and we should focus on that.

I'm +1 on this logic, let me approve.

Agreed. Let's go on with this fix for now.

pfalcon

Approving based on the fact that this improves the situation with consistency of UART drivers.

Mierunski · 2018-11-22T12:55:38Z

@carlescufi could you merge?

Mierunski requested review from galak, anangl and jakub-uC November 19, 2018 09:24

anangl previously approved these changes Nov 19, 2018

View reviewed changes

jakub-uC approved these changes Nov 19, 2018

View reviewed changes

jakub-uC requested a review from carlescufi November 19, 2018 09:49

jakub-uC added bug The issue is a bug, or the PR is fixing a bug area: Drivers labels Nov 19, 2018

carlescufi requested a review from lemrey November 19, 2018 10:30

jakub-uC requested changes Nov 19, 2018

View reviewed changes

jakub-uC reviewed Nov 19, 2018

View reviewed changes

Mierunski force-pushed the nrf_uarte_fifo_fill_fix branch from ad30298 to 8910b22 Compare November 19, 2018 13:07

anangl requested changes Nov 19, 2018

View reviewed changes

lemrey suggested changes Nov 19, 2018

View reviewed changes

drivers: nrf: nrf uarte fix TX interrupts disabling.

507678a

Make sure that TX interrupts are disabled after transmission is finished. Signed-off-by: Mieszko Mierunski <[email protected]>

Mierunski force-pushed the nrf_uarte_fifo_fill_fix branch from 8910b22 to 507678a Compare November 19, 2018 15:11

lemrey approved these changes Nov 19, 2018

View reviewed changes

jakub-uC approved these changes Nov 19, 2018

View reviewed changes

evenl reviewed Nov 20, 2018

View reviewed changes

anangl reviewed Nov 20, 2018

View reviewed changes

zephyrproject-rtos deleted a comment from jakub-uC Nov 20, 2018

lemrey mentioned this pull request Nov 20, 2018

Nordic PCA10059 support without additional Segger debugger #9055

Closed

6 tasks

pfalcon approved these changes Nov 21, 2018

View reviewed changes

anangl approved these changes Nov 22, 2018

View reviewed changes

carlescufi approved these changes Nov 22, 2018

View reviewed changes

carlescufi merged commit 1bfa721 into zephyrproject-rtos:master Nov 22, 2018

Mierunski deleted the nrf_uarte_fifo_fill_fix branch November 22, 2018 13:08

lemrey mentioned this pull request Nov 22, 2018

[DNM] [nrf noup] drivers: serial: uarte: fix UARTE driver nrfconnect/sdk-zephyr#51

Closed

jakub-uC mentioned this pull request Dec 19, 2018

nRF52: UARTE: uart_fifo_fill return value unreliable #10672

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: nrf: Fix nrf uarte fifo_fill function. #11475

drivers: nrf: Fix nrf uarte fifo_fill function. #11475

Mierunski commented Nov 19, 2018

codecov-io commented Nov 19, 2018 •

edited

Loading

jakub-uC left a comment

jakub-uC Nov 19, 2018

anangl Nov 19, 2018

anangl Nov 19, 2018

anangl Nov 19, 2018

anangl Nov 19, 2018

lemrey left a comment

lemrey left a comment

carlescufi commented Nov 19, 2018

evenl Nov 20, 2018

Mierunski Nov 21, 2018

anangl Nov 20, 2018

lemrey Nov 20, 2018 •

edited

Loading

anangl Nov 20, 2018

jakub-uC Nov 20, 2018 •

edited

Loading

pfalcon Nov 20, 2018

anangl Nov 20, 2018 •

edited

Loading

Mierunski Nov 21, 2018

pfalcon Nov 21, 2018

anangl Nov 22, 2018

pfalcon left a comment

Mierunski commented Nov 22, 2018

		@@ -70,6 +71,8 @@ static inline NRF_UARTE_Type get_uarte_instance(struct device dev)
		}

		#ifdef UARTE_INTERRUPT_DRIVEN

drivers: nrf: Fix nrf uarte fifo_fill function. #11475

drivers: nrf: Fix nrf uarte fifo_fill function. #11475

Conversation

Mierunski commented Nov 19, 2018

codecov-io commented Nov 19, 2018 • edited Loading

Codecov Report

jakub-uC 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemrey left a comment

Choose a reason for hiding this comment

lemrey left a comment

Choose a reason for hiding this comment

carlescufi commented Nov 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lemrey Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakub-uC Nov 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anangl Nov 20, 2018 • edited Loading

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

pfalcon left a comment

Choose a reason for hiding this comment

Mierunski commented Nov 22, 2018

codecov-io commented Nov 19, 2018 •

edited

Loading

lemrey Nov 20, 2018 •

edited

Loading

jakub-uC Nov 20, 2018 •

edited

Loading

anangl Nov 20, 2018 •

edited

Loading