Fix hardware flow control on NRF52 series #8292

marcuschangarm · 2018-10-02T00:00:32Z

Description

Due to buggy flow control logic in the UARTE, the stop signal is not being set as it is supposed to when the the module is not ready to receive data.

This commit signals the sender to halt transmitting when a DMA buffer is full and only continue again when the atomic FIFO buffer has been emptied. This allows platforms with hardware flow control to minimize all buffers and rely on flow control instead.

Pull request type

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

marcuschangarm · 2018-10-02T00:01:36Z

@naveenkaje @TacoGrandeTX

naveenkaje · 2018-10-02T20:19:35Z

targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_NRF52/serial_api.c

@@ -331,6 +331,20 @@ static void nordic_nrf5_uart_timeout_handler(uint32_t instance)
        /* No activity detected, no timeout set. */
        nordic_nrf5_uart_state[instance].ticker_is_running = false;

+        /* Check if hardware flow control is set and signal sender to stop.


This block of code is repeated 3 times. Would it make sense to wrap this up in it's own function?

It's being called twice. It interrupt handler is convoluted as it is, I don't think adding another function will make it easier to read.

naveenkaje · 2018-10-02T20:41:42Z

Please provide reference to the bug in UARTE (such as errata from Nordic) / a Github issue? It will be useful.

marcuschangarm · 2018-10-02T21:47:58Z

The bug is an artifact of the UARTE API, because there is no safe way to flush the DMA buffer and swap it for a new one. When you follow the documentation and flush the DMA you'll get a ~6 ms period where the UARTE is not ready to receive. During this period the hardware flow control is inactive because the hardware flow control is only ready when the UARTE is ready to receive.

This PR manually toggles the flow control pin during this timeout window.

TacoGrandeTX · 2018-10-02T22:08:46Z

Sounds like a dicey problem! Do you have a test that showcases the problem and fix? DMA buffers are just 1 byte - right?

marcuschangarm · 2018-10-02T22:33:24Z

DMA buffers are at least 5 bytes. You can connect a logic analyzer to any application. You'll notice the RTS line is never set even though it should be.

deepikabhavnani · 2018-10-04T19:40:25Z

Build requested by @marcuschangarm
Query : Is it for 5.10.1 or 5.101.2?

/morph build

mbed-ci · 2018-10-04T23:16:10Z

Build : SUCCESS

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

Triggering tests

/morph test
/morph mbed2-build

0xc0170 · 2018-10-05T12:50:52Z

/morph export-build

mbed-ci · 2018-10-05T15:09:14Z

Exporter Build : SUCCESS

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

mbed-ci · 2018-10-05T15:53:40Z

Test : FAILURE

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

theotherjimmy · 2018-10-05T17:48:20Z

/morph test

NRF52_DK timed out.

theotherjimmy · 2018-10-05T17:50:16Z

@marcuschangarm Is there a chance that the NRF52 timeout could have been caused by this change?

mbed-ci · 2018-10-06T11:46:39Z

Test : FAILURE

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

NirSonnenschein · 2018-10-06T14:40:42Z

failures in tests for NRF52_DK timeouts and sync issues. this doesn't seem incidental. @marcuschangarm please take a look

marcuschangarm · 2018-10-08T20:57:14Z

/morph test

mbed-ci · 2018-10-08T22:55:49Z

Test : FAILURE

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

cmonr · 2018-10-09T02:34:53Z

/morph build

New commit > new SHA > new Build step needed.

mbed-ci · 2018-10-09T05:32:17Z

Build : SUCCESS

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

Triggering tests

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

NirSonnenschein · 2018-10-14T13:42:16Z

@marcuschangarm failures in test look PR related, please take a look.

marcuschangarm · 2018-10-16T05:33:12Z

@studavekar @NirSonnenschein
The failures occur in a test with heavy UART traffic. I suspect the CI infrastructure not following the flow control signal causes the Tx buffer to overflow.

studavekar · 2018-10-16T16:21:48Z

@studavekar @NirSonnenschein
The failures occur in a test with heavy UART traffic. I suspect the CI infrastructure not following the flow control signal causes the Tx buffer to overflow.

@marcuschangarm not sure if Daplink supports flow control from host <-->daplink<---- hw fw ctrl ---> device.

We also need to make review the test, we should avoid heavy uart traffic in tests. If we want to stress uart it should part of a separate test.

Failure :

[1539363423.79][CONN][RXD] :144::FAIL: Expected 'passed' Was 'pased' - passed --> pased

cmonr · 2018-10-16T16:48:00Z

@c1728p9

not sure if Daplink supports flow control from host <-->daplink<---- hw fw ctrl ---> device.

Thoughts?

c1728p9 · 2018-10-16T21:26:09Z

not sure if Daplink supports flow control from host <-->daplink<---- hw fw ctrl ---> device.

I took a look at the DAPLink code and flow control is enabled for the NRF52 boards.

marcuschangarm · 2018-10-17T06:44:13Z

@c1728p9
Does DAPLink support flow control on the host side as well?

c1728p9 · 2018-10-17T15:10:50Z

@marcuschangarm yep, it is built into the USB CDC protocol and can't be turned off.

marcuschangarm · 2018-10-17T22:48:48Z

@c1728p9
So if the CI enables flow control then it should work all the way down?

marcuschangarm · 2018-10-23T00:44:09Z

The failing test is for a component that is not supported by the NRF52_DK target.

Both the rtc_time and rtc_time_conv HAL tests are missing this header:

#if !DEVICE_RTC
#error [NOT_SUPPORTED] RTC API not supported for this target
#endif

0xc0170 · 2018-10-24T15:08:31Z

@mprse Can you review ^^ ?

Due to buggy flow control logic in the UARTE, the stop signal is not being set as it is supposed to when the the module is not ready to receive data. This commit signals the sender to halt transmitting when a DMA buffer is full and only continue again when the atomic FIFO buffer has been emptied. This allows platforms with hardware flow control to minimize all buffers and rely on flow control instead.

c1728p9 · 2018-10-24T20:58:57Z

So if the CI enables flow control then it should work all the way down?

Yep

mprse · 2018-10-25T06:35:49Z

Both the rtc_time and rtc_time_conv HAL tests are missing this header:

#if !DEVICE_RTC
#error [NOT_SUPPORTED] RTC API not supported for this target
#endif

These tests verifies time conversion functions from the following library which is available for all targets and does not require RTC. Also even when RTC is not supported on the specific platform it is then replaced by LPTICKER if available (which is NRF52_DK case I think):

mbed-os/platform/mbed_rtc_time.cpp

Lines 25 to 74 in 140f3e2

    
           #if DEVICE_RTC 
        
           static void (*_rtc_init)(void) = rtc_init; 
        
           static int (*_rtc_isenabled)(void) = rtc_isenabled; 
        
           static time_t (*_rtc_read)(void) = rtc_read; 
        
           static void (*_rtc_write)(time_t t) = rtc_write; 
        
           #elif DEVICE_LPTICKER 
        
           #include "drivers/LowPowerTimer.h" 
        
           #define US_PER_SEC 1000000 
        
           static SingletonPtr<mbed::LowPowerTimer> _rtc_lp_timer; 
        
           static uint64_t _rtc_lp_base; 
        
           static bool _rtc_enabled; 
        
           static void _rtc_lpticker_init(void) 
        
           { 
        
               _rtc_lp_timer->start(); 
        
               _rtc_enabled = true; 
        
           } 
        
           static int _rtc_lpticker_isenabled(void) 
        
           { 
        
               return (_rtc_enabled == true); 
        
           } 
        
           static time_t _rtc_lpticker_read(void) 
        
           { 
        
               return _rtc_lp_timer->read_high_resolution_us() / US_PER_SEC + _rtc_lp_base; 
        
           } 
        
           static void _rtc_lpticker_write(time_t t) 
        
           { 
        
               _rtc_lp_base = t; 
        
           } 
        
           static void (*_rtc_init)(void) = _rtc_lpticker_init; 
        
           static int (*_rtc_isenabled)(void) = _rtc_lpticker_isenabled; 
        
           static time_t (*_rtc_read)(void) = _rtc_lpticker_read; 
        
           static void (*_rtc_write)(time_t t) = _rtc_lpticker_write; 
        
           #else /* DEVICE_LPTICKER */ 
        
           static void (*_rtc_init)(void) = NULL; 
        
           static int (*_rtc_isenabled)(void) = NULL; 
        
           static time_t (*_rtc_read)(void) = NULL; 
        
           static void (*_rtc_write)(time_t t) = NULL; 
        
           #endif /* DEVICE_LPTICKER */

