Fix NRF52840_DK UART driver and adapt FPGA test #12368

mprse · 2020-02-05T08:29:23Z

Summary of changes

The goal of this PR is to make tests-mbed_hal_fpga_ci_test_shield-uart passing on NRF52840_DK.

The test is failing because of the following reasons:

NRF52840_DK uart driver does not use pooling mode instead for all transfers DMA/Interrupt mode is used. According to the requirements, we assume that TxIrq interrupt is triggered when TXD register is empty (also after enabling TxIrq interrupt):

Lines 144 to 147 in f73a62a

    
            * * If `TxIrq` is enabled by ::serial_irq_set, ::serial_irq_handler will be invoked whenever 
        
            * Transmit Data Register Empty IRQ is generated. 
        
            * * If the interrupt condition holds true, when the interrupt is enabled with ::serial_irq_set, 
        
            * the ::serial_irq_handler is called instantly.

The driver fires interrupt when the whole DMA buffer is transmitted, but the interrupt is not fired after enabling the TxIrq interrupt when the TXD register is empty. To fix that we will trigger the interrupt manually.

Due to hardware limitations NRF52840_DK does not support odd parity bit and 2 stop bits. This PR will skip these test cases using !defined(TARGET_NRF52840) directive.
This should be temporary solutions since we need a fast fix. But in my opinion, we should consider adding something like uart_get_capabilities() function and use it to skip the unsupported test case instead of !defined(TARGET_NRF52840) directive. This requires discussion with @fkjagodzinski who is an author of the UART FPGA test and has wide knowledge about UART drivers and hardware limitations across different platforms.

Test results:

| target              | platform_name | test suite                              | result | elapsed_time (sec) | copy_method |
|---------------------|---------------|-----------------------------------------|--------|--------------------|-------------|
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal_fpga_ci_test_shield-uart | OK     | 32.06              | default     |
mbedgt: test suite results: 1 OK
mbedgt: test case report:
| target              | platform_name | test suite                              | test case                              | passed | failed | result | elapsed_time (sec) |
|---------------------|---------------|-----------------------------------------|----------------------------------------|--------|--------|--------|--------------------|
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal_fpga_ci_test_shield-uart | 115200, 8N1, FC off                    | 1      | 0      | OK     | 0.3                |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal_fpga_ci_test_shield-uart | 115200, 8N1, FC on                     | 1      | 0      | OK     | 0.35               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal_fpga_ci_test_shield-uart | 19200, 8N1, FC off                     | 1      | 0      | OK     | 0.33               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal_fpga_ci_test_shield-uart | 19200, 8N1, FC on                      | 1      | 0      | OK     | 0.37               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal_fpga_ci_test_shield-uart | 38400, 8N1, FC off                     | 1      | 0      | OK     | 0.32               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal_fpga_ci_test_shield-uart | 38400, 8N1, FC on                      | 1      | 0      | OK     | 0.35               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal_fpga_ci_test_shield-uart | 9600, 8E1, FC on                       | 1      | 0      | OK     | 0.4                |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal_fpga_ci_test_shield-uart | basic (direct init), 9600, 8N1, FC off | 1      | 0      | OK     | 0.38               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal_fpga_ci_test_shield-uart | basic (direct init), 9600, 8N1, FC on  | 1      | 0      | OK     | 0.42               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal_fpga_ci_test_shield-uart | basic, 9600, 8N1, FC off               | 1      | 0      | OK     | 0.36               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal_fpga_ci_test_shield-uart | basic, 9600, 8N1, FC on                | 1      | 0      | OK     | 0.39               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal_fpga_ci_test_shield-uart | init/free, FC off                      | 1      | 0      | OK     | 1.61               |
| NRF52840_DK-GCC_ARM | NRF52840_DK   | tests-mbed_hal_fpga_ci_test_shield-uart | init/free, FC on                       | 1      | 0      | OK     | 2.02               |

Impact of changes

Migration actions required

Documentation

Pull request type

[X] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[X] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Reviewers

@jamesbeyond @maciejbocianski @0xc0170 @fkjagodzinski

It is required by Mbed HAL API to generate TxIrq interrupt when TXD register is empty (also after enabling TxIrq interrupt): https://github.com/ARMmbed/mbed-os/blob/f73a62afbf4052b4da8c5b862ffb4708a80c1b6e/hal/serial_api.h#L144-L147 The driver uses DMA to perform uart transfer and TxIrq is generated after the transfer is finished. While enabling TxIrq we will check if TXD reg is empty and manually trigger the interrupt.

…0_DK target According to the documentation, `NRF52840_DK` does not support odd parity and 2 stop bits. Skip these test case using #if !defined(TARGET_NRF52840) directive. This is temporary solution. In the future we shell consider adding `uart_get_capabilities()` function. -

ciarmcom · 2020-02-05T10:00:44Z

@mprse, thank you for your changes.
@fkjagodzinski @maciejbocianski @jamesbeyond @0xc0170 @ARMmbed/mbed-os-hal @ARMmbed/mbed-os-test @ARMmbed/mbed-os-maintainers please review.

mprse · 2020-02-05T10:29:46Z

@0xc0170 Looks like something went wrong:

0.07s$ arm-none-eabi-gcc --version
The program 'arm-none-eabi-gcc' is currently not installed. To run 'arm-none-eabi-gcc' please ask your administrator to install the package 'gcc-arm-none-eabi'
The command "arm-none-eabi-gcc --version" failed and exited with 127 during .

0xc0170 · 2020-02-05T10:33:16Z

