LPC55S69: Fix UART & GPIO HAL to pass FPGA CI test shield tests #12342

fkjagodzinski · 2020-01-30T16:52:50Z

Summary of changes

Update the LPC55S69 HAL to pass the tests that make use of the FPGA CI test shield:

tests-mbed_hal_fpga_ci_test_shield-uart,
tests-mbed_hal_fpga_ci_test_shield-gpio.

Impact of changes

Migration actions required

Documentation

None

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

@ARMmbed/team-nxp, @jamesbeyond, @mprse, @maciejbocianski

ciarmcom · 2020-01-30T18:00:38Z

@fkjagodzinski, thank you for your changes.
@jamesbeyond @maclobdell @maciejbocianski @ARMmbed/mbed-os-maintainers please review.

bulislaw

@mmahadevan108 please review

bulislaw · 2020-02-03T11:34:27Z

targets/TARGET_NXP/TARGET_MCUXpresso_MCUS/TARGET_LPC55S69/TARGET_LPCXpresso/PeripheralPinMaps.h

@@ -19,6 +19,74 @@

 #include <mstd_cstddef>

+/************GPIO***************/
+MSTD_CONSTEXPR_OBJ_11 PinMap PinMap_GPIO[] = {


if that's a dummy for testing, should it only be compiled in while building tests?

This is only a header file and it looks like this is correct place to add pinmap, even dummy one for testing only.
Maybe we should add an extra comment like here:

mbed-os/targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_NRF52/TARGET_MCU_NRF52840/PeripheralPinMaps.h

Lines 49 to 50 in 02c5e08

// Pinmap used for testing only

MSTD_CONSTEXPR_OBJ_11 PinMap All_pins[] = {

I think this will also be changed for using const PinList *pinmap_restricted_gpio_pins() to exclude GPIO pins, instead of keeping this dummy list. just like what been discussed in the other PR #12380

mmahadevan108

@mmahadevan108 please review

I am awaiting a response for my review query to understand why the GPIO changes are needed

mmahadevan108 · 2020-01-30T19:33:41Z

targets/TARGET_NXP/TARGET_MCUXpresso_MCUS/TARGET_LPC55S69/TARGET_LPCXpresso/PeripheralNames.h

+typedef enum {
+    GPIO_X = 0, // dummy peripheral used instead of GPIO0 & GPIO1
+} GPIOName;
+


What is this for?

I believe that this is just dummy GPIO used in the PinMap_GPIO pin-map. We need GPIO pin-map to control the FPGA GPIO test and to skip testing the specific GPIOs which do not fulfill the defined behavior. As you can see some entries in the PinMap_GPIO are commented out and there is added a note why.

Thank you. Could you maybe update the comment stating this dummy is for FPGA GPIO testing so that its clear

Fixed as suggested

mprse · 2020-02-05T09:21:18Z

We need this PR asap to enable FPGA-test-shield support for LPC55S69.

@fkjagodzinski is off till the end of the week, so I tried to address review comments and responded to the provided questions.

Pull request has been modified.

0xc0170 · 2020-02-06T09:52:03Z

CI started

Also restarted Travis, should execute now

mbed-ci · 2020-02-06T11:14:49Z

Test run: SUCCESS

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

jamesbeyond · 2020-02-06T17:52:02Z

targets/TARGET_NXP/TARGET_MCUXpresso_MCUS/TARGET_LPC55S69/TARGET_LPCXpresso/PeripheralPinMaps.h

@@ -19,6 +19,74 @@

 #include <mstd_cstddef>

+/************GPIO***************/
+MSTD_CONSTEXPR_OBJ_11 PinMap PinMap_GPIO[] = {


I think this will also be changed for using const PinList *pinmap_restricted_gpio_pins() to exclude GPIO pins, instead of keeping this dummy list. just like what been discussed in the other PR #12380

Check that the RX or TX interrupt is enabled before calling a registered handler with RxIrq or TxIrq arg.

Pull request has been modified.

mprse · 2020-02-07T12:52:02Z

Provided the following changes:

dropped other commits than the fix for serial IRQ handling (STDIO uart is now restricted during FPGA testing by default),
added restricted GPIO pins for FPGA testing for LPC55S69.

Test results (this PR + PR #12379):

| target              | platform_name | test suite                              | result | elapsed_time (sec) | copy_method |
|---------------------|---------------|-----------------------------------------|--------|--------------------|-------------|
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-gpio | OK     | 30.08              | default     |
mbedgt: test suite results: 1 OK
mbedgt: test case report:
| target              | platform_name | test suite                              | test case                    | passed | failed | result | elapsed_time (sec) |
|---------------------|---------------|-----------------------------------------|------------------------------|--------|--------|--------|--------------------|
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-gpio | explicit init, input         | 1      | 0      | OK     | 1.26               |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-gpio | explicit init, output        | 1      | 0      | OK     | 1.25               |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-gpio | generic init, input & output | 1      | 0      | OK     | 1.26               |

| target              | platform_name | test suite                              | result | elapsed_time (sec) | copy_method |
|---------------------|---------------|-----------------------------------------|--------|--------------------|-------------|
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | OK     | 35.13              | default     |
mbedgt: test suite results: 1 OK
mbedgt: test case report:
| target              | platform_name | test suite                              | test case                              | passed | failed | result | elapsed_time (sec) |
|---------------------|---------------|-----------------------------------------|----------------------------------------|--------|--------|--------|--------------------|
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | 115200, 8N1, FC off                    | 1      | 0      | OK     | 0.35               |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | 115200, 8N1, FC on                     | 1      | 0      | OK     | 0.3                |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | 19200, 8N1, FC off                     | 1      | 0      | OK     | 0.37               |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | 19200, 8N1, FC on                      | 1      | 0      | OK     | 0.31               |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | 38400, 8N1, FC off                     | 1      | 0      | OK     | 0.35               |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | 38400, 8N1, FC on                      | 1      | 0      | OK     | 0.31               |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | 9600, 8E1, FC on                       | 1      | 0      | OK     | 0.31               |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | 9600, 8N2, FC off                      | 1      | 0      | OK     | 0.4                |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | 9600, 8N2, FC on                       | 1      | 0      | OK     | 0.32               |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | 9600, 8O1, FC on                       | 1      | 0      | OK     | 0.3                |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | basic (direct init), 9600, 8N1, FC off | 1      | 0      | OK     | 0.42               |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | basic (direct init), 9600, 8N1, FC on  | 1      | 0      | OK     | 0.34               |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | basic, 9600, 8N1, FC off               | 1      | 0      | OK     | 0.4                |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | basic, 9600, 8N1, FC on                | 1      | 0      | OK     | 0.31               |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | init/free, FC off                      | 1      | 0      | OK     | 0.33               |
| LPC55S69_NS-GCC_ARM | LPC55S69      | tests-mbed_hal_fpga_ci_test_shield-uart | init/free, FC on                       | 1      | 0      | OK     | 0.38               |
mbedgt: test case results: 16 OK

@0xc0170 This should be merged after PR #12379.

The second review round is required I think.

jamesbeyond · 2020-02-10T16:40:38Z

please tag this PR as alpha-2-release @adbridge

0xc0170 · 2020-02-11T09:09:57Z

CI restarted (also Travis)

mbed-ci · 2020-02-11T10:43:30Z

Test run: SUCCESS

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

jamesbeyond requested review from mmahadevan108 and mprse January 30, 2020 17:42

ciarmcom requested review from jamesbeyond, maciejbocianski, maclobdell and a team January 30, 2020 18:00

ciarmcom added the needs: review label Jan 30, 2020

bulislaw reviewed Feb 3, 2020

View reviewed changes

mmahadevan108 reviewed Feb 3, 2020

View reviewed changes

mprse approved these changes Feb 5, 2020

View reviewed changes

mmahadevan108 approved these changes 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

mprse force-pushed the fix-nxp-hal_fpga branch from dfb0901 to b6058ff Compare February 6, 2020 07:46

0xc0170 added the release-version: 6.0.0-alpha-2 Second pre-release version of 6.0.0 label Feb 6, 2020

0xc0170 previously approved these changes Feb 6, 2020

View reviewed changes

jamesbeyond previously requested changes Feb 6, 2020

View reviewed changes

mergify bot added needs: work and removed needs: CI labels Feb 6, 2020

Filip Jagodzinski and others added 2 commits February 7, 2020 13:32

LPC55S69: Fix serial IRQ handling

83b7b6d

Check that the RX or TX interrupt is enabled before calling a registered handler with RxIrq or TxIrq arg.

LPC55S69: Add restricted GPIO pins for FPGA testing

a0ff95b

mprse force-pushed the fix-nxp-hal_fpga branch from b6058ff to a0ff95b Compare February 7, 2020 12:33

jamesbeyond approved these changes Feb 10, 2020

View reviewed changes

0xc0170 approved these changes Feb 11, 2020

View reviewed changes

0xc0170 added needs: CI and removed needs: work labels Feb 11, 2020

mprse mentioned this pull request Feb 11, 2020

Add STDIO UART as restricted for FPGA testing for all targets and support for restricting GPIO #12379

Merged

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

0xc0170 merged commit a745525 into ARMmbed:master Feb 11, 2020

mergify bot removed the ready for merge label Feb 11, 2020

fkjagodzinski deleted the fix-nxp-hal_fpga branch February 11, 2020 11:39

	// Pinmap used for testing only
	MSTD_CONSTEXPR_OBJ_11 PinMap All_pins[] = {

LPC55S69: Fix UART & GPIO HAL to pass FPGA CI test shield tests #12342

LPC55S69: Fix UART & GPIO HAL to pass FPGA CI test shield tests #12342

Uh oh!

Conversation

fkjagodzinski commented Jan 30, 2020

Summary of changes

Impact of changes

Migration actions required

Documentation

Pull request type

Test results

Reviewers

Uh oh!

ciarmcom commented Jan 30, 2020

Uh oh!

bulislaw left a comment

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!

Choose a reason for hiding this comment

Uh oh!

mmahadevan108 left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprse commented Feb 5, 2020

Uh oh!

0xc0170 commented Feb 6, 2020

Uh oh!

mbed-ci commented Feb 6, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprse commented Feb 7, 2020

Uh oh!

jamesbeyond commented Feb 10, 2020

Uh oh!

0xc0170 commented Feb 11, 2020

Uh oh!

mbed-ci commented Feb 11, 2020

Uh oh!

Uh oh!