I can confirm that the failing test sends lots of data between host and device using uart. So this is a great test for this PR 😅 . Looking at the last results we have failure on IAR and ARM compilers.
For IAR there is a regular test timeout case, so we could consider increasing the test execution timeout.
For ARM we got some kind of communication error:
:144::FAIL: Expected 'passed' Was 'pased'
Looks like one byte has been lost during the transmission which is bad and may cause some unexpected issues in the future, so I think this needs to be investigated.

marcuschangarm · 2018-10-25T16:29:20Z

So, what's the plan?

Characters are missing, but the CI doesn't support flow control.
Tests are failing for components we don't claim support for.

Currently flow control is not supported by the CI and enabling it causes unwanted side effects.

marcuschangarm · 2018-10-25T19:27:07Z

/morph build

mbed-ci · 2018-10-25T20:17:00Z

Build : SUCCESS

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

Triggering tests

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

mbed-ci · 2018-10-26T07:59:12Z

Exporter Build : FAILURE

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

0xc0170 · 2018-10-26T10:43:31Z

Eclipse error, but all good there

/morph export-build

mbed-ci · 2018-10-26T11:15:16Z

Test : SUCCESS

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

mbed-ci · 2018-10-26T17:39:01Z

Exporter Build : SUCCESS

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

0xc0170 requested a review from TacoGrandeTX October 2, 2018 07:36

