UART RTT devicetree & async support #27704

JordanYates · 2020-08-21T06:10:19Z

Switches instantiation of the uart_rtt driver from Kconfig symbols to devicetree nodes.
Adds CONFIG_UART_ASYNC_API support to the driver, which eliminates hard faults when the RTT backend is used with async functions. Because asynchronous reception is not possible, only the TX path is implemented.

There are no in tree users of the previous CONFIG_UART_RTT_* symbols, so there are no tests validating the new behavior. The commit message contains an example of the devicetree nodes required to instantiate driver instances.

henrikbrixandersen · 2020-08-21T08:26:06Z

What is the real benefit of this? RTT is not a hardware device, it is purely software.

JordanYates · 2020-08-21T08:41:39Z

What is the real benefit of this? RTT is not a hardware device, it is purely software.

The benefit is that it allows the use of RTT in chosen and alias nodes.
I agree that the implementation is pure software, but it is pretending to be a hardware device, and therefore is seems reasonable (to me at least) to specify in devicetree.

As a more concrete example, I have a binary data logging system built on top of serial ports, and boards that use both UART and RTT as the primary serial comms output. This change lets me do

MY_SERIAL_LOGGER_INIT(DT_LABEL(DT_ALIAS(logger_backend)))

Instead of trying to detect somehow if this particular board uses serial or RTT.

drivers/serial/uart_rtt.c

pdunaj · 2020-08-28T07:59:46Z

drivers/serial/uart_rtt.c

+	ARG_UNUSED(timeout);
+
+	/* Output the buffer */
+	SEGGER_RTT_Write(ch, buf, len);


SEGGER_RTT_Write is taking a lock (mutex in case of zephyr). This can limit places where this driver can be used.
Does Zephyr allow calling uart_tx from atomic context?

I suspect that its not defined, and probably won't be until #18970 is resolved.
The nrfx and sam0 drivers are the only ones that currently implement UART_ASYNC_API.
They both look like they will work/return -EBUSY with interrupts disabled.
I will update to manually take the lock.

Not a bad idea but it will cover only cases when uart_tx is called from under irq_lock. It won't cover the case when it is called from an interrupt (mutex cannot be taken from an interrupt).

Maybe it would be enough to leave SEGGER_RTT_Write and add a check blocking usage from atomic context?

there is no is_atomic api in Zephyr but check can be done as

int key = irq_lock(); irq_unlock(key); bool is_atomic = arch_irq_unlocked(key) || k_is_in_isr(); if (is_atomic) return -ENOTSUP;

I'd rather not fail when IRQ's are locked if the mutex take would still succeed.
Is adding the following sufficient?

if (k_in_in_isr()) { return -ENOTSUP; }

That's ok. But I strongly suggest adding a check against calling from an interrupt. If anybody does it at least problem will be visible and explicit.

Doesn't this setup cover the cases?
If in ISR the k_in_in_isr will fail.
If interrupts are locked in a thread the TX will pass if mutex is free, fail if it is in use.
Otherwise it will pass.
Is there some other situation I'm missing?

No, it's fine :)

henrikbrixandersen · 2020-08-28T17:24:42Z

I would still like a discussion on if this really is the right solution here.

I often find myself enabling the RTT console as a pure software configuration change while debugging. It would be quite bothersome to have to edit the devicetree for this to work. Is there any way we could support both methods? After all, this is pure software functionality.

JordanYates · 2020-08-29T00:40:22Z

I often find myself enabling the RTT console as a pure software configuration change while debugging. It would be quite bothersome to have to edit the devicetree for this to work. Is there any way we could support both methods? After all, this is pure software functionality.

My understanding is that changes to the uart_rtt driver are completely independent of the RTT console. log_backend_rtt.c calls the SEGGER functions directly, as does the char_out function provided to printk in rtt_console.c. This PR therefore shouldn't change anything about enabling RTT for debug.

JordanYates · 2020-09-02T23:47:11Z

@galak @carlescufi any chance of another review, hoping to get this into 2.4
Actually, merge conflicts from device updates, will fix.

nordic-krch

I think there is a zephyr policy that code must be somehow validated in the tree (compiled). Is it used somewhere?

drivers/serial/uart_rtt.c

Instantiate RTT UART instances from devicetree nodes instead of from Kconfig symbols. While RTT is implemented using software, not hardware, it is emulating a hardware device, and thus should be configured through devicetree. This allows the simulated UART device to be selected via devicetree aliases and chosen nodes. The following devicetree snippet will instantiate RTT channels 0 and 2 as UART devices. ``` / { rtt0: rtt_terminal { compatible = "segger,rtt-uart"; label = "rtt_terminal"; status = "okay"; }; rtt2: rtt_secondary { compatible = "segger,rtt-uart"; label = "rtt_app_specific"; status = "okay"; }; }; ``` Fixes the RTT portion of #10621. Signed-off-by: Jordan Yates <[email protected]>

Add support for `CONFIG_UART_ASYNC_API` to the uart_rtt driver. As RTT provides no mechanism for knowing when new data has arrived, the reception commands all return errors. This fixes hard faults when asynchronous calls are made on the uart_rtt driver, in addition to providing a small level of functionality. Signed-off-by: Jordan Yates <[email protected]>

