drivers: nrf: Rework UARTE shim for uart ASYNC API and add power management #13064

Mierunski · 2019-02-05T14:44:34Z

Rework uart_nrfx_uarte shim to work with asynchronous API.

Fix power management issues

zephyrbot · 2019-02-05T14:49:37Z

All checks are passing now.

Review history of this comment for details about previous failed status.
Note that some checks might have not completed yet.

codecov-io · 2019-02-05T15:43:39Z

Codecov Report

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

@@           Coverage Diff           @@
##           master   #13064   +/-   ##
=======================================
  Coverage   52.93%   52.93%           
=======================================
  Files         309      309           
  Lines       45251    45251           
  Branches    10447    10447           
=======================================
  Hits        23953    23953           
  Misses      16533    16533           
  Partials     4765     4765

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 082cf7a...b1d41ba. Read the comment docs.

Mierunski · 2019-02-14T10:14:06Z

@jarz-nordic @nordic-krch @anangl ping

drivers/serial/uart_nrfx_uarte.c

drivers/serial/Kconfig.nrfx

nordic-krch

im leaving some initial comments but i'm not finished with this! will come back soon.

nordic-krch · 2019-02-14T13:46:45Z

drivers/serial/Kconfig.nrfx

+	  Hardware RX byte counting requires timer instance and one PPI channel
+
+config UART_0_NRF_HW_ASYNC_TIMER
+	int "Timer instance"


what will be the default value? if 0 then it will enable TIMER0 instance and it may conflict with bluetooth as im afraid that it's using timer0 directly.

There is no default value, user has to set it explicitly

So as for now default value is n

If I understand kconfig properly (@ulfalizer can help) this will be a bit undefined, i.e. it's not really a "not-set", cause the value is not boolean. So, it appears to me that the macro is still generated (Without a value though)

If value is not set then cmake will report an error and won't generate the project

We should give the developer the option to build this without menuconfig, and as far as I can understand Kconfig, this requires a default value for int- Kconfig symbols.

This value is taken into account only when developer selects hardware byte counting. When build with default configuration there are no issues with it. I don't want to select a default value here, as it should be explicitly set by developer if he decides to use hardware byte counting.

Understood, now, thanks. So, I suggest you do as @ulfalizer has done in several similar cases, i.e. remove the "depends on" and add the whole Kconfig definition in if UART_0_NRF_HW_ASYNC ... endif #UART_0_NRF_HW_ASYNC

There's some notes on int defaults on the best practices page.

Usually, it's a good idea to give int symbols defaults, because they don't default to zero (for semi-messy reasons). If no default is given, then the prj.conf (or board defconfig files, etc.) must set the symbol. Otherwise, you'll get a .config that generates warnings when read back in, and that probably breaks the menuconfig, because warnings are turned into errors in scripts/kconfig/kconfig.py (there's an issue for it).

(Super nit: Trying to standardize making it # UART_0_NRF_HW_ASYNC instead of #UART_0_NRF_HW_ASYNC too btw. It's more common.)

Re. if vs depends on, depends on is cleaner if there's just a single symbol with the dependency.

If there's many symbols with the same dependencies in a row, if is cleaner.

It's a bit subjective when you have just two symbols or the like. It's good to be aware that if FOO is just a shorthand for adding depends on FOO to all the stuff within at least.

People tend to read a bunch of things into Kconfig that aren't there (probably because the official docs aren't great). :)

drivers/serial/uart_nrfx_uarte.c

nordic-krch · 2019-02-14T14:04:36Z

drivers/serial/uart_nrfx_uarte.c

+	u32_t ret;
+
+	if (IS_ENABLED(CONFIG_UARTE_NRF_HW_ASYNC) && data->async->hw_async) {
+		ret = nrfx_ppi_channel_alloc(&data->async->rx_cnt.ppi);


what about dppi? maybe it would be worth extracting dppi/ppi stuff to another file with simple call like ppi_configure or gppi_configure where g stands for generic :)

There are some differences in how PPI and DPPI handle signals, I believe it can be done but it won't be a part of this PR

nordic-krch · 2019-02-14T14:05:53Z

drivers/serial/uart_nrfx_uarte.c

+			LOG_ERR("Failed to allocate PPI Channel, "
+					"switching to software byte counting.");
+			data->async->hw_async = false;
+			data->async->rx_cnt.cnt = 0;


