Make FPGA tests to pass on CI targets (SPI, analogIn, PWM) #11682

mprse · 2019-10-14T11:16:56Z

Description

I run the FPGA tests on potential CI targets and got the following results:

	GPIO	GPIO_IRQ	AnalogIn	PWM	I2C	SPI	UART
NUCLEO_F429ZI	OK	OK	OK	OK	OK	OK	OK
K64F	OK	OK	OK	OK	OK	OK	OK
NUCLEO_F411RE	OK	OK	FAIL: D13 pin not supported	OK	FAIL	TIMEOUT: SPI async issue(lp-ticker ISR execution is too long)	OK
DISCO_L475VG_IOT01A	FAIL	OK	OK	OK	OK	OK	FAIL
NRF52840_DK	OK	FAIL	OK	FAIL: Driver bug/hw limitation	FAIL	TIMEOUT: 16 bit symbol not supported	TIMEOUT
NUCLEO_F303RE	OK	OK	FAIL: D13 pin not supported	OK	OK	OK	TIMEOUT
LPC55S69_NS	FAIL	FAIL	OK	NOT SUPPORTED	FAIL	OK	TIMEOUT
NUCLEO_L073RZ	OK	OK	FAIL: D13 pin not supported	OK	OK	OK	OK

This PR should solve the issues with SPI, AnalogIn, PWM tests.

@jeromecoutant @LMESTM
I noticed the problem with the SPI async mode on NUCLEO_F411RE. It looks like the system tick breaks the transmission. Can you take a look? More details in the commit description.
Also, it looks like PA_5 pin can not be used as analogin. I removed it from the pinmap table. Is this an acceptable solution?

Pull request type

[X] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@maciejbocianski @fkjagodzinski
@0xc0170 @jamesbeyond

ciarmcom · 2019-10-14T13:00:33Z

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

jeromecoutant

Maybe you should separate TESTS update from targets update ?

jeromecoutant · 2019-10-14T13:04:29Z

targets/targets.json

@@ -2622,6 +2622,9 @@
            "FLASH",
            "MPU"
        ],