JordanYates · 2020-09-03T08:39:11Z

I think there is a zephyr policy that code must be somehow validated in the tree (compiled). Is it used somewhere?

uart_rtt was previously not used anywhere in tree. I can whip up a sample application.

JordanYates · 2020-09-03T10:02:49Z

I think there is a zephyr policy that code must be somehow validated in the tree (compiled). Is it used somewhere?

@nordic-krch Pretty sure the testcase I added will compile for the 'qemu_cortex_m0' platform at least. (It does locally for me)

nordic-krch · 2020-09-03T10:23:52Z

don't you need overlay which will configure uart rtt?

JordanYates · 2020-09-03T10:33:59Z

don't you need overlay which will configure uart rtt?

Thats the segger_rtt.overlay

Add a build only test to validate correct compilation of the uart_rtt driver. No harness that connects to RTT is defined, and therefore the test can't be run by CI. Signed-off-by: Jordan Yates <[email protected]>

nordic-krch · 2020-09-03T12:42:45Z

Thats the segger_rtt.overlay

somehow i missed that.

Populate the `channel` index when constructing configuration structs for secondary RTT channels. Originally missed in #27704. Fixes #54955. Signed-off-by: Jordan Yates <[email protected]>

Populate the `channel` index when constructing configuration structs for secondary RTT channels. Originally missed in zephyrproject-rtos#27704. Fixes zephyrproject-rtos#54955. Signed-off-by: Jordan Yates <[email protected]>

JordanYates requested review from carlescufi and galak as code owners August 21, 2020 06:10

github-actions bot added area: Devicetree area: Devicetree Binding PR modifies or adds a Device Tree binding labels Aug 21, 2020

carlescufi requested review from pdunaj, nordic-krch, Mierunski and jakub-uC August 27, 2020 16:14

pdunaj reviewed Aug 28, 2020

View reviewed changes

drivers/serial/uart_rtt.c Show resolved Hide resolved

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

pdunaj reviewed Aug 28, 2020

View reviewed changes

pdunaj approved these changes Aug 28, 2020

View reviewed changes

nordic-krch reviewed Sep 3, 2020

View reviewed changes

drivers/serial/uart_rtt.c Show resolved Hide resolved

Jordan Yates added 2 commits September 3, 2020 18:22

github-actions bot added the area: Tests Issues related to a particular existing or missing test label Sep 3, 2020

tests: uart_async_api: build segger rtt driver

b1f15ec

Add a build only test to validate correct compilation of the uart_rtt driver. No harness that connects to RTT is defined, and therefore the test can't be run by CI. Signed-off-by: Jordan Yates <[email protected]>

nordic-krch approved these changes Sep 3, 2020

View reviewed changes

carlescufi merged commit c4d0c1e into zephyrproject-rtos:master Sep 3, 2020

JordanYates deleted the uart_rtt_async branch September 4, 2020 07:12

JordanYates mentioned this pull request Feb 25, 2023

drivers: serial: uart_rtt: fix multi-channel #55183

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

UART RTT devicetree & async support #27704

UART RTT devicetree & async support #27704

JordanYates commented Aug 21, 2020

henrikbrixandersen commented Aug 21, 2020

JordanYates commented Aug 21, 2020 •

edited

Loading

pdunaj Aug 28, 2020

JordanYates Aug 28, 2020

JordanYates Aug 28, 2020

pdunaj Aug 28, 2020

JordanYates Aug 28, 2020

pdunaj Aug 28, 2020

JordanYates Aug 28, 2020

pdunaj Aug 28, 2020

henrikbrixandersen commented Aug 28, 2020

JordanYates commented Aug 29, 2020

JordanYates commented Sep 2, 2020 •

edited

Loading

nordic-krch left a comment

JordanYates commented Sep 3, 2020

JordanYates commented Sep 3, 2020 •

edited

Loading

nordic-krch commented Sep 3, 2020

JordanYates commented Sep 3, 2020 •

edited

Loading

nordic-krch commented Sep 3, 2020

UART RTT devicetree & async support #27704

UART RTT devicetree & async support #27704

Conversation

JordanYates commented Aug 21, 2020

henrikbrixandersen commented Aug 21, 2020

JordanYates commented Aug 21, 2020 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrikbrixandersen commented Aug 28, 2020

JordanYates commented Aug 29, 2020

JordanYates commented Sep 2, 2020 • edited Loading

nordic-krch left a comment

Choose a reason for hiding this comment

JordanYates commented Sep 3, 2020

JordanYates commented Sep 3, 2020 • edited Loading

nordic-krch commented Sep 3, 2020

JordanYates commented Sep 3, 2020 • edited Loading

nordic-krch commented Sep 3, 2020

JordanYates commented Aug 21, 2020 •

edited

Loading

JordanYates commented Sep 2, 2020 •

edited

Loading

JordanYates commented Sep 3, 2020 •

edited

Loading

JordanYates commented Sep 3, 2020 •

edited

Loading