0xc0170 added the needs: review label Oct 2, 2018

naveenkaje reviewed Oct 2, 2018

View reviewed changes

naveenkaje approved these changes Oct 4, 2018

View reviewed changes

0xc0170 added the release-version: 5.10.1 label Oct 5, 2018

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

0xc0170 approved these changes Oct 5, 2018

View reviewed changes

NirSonnenschein added needs: work and removed needs: CI labels Oct 6, 2018

0xc0170 added release-version: 5.10.2 and removed release-version: 5.10.1 labels Oct 8, 2018

NirSonnenschein removed the needs: CI label Oct 14, 2018

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

Remove debug flow control from NRF52_DK and NRF52840_DK

ab04ff1

Currently flow control is not supported by the CI and enabling it causes unwanted side effects.

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

cmonr merged commit a5b7573 into ARMmbed:master Oct 26, 2018

cmonr removed the needs: CI label Oct 26, 2018

mprse mentioned this pull request Nov 9, 2018

tests-mbed_hal-rtc_time_conv randomly fails on CI #8691

Closed

marcuschangarm deleted the fix_flow branch November 14, 2018 22:41

Fix hardware flow control on NRF52 series #8292

Fix hardware flow control on NRF52 series #8292

Uh oh!

Conversation

marcuschangarm commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

marcuschangarm commented Oct 2, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naveenkaje commented Oct 2, 2018

Uh oh!

marcuschangarm commented Oct 2, 2018

Uh oh!

TacoGrandeTX commented Oct 2, 2018

Uh oh!

marcuschangarm commented Oct 2, 2018

Uh oh!

deepikabhavnani commented Oct 4, 2018

Uh oh!

mbed-ci commented Oct 4, 2018

Triggering tests

Uh oh!

0xc0170 commented Oct 5, 2018

Uh oh!

mbed-ci commented Oct 5, 2018

Uh oh!

mbed-ci commented Oct 5, 2018

Uh oh!

theotherjimmy commented Oct 5, 2018

Uh oh!

theotherjimmy commented Oct 5, 2018

Uh oh!

mbed-ci commented Oct 6, 2018

Uh oh!

NirSonnenschein commented Oct 6, 2018

Uh oh!

marcuschangarm commented Oct 8, 2018

Uh oh!

mbed-ci commented Oct 8, 2018

Uh oh!

cmonr commented Oct 9, 2018

Uh oh!

mbed-ci commented Oct 9, 2018

Triggering tests

Uh oh!

NirSonnenschein commented Oct 14, 2018

Uh oh!

marcuschangarm commented Oct 16, 2018

Uh oh!

studavekar commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmonr commented Oct 16, 2018

Uh oh!

c1728p9 commented Oct 16, 2018

Uh oh!

marcuschangarm commented Oct 17, 2018

Uh oh!

c1728p9 commented Oct 17, 2018

Uh oh!

marcuschangarm commented Oct 17, 2018

Uh oh!

marcuschangarm commented Oct 23, 2018

Uh oh!

0xc0170 commented Oct 24, 2018

Uh oh!

c1728p9 commented Oct 24, 2018

Uh oh!

mprse commented Oct 25, 2018

Uh oh!

marcuschangarm commented Oct 25, 2018

Uh oh!

marcuschangarm commented Oct 25, 2018

Uh oh!

marcuschangarm commented Oct 2, 2018 •

edited

Loading

studavekar commented Oct 16, 2018 •

edited

Loading