NRF52 serial: Fix UART console RX #8046

naveenkaje · 2018-09-09T20:59:19Z

While investigating the RX issue on NRF52_DK after SDK 14 updates,
it is observed that the RX FIFO doesn't get filled up, when the
flow control is disabled. Hence the readable never returns true.
If using Serial interface, the stdio file handles (0, 1, 2) get opened.
This results in configuring the flow control for STDIO, and it is observed
that the RX FIFO gets filled.

However, if RawSerial is used, the STDIO file handles
don't get opened. During the debug process it was observed that if the
flow control is configured once and then set to disabled, RX worked
as expected.

Alternative to this approach is that user application specifically
enables flow control as done in mbed's Greentea test suite. See https://goo.gl/r8nBYH

See https://goo.gl/8VB2qg step 14 for _initio's description.
See test code to reproduce the issue and test fix here: https://goo.gl/AQU1xG

Description

The change in behavior with NRF52's UART RX is documented here. #6891
This change is a fix for the above issue.

Pull request type

[ x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Breaking change

naveenkaje · 2018-09-09T21:05:48Z

This change is tested on NRF52_DK with SDK14 with GCC and ARM compilers.

naveenkaje · 2018-09-10T19:01:52Z

This behavior is seen only on NRF52_DK (PCA10040). The test program works correctly on NRF528340_DK (PCA10056).

kjbracey · 2018-09-11T11:48:34Z

This seems like a hack. We now have the mechanism for the target to specify that flow control should be used on the console UART and which pins (
#6603).

Is this mechanism not working, or are you not using it?

kjbracey · 2018-09-11T11:59:38Z

If the issue is that some programs are using RawSerial or Serial to access the console, without specifying flow control, then those programs are in error. This appears to be a good example of why programs shouldn't be doing that - it's not portable.

It assumes that the console is on a serial port at all, and usually assumes no flow control. And probably assumes stuff about baud rate and format too.

To some extent you can fudge serial settings by picking up the same macros that mbed_retarget.cpp uses to specify console pins and settings, which is what Greentea is doing, but still it's bad - you're double-initialising the serial port if it is a serial port, and failing totally if it isn't.

Programs should be using C stdout/stderr or POSIX FILENO_STDOUT/FILENO_STDERR to access the console. I have an open issue for Greentea relating to that: #6674, and I've just submitted a related PR for fault handling: #8076.

I'm not very keen on putting a hack into the HAL to ignore the settings it's told to use.

c1728p9 · 2018-09-17T16:46:20Z

targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_NRF52/serial_api.c

@@ -822,7 +822,14 @@ static void nordic_nrf5_uart_configure_object(serial_t *obj)
        nrf_uarte_hwfc_pins_set(nordic_nrf5_uart_register[uart_object->instance],
                                uart_object->rts,
                                uart_object->cts);
-    }
+    }  else { /* NRF_UART_HWFC_DISABLED */
+        /* Check if pin is set before configuring it */


Is this comment still relevant?

c1728p9 · 2018-09-17T16:58:35Z

targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_NRF52/serial_api.c

+        /* Check if pin is set before configuring it */
+			  uart_object->cts = NRF_UART_PSEL_DISCONNECTED;
+                          uart_object->rts = NRF_UART_PSEL_DISCONNECTED;
+			  nrf_uarte_hwfc_pins_set(nordic_nrf5_uart_register[uart_object->instance],


What is the purpose of this addition?

c1728p9 · 2018-09-17T17:01:01Z

targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_NRF52/serial_api.c

+		 * Even though we set the flow control once here, it gets reset subsequentially.
+		 * This seems necessary until a better fix is found as users can specify RawSerial
+		 * interface with no flow control, resulting in broken RX.
+		 */


So by default UART0 will use flow control connected to the terminal:

Flow control enabled by default

Flow control can be explicitly enabled or disabled by user by calling Serial::set_flow_control

If this is the case it seems reasonable to me.

c1728p9 · 2018-09-17T18:27:08Z

I'm not very keen on putting a hack into the HAL to ignore the settings it's told to use.

One possible alternative to changing the uart initialization is to on boot configure STDIO_UART_CTS as a gpio driven low for the nrf5x. That way DAPLink will see the NRF5X as ready to receive data when flow control is turned off. Enabling flow control explicitly (or via stdout or other use) will override the GPIO automatically.

kjbracey · 2018-09-18T08:52:01Z

I was under a misconception here, that Serial et al were totally enforcing default format at the C++ layer - 9600 baud, 8N1, no flow control.

In fact they only default to 9600 baud, while the flow and format are left unmodified from however serial_init sets them.

Given that, having the HAL preset the flow mode for that UART so that use of Serial without an explicit set_flow_control does default to flow control isn't actively fighting against the higher layers quite as much as I thought.

But the HAL does need to assume pins to do that, which is a cheat - activating 2 pins which weren't mentioned in the serial_init.

Maybe that's acceptable, but such magic should be more specific in the code - this isn't really a "NRF52" thing, it's specific to a board (isn't it?), and you want to activate it when configuring a UART against those particular pins you know are connected to the debug port on that particular board. There are a pile of board abstraction layers like pin_instance_uart underneath this function you're modifying, and you're effectively sidestepping them here. There needs to a bit more framework to it.

It feels like it would be a lot of complication to add magic so that code does what you think it means rather than what it says - making it so that

 Serial(A, B)

magically knows to also wiggle pins C and D without ever being told to do so because it makes that code work on a particular board.

I tend to agree with @marcuschangarm's abstraction comments on #6891. I'm not sure behind- the-scenes magic is the way to go here. Someone asking for Serial(A,B) is explicitly asking for 2-pin serial - if you are going to add 2 pins quietly, that could surprise folks, and even if it does make some insufficiently-abstracted programs work quietly, I can imagine people coming along later being confused about why it works - probably more confusing than why it currently doesn't work.

We already have mechanisms to abstract away from such an explicit serial request for the console. If you really need to explicitly reference the console by creating your own Serial-type object, we have console-specifying macros to configure the deeper settings as Greentea does. But ideally you shouldn't reference serial at all and just use stdin/stdout/STDIN_FILENO/STDOUT_FILENO, treating the console as device-independent.

(It may be we're still lacking a little in the abstraction area - I note you can find STDIN_FILENO which gives you most of the abilities via the POSIX API, but you can't get at the actual FileHandle object for the few things not available via the POSIX API, such as the sigio callback. I'm pondering exposing that)

kjbracey · 2018-09-18T08:54:55Z

One possible alternative to changing the uart initialization is to on boot configure STDIO_UART_CTS as a gpio driven low for the nrf5x. That way DAPLink will see the NRF5X as ready to receive data when flow control is turned off. Enabling flow control explicitly (or via stdout or other use) will override the GPIO automatically.

That's sounds like a good solution to me. Obviously it means you won't actually have working data flow control on a Serial if you don't specify it - you'll just get uncontrolled input and output - but then that's normal behaviour.

0xc0170 · 2018-09-24T12:18:53Z

@kjbracey-arm @c1728p9 Should this PR be updated to the proposed solution (CTS driven low for nrf5x ) ?

While investigating the RX issue on NRF52_DK after SDK 14 updates, it is observed that the RX FIFO doesn't get filled up, when the flow control is disabled. Hence the readable never returns true. If using Serial interface, the stdio file handles (0, 1, 2) get opened. This results in configuring the flow control for STDIO, and it is observed that the RX FIFO gets filled. However, if RawSerial is used, the STDIO file handles don't get opened. During the debug process it was observed that if the flow control is configured once and then set to disabled, RX worked as expected. Alternative to this approach is that user application specifically enables flow control as done in mbed's Greentea test suite. See https://goo.gl/r8nBYH See https://goo.gl/8VB2qg step 14 for _initio's description. See test code to reproduce the issue and test fix here: https://goo.gl/AQU1xG Description The change in behavior with NRF52's UART RX is documented here. ARMmbed#6891 This change is a fix for the above issue.

naveenkaje · 2018-10-02T19:30:47Z

Thanks @c1728p9 I made a modification to the fix in the latest patchset.
The issue is only seen on DAPLINK based boards. Here is the test results with this test code
https://goo.gl/AQU1xG

studavekar · 2018-10-09T20:23:35Z

cc @ARMmbed/mbed-os-maintainers @marcuschangarm do we need this change as well for #8292

naveenkaje · 2018-10-09T20:25:57Z

This fix is independent of #8292

marcuschangarm · 2018-10-09T22:17:26Z

targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_NRF52/reloc_vector_table.c

+	gpio_t rts;
+	gpio_init_out(&rts, STDIO_UART_RTS);
+	/* Set STDIO_UART_RTS as gpio driven low */
+	gpio_write(&rts, 0);


This call will trigger an assert for boards with STDIO_UART_RTS set to NC.

It would be better to wrap everything in an #if macro.

While investigating the RX issue on NRF52_DK after SDK 14 updates, it is observed that the RX FIFO doesn't get filled up, when the flow control is disabled. Hence the readable never returns true. If using Serial interface, the stdio file handles (0, 1, 2) get opened. This results in configuring the flow control for STDIO, and it is observed that the RX FIFO gets filled. However, if RawSerial is used, the STDIO file handles don't get opened. During the debug process it was observed that if the flow control is configured once and then set to disabled, RX worked as expected. Alternative to this approach is that user application specifically enables flow control as done in mbed's Greentea test suite. See https://goo.gl/r8nBYH See https://goo.gl/8VB2qg step 14 for _initio's description. See test code to reproduce the issue and test fix here: https://goo.gl/AQU1xG Description The change in behavior with NRF52's UART RX is documented here. ARMmbed#6891 This change is a fix for the above issue.

cmonr · 2018-10-15T20:01:15Z

/morph build

mbed-ci · 2018-10-15T23:01:04Z

Build : SUCCESS

Build number : 3361
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/8046/

Triggering tests

/morph test
/morph export-build
/morph mbed2-build

mbed-ci · 2018-10-16T00:44:07Z

Exporter Build : SUCCESS

Build number : 2996
Build artifacts/logs : http://mbed-os.s3-website-eu-west-1.amazonaws.com/?prefix=builds/exporter/8046/

cmonr · 2018-10-16T01:59:24Z

IAR network license issue.
/morph mbed2-build

mbed-ci · 2018-10-16T04:33:18Z

Test : SUCCESS

Build number : 3166
Test logs :http://mbed-os-logs.s3-website-us-west-1.amazonaws.com/?prefix=logs/8046/3166

The preprocessor based macro check #if evaluates all enums as 0 and hence the code does not get compiled. Since move this to a runtime check where the pin variable can be correctly evaluated. Delete mbed_overrides.c as it has a target specific mbed_sdk_init() to resolve linking problem. This is a follow on patch to: ARMmbed#8046

The preprocessor based macro check #if evaluates all enums as 0 and hence the code does not get compiled. Since move this to a runtime check where the pin variable can be correctly evaluated. Delete mbed_overrides.c as it has a target specific mbed_sdk_init() to resolve linking problem. This is a follow on patch to: #8046

adbridge added the needs: review label Sep 10, 2018

adbridge requested a review from a team September 10, 2018 11:16

naveenkaje force-pushed the UART_HWFC_Error_Fix_Upstream_Based branch from 023922b to 96eb87b Compare September 10, 2018 18:57

c1728p9 reviewed Sep 17, 2018

View reviewed changes

kjbracey mentioned this pull request Oct 2, 2018

Use Mbed config system to choose default flow control pins ARMmbed/esp8266-driver#99

Closed

naveenkaje force-pushed the UART_HWFC_Error_Fix_Upstream_Based branch from 96eb87b to 72abe51 Compare October 2, 2018 19:13

naveenkaje mentioned this pull request Oct 2, 2018

NRF52_DK Serial RX doesn't work #6891

Closed

c1728p9 approved these changes Oct 2, 2018

View reviewed changes

0xc0170 approved these changes Oct 9, 2018

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Oct 9, 2018

marcuschangarm reviewed Oct 9, 2018

View reviewed changes

0xc0170 added needs: work and removed needs: CI labels Oct 9, 2018

cmonr added needs: CI and removed needs: work labels Oct 15, 2018

cmonr added the release-version: 5.10.2 label Oct 16, 2018

0xc0170 added ready for merge and removed needs: CI labels Oct 16, 2018

cmonr merged commit 50e6de7 into ARMmbed:master Oct 16, 2018

cmonr removed the ready for merge label Oct 16, 2018

naveenkaje mentioned this pull request Oct 18, 2018

NRF52 : Fix UART RTS initialization #8474

Merged

cmonr added release-version: 5.10.3 and removed release-version: 5.10.2 labels Oct 18, 2018

NRF52 serial: Fix UART console RX #8046

NRF52 serial: Fix UART console RX #8046

Uh oh!

Conversation

naveenkaje commented Sep 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

naveenkaje commented Sep 9, 2018

Uh oh!

naveenkaje commented Sep 10, 2018

Uh oh!

kjbracey commented Sep 11, 2018

Uh oh!

kjbracey commented Sep 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

c1728p9 commented Sep 17, 2018

Uh oh!

kjbracey commented Sep 18, 2018

Uh oh!

kjbracey commented Sep 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Sep 24, 2018

Uh oh!

naveenkaje commented Oct 2, 2018

Uh oh!

studavekar commented Oct 9, 2018

Uh oh!

naveenkaje commented Oct 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cmonr commented Oct 15, 2018

Uh oh!

mbed-ci commented Oct 15, 2018

Triggering tests

Uh oh!

mbed-ci commented Oct 16, 2018

Uh oh!

cmonr commented Oct 16, 2018

Uh oh!

mbed-ci commented Oct 16, 2018

Uh oh!

Uh oh!

naveenkaje commented Sep 9, 2018 •

edited

Loading

kjbracey commented Sep 18, 2018 •

edited

Loading