return error

If we return error here, device won't be initialized and if this UARTE instance is set as shell output, user won't see any information what has gone wrong.

anangl

A couple of minor comments for the beginning. I just started digging in the code (well, some comments there could help a lot...).

dts/bindings/arm/nordic,nrf-dppi.yaml

soc/arm/nordic_nrf/Kconfig.peripherals

soc/arm/nordic_nrf/nrf52/Kconfig.soc

drivers/serial/uart_nrfx_uarte.c

anangl

Some more comments. Not finished reviewing.

drivers/serial/uart_nrfx_uarte.c

anangl · 2019-02-22T11:50:31Z

drivers/serial/uart_nrfx_uarte.c

+	user_callback(dev, &evt);
+	data->async->rx_buf = NULL;
+	if (data->async->rx_next_buf) {
+		evt.data.rx_buf.buf = data->async->rx_next_buf;


evt.type = UART_RX_BUF_RELEASED; missing (the evt structure might be modified in evt_handler called from user_callback).
BTW do you know why API does not define evt in uart_callback_t as a pointer to a constant structure?

Good point, I will patch it later

drivers/serial/uart_nrfx_uarte.c

anangl · 2019-02-22T12:35:59Z

drivers/serial/uart_nrfx_uarte.c

+	data->async->rx_total_byte_cnt += evt.data.rx.len;
+	evt.type = UART_RX_BUF_RELEASED;
+	evt.data.rx_buf.buf = data->async->rx_buf;
+	user_callback(dev, &evt);


What if the user decides to reuse the released buffer and provides it in a call to uart_rx_buf_rsp from the event handler? API does not seem to forbid such scenario.

uart_rx_buf_rsp can be called only in response to UART_RX_BUF_REQUEST

drivers/serial/uart_nrfx_uarte.c

anangl · 2019-02-22T12:53:15Z

drivers/serial/uart_nrfx_uarte.c

@@ -286,8 +820,24 @@ static int uarte_nrfx_poll_in(struct device *dev, unsigned char *c)
 static void uarte_nrfx_poll_out(struct device *dev,


What about uarte_nrfx_poll_in? Currently it will always return -1 when CONFIG_UART_ASYNC_API is enabled.

There is no possibility to handle poll_in while using asynchronous API, I will change it to return -EBUSY

Mierunski · 2019-03-21T14:55:09Z

Fixes #12501

ioannisg

Some comments/suggestions from my side

boards/arm/nrf9160_pca10090/nrf9160_pca10090_common.dts

drivers/serial/Kconfig.nrfx

ioannisg · 2019-03-21T15:26:00Z

drivers/serial/Kconfig.nrfx

+	  Hardware RX byte counting requires timer instance and one PPI channel
+
+config UART_0_NRF_HW_ASYNC_TIMER
+	int "Timer instance"


If I understand kconfig properly (@ulfalizer can help) this will be a bit undefined, i.e. it's not really a "not-set", cause the value is not boolean. So, it appears to me that the macro is still generated (Without a value though)

dts/arm/nordic/nrf9160_common.dtsi

ext/hal/nordic/Kconfig

drivers/serial/Kconfig.nrfx

Rework uart_nrfx_uarte shim to work with asynchronous API. Signed-off-by: Mieszko Mierunski <[email protected]>

Add DPPIC to dts. Add HAS_HW_NRF_DPPIC to nrf91 soc. Signed-off-by: Mieszko Mierunski <[email protected]>

Enable TIMER1 by default in nrf9160_pca10090 so it can be selected by user for hardware byte counting in UARTE or other purpose. Signed-off-by: Mieszko Mierunski <[email protected]>

Modify driver to use PPI or DPPI depending on soc. Signed-off-by: Mieszko Mierunski <[email protected]>

Add new test case Signed-off-by: Mieszko Mierunski <[email protected]>

Add nrf9160_pca10090 to UART async test. Add myself as codeowner to uart_async_api tests. Signed-off-by: Mieszko Mierunski <[email protected]>

Adds power management to uarte shim. Fixes 12501 Signed-off-by: Mieszko Mierunski <[email protected]>

ulfalizer

Kconfig nits fixed

Mierunski added area: Drivers platform: nRF Nordic nRFx labels Feb 5, 2019

Mierunski requested review from carlescufi, nordic-krch, anangl and jakub-uC February 5, 2019 14:44

Mierunski force-pushed the nrfx_uarte_async_api branch from 8f07709 to 33f8947 Compare February 5, 2019 15:06

Mierunski force-pushed the nrfx_uarte_async_api branch 2 times, most recently from eb8d65c to 29b7988 Compare February 6, 2019 10:22

Mierunski requested a review from ioannisg as a code owner February 7, 2019 10:17

Mierunski force-pushed the nrfx_uarte_async_api branch from d15a823 to fbcf9a9 Compare February 7, 2019 10:26

jakub-uC reviewed Feb 14, 2019

View reviewed changes

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

jakub-uC reviewed Feb 14, 2019

View reviewed changes

drivers/serial/Kconfig.nrfx Show resolved Hide resolved

nordic-krch requested changes Feb 14, 2019

View reviewed changes

anangl reviewed Feb 18, 2019

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

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

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

anangl reviewed Feb 19, 2019

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

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

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

anangl reviewed Feb 19, 2019

View reviewed changes

drivers/serial/uart_nrfx_uarte.c Show resolved Hide resolved

Mierunski force-pushed the nrfx_uarte_async_api branch 3 times, most recently from 22af70a to 72d4b40 Compare February 20, 2019 09:36

anangl reviewed Feb 21, 2019

View reviewed changes

anangl reviewed Feb 22, 2019

View reviewed changes

Mierunski force-pushed the nrfx_uarte_async_api branch 2 times, most recently from 50d6b24 to be5f6d8 Compare February 26, 2019 13:47

ioannisg reviewed Mar 21, 2019

View reviewed changes

Mierunski force-pushed the nrfx_uarte_async_api branch from a978297 to e37d672 Compare March 25, 2019 10:11

Mierunski added the bug The issue is a bug, or the PR is fixing a bug label Mar 25, 2019

anangl requested changes Mar 25, 2019

View reviewed changes

ext/hal/nordic/Kconfig Outdated Show resolved Hide resolved

Mierunski force-pushed the nrfx_uarte_async_api branch from e37d672 to 224136e Compare March 25, 2019 14:40

anangl approved these changes Mar 25, 2019

View reviewed changes

jakub-uC approved these changes Mar 26, 2019

View reviewed changes

carlescufi mentioned this pull request Mar 26, 2019

nRF52: UARTE lacks pm interface #12501

Closed

carlescufi added this to the v1.14.0 milestone Mar 26, 2019

carlescufi requested a review from ulfalizer March 26, 2019 10:59

carlescufi changed the title ~~drivers: nrf: Rework UARTE shim for uart ASYNC API.~~ drivers: nrf: Rework UARTE shim for uart ASYNC API and add power management Mar 26, 2019

ulfalizer suggested changes Mar 26, 2019

View reviewed changes

drivers/serial/Kconfig.nrfx Show resolved Hide resolved

drivers/serial/Kconfig.nrfx Show resolved Hide resolved

drivers/serial/Kconfig.nrfx Show resolved Hide resolved

carlescufi added the TSC Topics that need TSC discussion label Mar 26, 2019

Mierunski force-pushed the nrfx_uarte_async_api branch 2 times, most recently from 5cc55c7 to b545a6f Compare March 27, 2019 10:28

Mieszko Mierunski added 7 commits March 27, 2019 11:38

drivers: nrf: Rework UARTE shim for uart ASYNC API.

ef33496

Rework uart_nrfx_uarte shim to work with asynchronous API. Signed-off-by: Mieszko Mierunski <[email protected]>

dts: nrf: Add DPPIC to device tree.

fd2f1d3

Add DPPIC to dts. Add HAS_HW_NRF_DPPIC to nrf91 soc. Signed-off-by: Mieszko Mierunski <[email protected]>

boards: nrf: Enable TIMER1 by default in nrf9160_pca10090

bafa4bd

Enable TIMER1 by default in nrf9160_pca10090 so it can be selected by user for hardware byte counting in UARTE or other purpose. Signed-off-by: Mieszko Mierunski <[email protected]>

drivers: nrf: Enable usage of Async UART API for UARTE on nrf91

587aa84

Modify driver to use PPI or DPPI depending on soc. Signed-off-by: Mieszko Mierunski <[email protected]>

tests: uart: Add chained write test case for UART async API

00f384f

Add new test case Signed-off-by: Mieszko Mierunski <[email protected]>

tests: uart: Add nrf9160_pca10090 to UART async test

d61351a

Add nrf9160_pca10090 to UART async test. Add myself as codeowner to uart_async_api tests. Signed-off-by: Mieszko Mierunski <[email protected]>

drivers: nrf: Add power management to uarte shim

b1d41ba

Adds power management to uarte shim. Fixes 12501 Signed-off-by: Mieszko Mierunski <[email protected]>

Mierunski force-pushed the nrfx_uarte_async_api branch from b545a6f to b1d41ba Compare March 27, 2019 10:38

Mierunski requested a review from ulfalizer March 27, 2019 11:45

ulfalizer approved these changes Mar 27, 2019

View reviewed changes

galak approved these changes Mar 27, 2019

View reviewed changes

nashif removed the TSC Topics that need TSC discussion label Mar 27, 2019

nashif merged commit 0b2946f into zephyrproject-rtos:master Mar 28, 2019

Mierunski deleted the nrfx_uarte_async_api branch March 28, 2019 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: nrf: Rework UARTE shim for uart ASYNC API and add power management #13064

drivers: nrf: Rework UARTE shim for uart ASYNC API and add power management #13064

Mierunski commented Feb 5, 2019 •

edited by carlescufi

Loading

zephyrbot commented Feb 5, 2019 •

edited

Loading

codecov-io commented Feb 5, 2019 •

edited

Loading

Mierunski commented Feb 14, 2019

nordic-krch left a comment

nordic-krch Feb 14, 2019

Mierunski Feb 19, 2019

jakub-uC Feb 27, 2019

ioannisg Mar 21, 2019 •

edited

Loading

Mierunski Mar 25, 2019

ioannisg Mar 25, 2019

Mierunski Mar 25, 2019

ioannisg Mar 25, 2019

ulfalizer Mar 26, 2019 •

edited

Loading

ulfalizer Mar 26, 2019 •

edited

Loading

nordic-krch Feb 14, 2019

Mierunski Feb 26, 2019

nordic-krch Feb 14, 2019

Mierunski Feb 19, 2019

anangl left a comment

anangl left a comment

anangl Feb 22, 2019

Mierunski Feb 26, 2019

anangl Feb 22, 2019

Mierunski Feb 26, 2019

anangl Feb 22, 2019

Mierunski Feb 26, 2019

Mierunski commented Mar 21, 2019

ioannisg left a comment

ioannisg Mar 21, 2019 •

edited

Loading

ulfalizer left a comment

		@@ -286,8 +820,24 @@ static int uarte_nrfx_poll_in(struct device dev, unsigned char c)
		static void uarte_nrfx_poll_out(struct device *dev,

drivers: nrf: Rework UARTE shim for uart ASYNC API and add power management #13064

drivers: nrf: Rework UARTE shim for uart ASYNC API and add power management #13064

Conversation

Mierunski commented Feb 5, 2019 • edited by carlescufi Loading

zephyrbot commented Feb 5, 2019 • edited Loading

codecov-io commented Feb 5, 2019 • edited Loading

Codecov Report

Mierunski commented Feb 14, 2019

nordic-krch 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

ioannisg Mar 21, 2019 • 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

Choose a reason for hiding this comment

ulfalizer Mar 26, 2019 • edited Loading

Choose a reason for hiding this comment

ulfalizer Mar 26, 2019 • 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

Choose a reason for hiding this comment

anangl left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mierunski commented Mar 21, 2019

ioannisg left a comment

Choose a reason for hiding this comment

ioannisg Mar 21, 2019 • edited Loading

Choose a reason for hiding this comment

ulfalizer left a comment

Choose a reason for hiding this comment

Mierunski commented Feb 5, 2019 •

edited by carlescufi

Loading

zephyrbot commented Feb 5, 2019 •

edited

Loading

codecov-io commented Feb 5, 2019 •

edited

Loading

ioannisg Mar 21, 2019 •

edited

Loading

ulfalizer Mar 26, 2019 •

edited

Loading

ulfalizer Mar 26, 2019 •

edited

Loading

ioannisg Mar 21, 2019 •

edited

Loading