@mprse It is. I checked other tools jobs in Travis, they passed the same command. I restarted the subjob (if it does not help, will restart entire pipeline).

One PR after this one passed Travis, so this was a hiccup somewhere in the CI.

bulislaw · 2020-02-05T10:56:09Z

TESTS/mbed_hal_fpga_ci_test_shield/uart/main.cpp

@@ -338,7 +338,9 @@ Case cases[] = {
    Case("38400, 8N1, FC off", one_peripheral<UARTNoFCPort, DefaultFormFactor, fpga_uart_test_common_no_fc<38400, 8, ParityNone, 1, false> >),
    Case("115200, 8N1, FC off", one_peripheral<UARTNoFCPort, DefaultFormFactor, fpga_uart_test_common_no_fc<115200, 8, ParityNone, 1, false> >),
    // stop bits
+#if !defined(TARGET_NRF52840)


i really don't like this per board exceptions. I'd rather have a macro defined in the target definition, something like UART_WHATEVER_NOT_SUPPORTED. It's way clearer and also gives you a clear answer to "why".

Like I mentioned in the description I consider this as a temporary solution that should be replaced by get_capabilities() function when @fkjagodzinski is back.
But using UART_WHATEVER_NOT_SUPPORTED should be easy to change. I will do it later today.

maciejbocianski · 2020-02-06T10:40:07Z

targets/targets.json

@@ -8,7 +8,8 @@
        "components": [
            "PSA_SRV_IMPL",
            "PSA_SRV_EMUL",
-            "NSPE"
+            "NSPE",
+            "FPGA_CI_TEST_SHIELD"


@mprse adding this is intentional?

Nice catch. 👍

I will remove this. I added this to be able to run FPGA tests and also added this to commit by mistake.

Pull request has been modified.

0xc0170 · 2020-02-07T15:22:46Z

CI started

mbed-ci · 2020-02-07T17:57:21Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

fkjagodzinski

Thanks for this fix 👍

mergify · 2020-02-10T14:27:14Z

This PR does not contain release version label after merging.

mprse added 3 commits February 5, 2020 08:47

TARGET_NRF52/serial_api.c: Fix style

6a9b4bb

ciarmcom requested review from 0xc0170, fkjagodzinski, jamesbeyond, maciejbocianski and a team February 5, 2020 10:00

ciarmcom added the needs: review label Feb 5, 2020

bulislaw reviewed Feb 5, 2020

View reviewed changes

jamesbeyond previously approved these changes Feb 5, 2020

View reviewed changes

mergify bot added needs: CI and removed needs: review labels Feb 5, 2020

maciejbocianski reviewed Feb 6, 2020

View reviewed changes

Use dedicated macros to skip test cases instead target macro.

8fda5a4

mprse force-pushed the NRF_Serial_Fpga_fix branch from ecf9ce1 to 8fda5a4 Compare February 6, 2020 11:54

jamesbeyond self-requested a review February 7, 2020 16:30

jamesbeyond approved these changes Feb 7, 2020

View reviewed changes

mergify bot added ready for merge and removed needs: CI labels Feb 7, 2020

fkjagodzinski approved these changes Feb 10, 2020

View reviewed changes

0xc0170 merged commit a8e8723 into ARMmbed:master Feb 10, 2020

mergify bot added release version missing When PR does not contain release version, bot should label it and we fix it afterwards and removed ready for merge labels Feb 10, 2020

0xc0170 added release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 and removed release version missing When PR does not contain release version, bot should label it and we fix it afterwards labels Feb 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix NRF52840_DK UART driver and adapt FPGA test #12368

Fix NRF52840_DK UART driver and adapt FPGA test #12368

mprse commented Feb 5, 2020

ciarmcom commented Feb 5, 2020

mprse commented Feb 5, 2020

0xc0170 commented Feb 5, 2020 •

edited

Loading

bulislaw Feb 5, 2020

mprse Feb 5, 2020

mprse Feb 5, 2020

maciejbocianski Feb 6, 2020

mprse Feb 6, 2020

mprse Feb 6, 2020

0xc0170 commented Feb 7, 2020

mbed-ci commented Feb 7, 2020

fkjagodzinski left a comment

mergify bot commented Feb 10, 2020

	* * If `TxIrq` is enabled by ::serial_irq_set, ::serial_irq_handler will be invoked whenever
	* Transmit Data Register Empty IRQ is generated.
	* * If the interrupt condition holds true, when the interrupt is enabled with ::serial_irq_set,
	* the ::serial_irq_handler is called instantly.

Fix NRF52840_DK UART driver and adapt FPGA test #12368

Fix NRF52840_DK UART driver and adapt FPGA test #12368

Conversation

mprse commented Feb 5, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

ciarmcom commented Feb 5, 2020

mprse commented Feb 5, 2020

0xc0170 commented Feb 5, 2020 • edited Loading

bulislaw Feb 5, 2020

Choose a reason for hiding this comment

mprse Feb 5, 2020

Choose a reason for hiding this comment

mprse Feb 5, 2020

Choose a reason for hiding this comment

maciejbocianski Feb 6, 2020

Choose a reason for hiding this comment

mprse Feb 6, 2020

Choose a reason for hiding this comment

mprse Feb 6, 2020

Choose a reason for hiding this comment

0xc0170 commented Feb 7, 2020

mbed-ci commented Feb 7, 2020

Test run: SUCCESS

fkjagodzinski left a comment

Choose a reason for hiding this comment

mergify bot commented Feb 10, 2020

0xc0170 commented Feb 5, 2020 •

edited

Loading