+        "device_has_remove": [
+            "SPI_ASYNCH"


We don't see any reason to remove SPI_ASYNC for this specific target...

The main reason is that async mode does not work on this target (test case fails).
The best option would be to find why. I will try to do further investigation, but I wanted first to check if maybe you know something about this.

jeromecoutant · 2019-10-14T13:06:55Z

targets/TARGET_STM/TARGET_STM32F3/TARGET_STM32F303xE/TARGET_NUCLEO_F303RE/PeripheralPins.c

@@ -58,7 +58,7 @@ MBED_WEAK const PinMap PinMap_ADC[] = {
 //  {PA_2,       ADC_1, STM_PIN_DATA_EXT(STM_MODE_ANALOG, GPIO_NOPULL, 0, 3, 0)}, // ADC1_IN3 // Connected to STDIO_UART_TX
 //  {PA_3,       ADC_1, STM_PIN_DATA_EXT(STM_MODE_ANALOG, GPIO_NOPULL, 0, 4, 0)}, // ADC1_IN4 // Connected to STDIO_UART_RX
    {PA_4,       ADC_2, STM_PIN_DATA_EXT(STM_MODE_ANALOG, GPIO_NOPULL, 0, 1, 0)}, // ADC2_IN1
-    {PA_5,       ADC_2, STM_PIN_DATA_EXT(STM_MODE_ANALOG, GPIO_NOPULL, 0, 2, 0)}, // ADC2_IN2 // Connected to LD2 [Green Led]
+//    {PA_5,       ADC_2, STM_PIN_DATA_EXT(STM_MODE_ANALOG, GPIO_NOPULL, 0, 2, 0)}, // ADC2_IN2 // Connected to LD2 [Green Led]


Maybe pin should be added to the restricted list ?
To be tested

I was thinking about this. But it looks like this pin can not be used as Analogin pin (the results are incorrect), so it should be removed from the pinmap table.

c1728p9 · 2019-10-15T15:49:20Z

TESTS/mbed_hal_fpga_ci_test_shield/spi/main.cpp

@@ -177,9 +177,9 @@ Case cases[] = {
    Case("SPI - mode testing (MODE_1)", one_peripheral<SPIPort, DefaultFormFactor, spi_test_common<SPITester::Mode1, 8, TRANSFER_SPI_MASTER_WRITE_SYNC, FREQ_1_MHZ> >),
    Case("SPI - mode testing (MODE_2)", one_peripheral<SPIPort, DefaultFormFactor, spi_test_common<SPITester::Mode2, 8, TRANSFER_SPI_MASTER_WRITE_SYNC, FREQ_1_MHZ> >),
    Case("SPI - mode testing (MODE_3)", one_peripheral<SPIPort, DefaultFormFactor, spi_test_common<SPITester::Mode3, 8, TRANSFER_SPI_MASTER_WRITE_SYNC, FREQ_1_MHZ> >),
-
+#if !defined(TARGET_NRF52840_DK)


Why is this test skipped just for the NRF52840? Is this test tied to a defined behavior in the SPI HAL specification? If so users and library may be relying on all targets having this behavior. If this isn't part of the HAL specification then maybe this could be an optional test? What do you think?

Thanks for the review!
16-bit symbols are not supported on NRF target. On master, specification is not very restrictive since it must be consistent with all targets. We also don't have capabilities like on the SPI feature branch, so I can't run this case conditionally.

I see two options:

remove the test case permanently, but it would be good to run this test on targets that support 16-bit symbols.

use the preprocessor to disable the test case on the specific target.

I don't like the first option. Let's consider that we are adding the next target which supports only one clock mode. If we choose this approach, then I would have to remove test cases for other modes.

Also on the feature branch, we have nice specification and tests, but it looks like there are no plans to bring new SPI into the master in the nearest future 😞 Maybe it would be good to add spi_get_capabilities() function on the master.

Also on the feature branch, we have nice specification and tests, but it looks like there are no plans to bring new SPI into the master in the nearest future 😞 Maybe it would be good to add spi_get_capabilities() function on the master.

How much work it would be?

We could add WEAK definition for all targets which returns that all SPI capabilities are supported. And then implement a valid version for CI targets and disable unsupported capabilities for each CI target. This way we could also extend the SPI test on the master. I think that this should be easy to implement.

I've realized just now this has not yet been fixed?

Not at the moment. This must be discussed. I'll try to write an email about this. The same problem we have already here:

mbed-os/TESTS/mbed_hal_fpga_ci_test_shield/gpio/main.cpp

Lines 67 to 72 in df79609

// Some targets don't support input pull mode.

#if !defined(TARGET_NANO100) && \

!defined(TARGET_NUC472) && \

!defined(TARGET_M451)

// Test GPIO used as an input.

gpio_dir(&gpio, PIN_INPUT);

ABOSTM · 2019-10-18T15:23:01Z

Hi @mprse ,
I analysed issue with SPI async mode and there is an issue with MBED_TICKLESS
See details in PR #11713
PR #11713 removes MBED_TICKLESS from STM32F4 boards.
With this PR you can revert your commit : "NUCLEO_F411RE: disable spi asynch mode support"

mprse · 2019-10-21T09:11:36Z

@ABOSTM 👍 Thank you for checking this.

I added a related comment in PR #11713 : #11713 (comment).

mprse · 2019-10-23T07:11:59Z

Dropped commit which disables SPI async mode for STM F4. SPI async mode fails because of too long execution of lp-ticker ISR. Verified that PR #11713 fixes the problem.

mprse · 2019-10-28T14:43:57Z

Please review @maciejbocianski @0xc0170 @jamesbeyond .

0xc0170

Just one cosmetic change needed

0xc0170 · 2019-10-28T14:49:07Z

targets/TARGET_NORDIC/TARGET_NRF5x/TARGET_NRF52/pwmout_api.c

@@ -295,9 +295,9 @@ void pwmout_period_us(pwmout_t *obj, int period)
 void pwmout_pulsewidth(pwmout_t *obj, float pulse)
 {
    DEBUG_PRINTF("pwmout_pulsewidt: %f\r\n", pulse);
-
+    


added some spaces?

0xc0170 · 2019-10-28T15:05:51Z

Started first CI run.

I also left a comment, what can we do about the nrf target skip test, would not rather to have it there.

mbed-ci · 2019-10-28T21:12:57Z

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_build-ARM
jenkins-ci/mbed-os-ci_build-GCC_ARM
jenkins-ci/mbed-os-ci_build-IAR

mprse · 2019-10-29T07:08:00Z

Test run: FAILED

Not sure but it looks like some CI issue. Few boards in Jenkins are marked with an exclamation mark with info passed in 0 s, but marked as unstable.

…5) pin. Analogin test fails on D13(PA_5) pin. When logic one (3.3V) is provided on this pin ADC reads 0.86 value. On other pins we got 0.98. This is caused because this pin is connected to led2.

Fix pwm nrf

Looks like the PWM works fine on NRF52840_DK target, but this target has hardware limitation and the max PWM period is ‭32 767‬ us. That is why test cases when the period is set 50 ms are failing. The test will be modified to test the 30 ms period instead of 50 ms, so the test can pass on all CI targets.

mprse · 2019-10-30T14:11:06Z

Rebased to solve conflicts. Ready for CI (last time some problems were encountered during CI).

Add also default weak version of spi_get_capabilities() which provides default/most common SPI parameters. This function can be replaced if a specific target has different capabilities.

… unsupported features

mprse · 2019-11-06T09:33:52Z

I also left a comment, what can we do about the nrf target skip test, would not rather to have it there.

@0xc0170 I added spi_get_capabilities() to SPI HAL API and used it in the SPI FPGA test to skip unsupported test cases (solution for NRF target). This will also allow us to extend the test. This will be done in the separate PR.

Can we start CI?

jamesbeyond

Looks good, well done !

mprse · 2019-11-07T08:47:28Z

@0xc0170 Can we start CI?

0xc0170 · 2019-11-07T08:48:35Z

CI started

mbed-ci · 2019-11-07T10:31:11Z

Test run: SUCCESS

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

0xc0170 · 2019-11-07T10:45:47Z

Set this to 6.0 (will be checked again and might get earlier in the feature release).

ciarmcom requested review from 0xc0170, fkjagodzinski, jamesbeyond, maciejbocianski and a team October 14, 2019 13:00

ciarmcom added the needs: review label Oct 14, 2019

jeromecoutant suggested changes Oct 14, 2019

View reviewed changes

c1728p9 reviewed Oct 15, 2019

View reviewed changes

fkjagodzinski approved these changes Oct 16, 2019

View reviewed changes

jeromecoutant mentioned this pull request Oct 17, 2019

Enable tickless mode in additional tested targets #11480

Merged

mprse force-pushed the fpga_tests_CI_targets branch from f520ebc to 164286c Compare October 23, 2019 06:53

0xc0170 requested a review from jeromecoutant October 23, 2019 08:26

0xc0170 approved these changes Oct 28, 2019

View reviewed changes

0xc0170 added the release-version: 5.14.2 label Oct 28, 2019

mprse force-pushed the fpga_tests_CI_targets branch from 164286c to 48fa594 Compare October 28, 2019 15:02

mprse added 4 commits October 30, 2019 14:34

NUCLEO_F411RE, NUCLEO_L073RZ, NUCLEO_F303RE: Disable Analogin D13(PA_…

ee519e6

…5) pin. Analogin test fails on D13(PA_5) pin. When logic one (3.3V) is provided on this pin ADC reads 0.86 value. On other pins we got 0.98. This is caused because this pin is connected to led2.

SPI fpga test: disable 16 bit test case for NRF52840_DK (not supported)

95dc61d

NRF52: Fix PWM driver

c83307e

Fix pwm nrf

mprse force-pushed the fpga_tests_CI_targets branch from 48fa594 to 766c364 Compare October 30, 2019 14:02

mprse added 2 commits November 6, 2019 09:48

Add spi_get_capabilities() function to HAL API

4b1b4f7

Add also default weak version of spi_get_capabilities() which provides default/most common SPI parameters. This function can be replaced if a specific target has different capabilities.

SPI fpga test: use get_capabilities() function to skip test cases for…

b24afed

… unsupported features

jamesbeyond approved these changes Nov 6, 2019

View reviewed changes

0xc0170 added needs: CI and removed needs: review labels Nov 7, 2019

0xc0170 added ready for merge release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed needs: CI release-version: 5.14.2 labels Nov 7, 2019

0xc0170 merged commit 33e392e into ARMmbed:master Nov 7, 2019

0xc0170 removed the ready for merge label Nov 7, 2019

LMESTM mentioned this pull request Nov 13, 2019

UBLOX_ODIN_EVK_W2 crashing in Wifi tests after tickless change #11557

Closed

0xc0170 added release-version: 5.15.0-rc1 and removed release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Nov 19, 2019

jamesbeyond mentioned this pull request Jan 29, 2020

FIX: Disable Analogin D13(PA_5) on some NUCLEO targets #12332

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make FPGA tests to pass on CI targets (SPI, analogIn, PWM) #11682

Make FPGA tests to pass on CI targets (SPI, analogIn, PWM) #11682

mprse commented Oct 14, 2019 •

edited

Loading

ciarmcom commented Oct 14, 2019

jeromecoutant left a comment

jeromecoutant Oct 14, 2019

mprse Oct 14, 2019

jeromecoutant Oct 14, 2019

jeromecoutant Oct 14, 2019

mprse Oct 14, 2019

c1728p9 Oct 15, 2019

mprse Oct 16, 2019

0xc0170 Oct 21, 2019

mprse Oct 21, 2019

0xc0170 Oct 28, 2019

mprse Oct 29, 2019

ABOSTM commented Oct 18, 2019

mprse commented Oct 21, 2019 •

edited

Loading

mprse commented Oct 23, 2019

mprse commented Oct 28, 2019

0xc0170 left a comment

0xc0170 Oct 28, 2019

mprse Oct 28, 2019

0xc0170 commented Oct 28, 2019

mbed-ci commented Oct 28, 2019

mprse commented Oct 29, 2019

mprse commented Oct 30, 2019

mprse commented Nov 6, 2019

jamesbeyond left a comment

mprse commented Nov 7, 2019

0xc0170 commented Nov 7, 2019

mbed-ci commented Nov 7, 2019

0xc0170 commented Nov 7, 2019

	// Some targets don't support input pull mode.
	#if !defined(TARGET_NANO100) && \
	!defined(TARGET_NUC472) && \
	!defined(TARGET_M451)
	// Test GPIO used as an input.
	gpio_dir(&gpio, PIN_INPUT);

Make FPGA tests to pass on CI targets (SPI, analogIn, PWM) #11682

Make FPGA tests to pass on CI targets (SPI, analogIn, PWM) #11682

Conversation

mprse commented Oct 14, 2019 • edited Loading

Description

Pull request type

Reviewers

ciarmcom commented Oct 14, 2019

jeromecoutant left a 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

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

ABOSTM commented Oct 18, 2019

mprse commented Oct 21, 2019 • edited Loading

mprse commented Oct 23, 2019

mprse commented Oct 28, 2019

0xc0170 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xc0170 commented Oct 28, 2019

mbed-ci commented Oct 28, 2019

mprse commented Oct 29, 2019

mprse commented Oct 30, 2019

mprse commented Nov 6, 2019

jamesbeyond left a comment

Choose a reason for hiding this comment

mprse commented Nov 7, 2019

0xc0170 commented Nov 7, 2019

mbed-ci commented Nov 7, 2019

0xc0170 commented Nov 7, 2019

mprse commented Oct 14, 2019 •

edited

Loading

mprse commented Oct 21, 2019 •

edited

Loading