Bring FPGA-Test-Shield tests into Mbed-os master. #10820

mprse · 2019-06-13T09:27:53Z

Description

Bring FPGA-Test-Shield tests to Mbed-os.

When this is merged these tests can be removed from ci-tes-shield-fpga repository.

By default, all these tests will be skipped on CI. To enable these tests FPGA_CI_TEST_SHIELD component must be added in targets.json configuration file.

Related PRs: PR #10816

Pull request type

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

Reviewers

@maciejbocianski
@fkjagodzinski
@c1728p9
@jamesbeyond

ciarmcom · 2019-06-13T11:00:39Z

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

adbridge · 2019-06-20T15:31:27Z

@maciejbocianski @jamesbeyond could you review please ?

jamesbeyond

These tests are HAL tests, shall we add the header files for the test along with doxygen comments. (same as rest of the hal tests)
@mprse @maciejbocianski @fkjagodzinski @c1728p9

c1728p9 · 2019-06-28T19:18:21Z

@jamesbeyond it would be good to have header files with doxygen comments for the APIs which have a HAL specification. This should map the defined behavior to tests which validates it.

c1728p9 · 2019-06-28T19:19:12Z

TESTS/mbed_hal_fpga_ci_test_shield/i2c/main.cpp

+#error [NOT_SUPPORTED] I2C not supported for this target
+#elif !COMPONENT_FPGA_CI_TEST_SHIELD
+#error [NOT_SUPPORTED] FPGA CI Test Shield is needed to run this test
+#elif !defined(FULL_TEST_SHIELD)


FULL_TEST_SHIELD should probably be removed since this only applies to the Basys3.

mprse · 2019-06-29T19:18:51Z

These tests are HAL tests, shall we add the header files for the test along with doxygen comments. (same as rest of the hal tests)

We missed that while working on FPGA tests. I agree that headers should be added. I will add when I come back to the office.

Divide one test case to four test cases to increase readability. Adapted to the last version of the FPGA CI Test Shield API.

mprse · 2019-07-03T08:32:30Z

Pushed last changes provided by @maciejbocianski to FPGA I2C test (261e67b).

Removed FULL_TEST_SHIELD from FPGA analogin test(5ffa260).

mprse · 2019-07-03T08:42:54Z

At the moment we do not have HAL specification for the tested APIs here. We can add headers when HAL specification is ready and we need headers to map the test cases to the defined behavior.

I think we should move forward with this PR. It is hard now to keep consistency between FPGA repo and this one. Even if some dummy headers are required we can add this in the additional PR.

0xc0170

Before I approve, just one comment about dead code below.

Where is the documentation for fpga testing (reference for docs PR?), plus what these are testing - I am seeing just test files here and defined test cases but where is the test plan (or something similar) or is all suficient what we have in Cases() in the files?

0xc0170 · 2019-07-03T09:31:26Z

TESTS/mbed_hal_fpga_ci_test_shield/analogout/main.cpp

+    // printf("Sum: %llu\r\n", sum);
+    // printf("Num power samples: %d\r\n", samples);
+    // printf("Num power cycles: %llu\r\n", cycles);
+    // printf("ANIN0 voltage: %.6fV\r\n", tester.get_anin_voltage(0));


Can we remove these lines not used?

I think that this code has been added for debugging. Maybe I can wrap this code around #if DEBUG directive?

0xc0170 · 2019-07-03T09:34:51Z

Is this OK to go to 5.13.2 (in 2 weeks time) , landing to master once it's ready?

mprse · 2019-07-03T10:05:56Z

Before I approve, just one comment about dead code below.

Where is the documentation for fpga testing (reference for docs PR?), plus what these are testing - I am seeing just test files here and defined test cases but where is the test plan (or something similar) or is all suficient what we have in Cases() in the files?

It looks like we do not have HAL specification ready for the tested APIs. I would consider these tests as general tests to show the capabilities of the FPGA Test Shields for the SiP workshop.
The plan is to add dummy test headers with given/when/then description in the separate PR.

Header file and mapping between test cases and defined behavior will be added to this PR:
#10816

mprse · 2019-07-03T10:07:07Z

Is this OK to go to 5.13.2 (in 2 weeks time) , landing to master once it's ready?

cc @jamesbeyond
I think we need this for the next week...

jamesbeyond · 2019-07-03T10:24:43Z

Hi @0xc0170, this PR is for workshop next week, please mark this as high importance.

… tolerance Keep "AnalogIn - full test" disabled due to hardware issue in the first rev of FPGA Test Shield.

mprse · 2019-07-03T10:36:48Z

@0xc0170 Ready for CI.

jamesbeyond

After a offline conversation, I'd agree that because we don't have defined behavior for current these HAL APIs, the test header files can come to the repo as separate PR

mprse · 2019-07-04T06:52:01Z

This one does not need preceding PR. It is ready for CI.

0xc0170 · 2019-07-04T11:05:57Z

This one does not need preceding PR. It is ready for CI.

I am reviewing at the moment

0xc0170 · 2019-07-04T11:08:27Z

TESTS/mbed_hal_fpga_ci_test_shield/uart/main.cpp

+    Case("115200, 8N1, FC on", one_peripheral<UARTPort, DefaultFormFactor, test_common<115200, 8, ParityNone, 1> >),
+    Case("115200, 8N1, FC off", one_peripheral<UARTNoFCPort, DefaultFormFactor, test_common_no_fc<115200, 8, ParityNone, 1> >), // DISCO_L475VG_IOT01A
+    // data bits
+    /* FIXME not supported: K64F


do we need to keep this dead code here? Can it be tracked separately (removed from here, stashed somewhere else with the tracking issue for this)

@fkjagodzinski Can you check this?

Ok, so I checked this part -- data bits on K64F.

Our HAL allows setting data_bits and does not restrict this value in any way:

mbed-os/hal/serial_api.h

Lines 137 to 144 in b55344c

/** Configure the format. Set the number of bits, parity and the number of stop bits

*

* @param obj The serial object

* @param data_bits The number of data bits

* @param parity The parity

* @param stop_bits The number of stop bits

*/

void serial_format(serial_t *obj, int data_bits, SerialParity parity, int stop_bits);

The HAL implementation for K64F does not really use data_bits param:

mbed-os/targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/serial_api.c

Line 102 in b55344c

void serial_format(serial_t *obj, int data_bits, SerialParity parity, int stop_bits)

... and this is because the fsl_uart.c does not have any data_bits-related config item:

mbed-os/targets/TARGET_Freescale/TARGET_MCUXpresso_MCUS/TARGET_MCU_K64F/drivers/fsl_uart.h

Lines 167 to 181 in b55344c

/*! @brief UART configuration structure. */

typedef struct _uart_config

{

uint32_t baudRate_Bps; /*!< UART baud rate */

uart_parity_mode_t parityMode; /*!< Parity mode, disabled (default), even, odd */

#if defined(FSL_FEATURE_UART_HAS_STOP_BIT_CONFIG_SUPPORT) && FSL_FEATURE_UART_HAS_STOP_BIT_CONFIG_SUPPORT

uart_stop_bit_count_t stopBitCount; /*!< Number of stop bits, 1 stop bit (default) or 2 stop bits */

#endif

#if defined(FSL_FEATURE_UART_HAS_FIFO) && FSL_FEATURE_UART_HAS_FIFO

uint8_t txFifoWatermark; /*!< TX FIFO watermark */

uint8_t rxFifoWatermark; /*!< RX FIFO watermark */

#endif

bool enableTx; /*!< Enable TX */

bool enableRx; /*!< Enable RX */

} uart_config_t;

However, the K64 Sub-Family Reference Manual says there are two settings:

Programmable 8-bit or 9-bit data format

Initially I thought we should uncomment this block of code and expect NXP to provide a fix. Now, knowing NXP would have to update their HAL, I don't think it would be reasonable to do so. Also, I'm not so sure if Mbed OS should require support of 9-bit UART data at all.

@jamesbeyond, what do you think about this? Should I remove the data_bits part of this test?
BTW, there are two more similar FIXME comments regarding ST targets. Remove these as well? (don't know why they fail yet).

I vote for yes. dead code should be removed (tracking issue to have this, with commit including the changes needed). It will be easy to bring this back once it is fixed

Fixed (d2d3d5f)

0xc0170 · 2019-07-04T11:09:00Z

TESTS/mbed_hal_fpga_ci_test_shield/analogin/main.cpp

+    Case("AnalogIn - init test", all_ports<AnaloginPort, DefaultFormFactor, analogin_init>),
+    // This test case is disabled for now due to hardware issue in the first rev of FPGA Test Shield
+#if 0
+    Case("AnalogIn - full test", all_ports<AnaloginPort, DefaultFormFactor, analogin_full_test>),


remove as well this one not used

This one will be enabled as soon as we get the rev.2 of the FPGA-Test-Shield and I will be able to rework and execute this test case.

It should come with rv2 (we should not have if 0 in the code)

Ok, I will remove this test case for now and add it later.

fixed (879d891)

0xc0170 · 2019-07-04T11:09:53Z

TESTS/mbed_hal_fpga_ci_test_shield/pwm/main.cpp

+#include "test_utils.h"
+
+
+#define pwm_debug_printf(...)


This is macro for debugging. By default, it does nothing.

OK, I was expecting if debug , it does something

The test case which checks full range cannot be executed at the moment due to a hardware bug in FPGA-Test-Shield. This test case will be restored when the final version of FPGA-Test-Shield is ready.

0xc0170 · 2019-07-04T12:42:24Z

CI started meanwhile

Not all uart features are supported by specific platforms.

0xc0170 · 2019-07-04T13:15:25Z

Restarted CI after latest changes

mbed-ci · 2019-07-04T13:16:07Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_greentea-test

mbed-ci · 2019-07-04T13:37:38Z

Test run: SUCCESS

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

ciarmcom requested review from c1728p9, fkjagodzinski, jamesbeyond, maciejbocianski and a team June 13, 2019 11:00

ciarmcom added the needs: review label Jun 13, 2019

Bring FPGA-Test-Shield tests into Mbed-os master.

cd0ad51

mprse force-pushed the bring_fpga_tests_master branch from 9883ce7 to cd0ad51 Compare June 13, 2019 11:43

mprse mentioned this pull request Jun 19, 2019

Start FPGA getting started guide in target.md ARMmbed/mbed-os-5-docs#1083

Merged

jamesbeyond reviewed Jun 27, 2019

View reviewed changes

c1728p9 reviewed Jun 28, 2019

View reviewed changes

0xc0170 added needs: work and removed needs: review labels Jul 2, 2019

FPGA I2C test update.

261e67b

Divide one test case to four test cases to increase readability. Adapted to the last version of the FPGA CI Test Shield API.

maciejbocianski approved these changes Jul 3, 2019

View reviewed changes

0xc0170 reviewed Jul 3, 2019

View reviewed changes

FPGA Analogin test: Remove usage of FULL_TEST_SHIELD symbol, increase…

8524560

… tolerance Keep "AnalogIn - full test" disabled due to hardware issue in the first rev of FPGA Test Shield.

mprse force-pushed the bring_fpga_tests_master branch from 5ffa260 to c6acc85 Compare July 3, 2019 10:33

mprse force-pushed the bring_fpga_tests_master branch from c6acc85 to c016f2a Compare July 3, 2019 10:34

FPGA Analogout test: mark dead code as debug code, increase tolerance

c016f2a

jamesbeyond approved these changes Jul 3, 2019

View reviewed changes

0xc0170 added release-version: 5.13.2 needs: preceding PR labels Jul 3, 2019

0xc0170 removed needs: preceding PR needs: work labels Jul 4, 2019

0xc0170 requested changes Jul 4, 2019

View reviewed changes

FPGA Analogin test: Remove test case which can not be executed

879d891

The test case which checks full range cannot be executed at the moment due to a hardware bug in FPGA-Test-Shield. This test case will be restored when the final version of FPGA-Test-Shield is ready.

FPGA uart test: Remove unsupported test cases

d2d3d5f

Not all uart features are supported by specific platforms.

0xc0170 approved these changes Jul 4, 2019

View reviewed changes

fkjagodzinski approved these changes Jul 4, 2019

View reviewed changes

0xc0170 added the ready for merge label Jul 4, 2019

0xc0170 merged commit c709955 into ARMmbed:master Jul 4, 2019

0xc0170 removed the ready for merge label Jul 4, 2019

adbridge added release-version: 5.13.1 and removed release-version: 5.13.2 labels Jul 5, 2019

	/** Configure the format. Set the number of bits, parity and the number of stop bits
	*
	* @param obj The serial object
	* @param data_bits The number of data bits
	* @param parity The parity
	* @param stop_bits The number of stop bits
	*/
	void serial_format(serial_t *obj, int data_bits, SerialParity parity, int stop_bits);

	/! @brief UART configuration structure. /
	typedef struct _uart_config
	{
	uint32_t baudRate_Bps; /!< UART baud rate /
	uart_parity_mode_t parityMode; /!< Parity mode, disabled (default), even, odd /
	#if defined(FSL_FEATURE_UART_HAS_STOP_BIT_CONFIG_SUPPORT) && FSL_FEATURE_UART_HAS_STOP_BIT_CONFIG_SUPPORT
	uart_stop_bit_count_t stopBitCount; /!< Number of stop bits, 1 stop bit (default) or 2 stop bits /
	#endif
	#if defined(FSL_FEATURE_UART_HAS_FIFO) && FSL_FEATURE_UART_HAS_FIFO
	uint8_t txFifoWatermark; /!< TX FIFO watermark /
	uint8_t rxFifoWatermark; /!< RX FIFO watermark /
	#endif
	bool enableTx; /!< Enable TX /
	bool enableRx; /!< Enable RX /
	} uart_config_t;

		#include "test_utils.h"


		#define pwm_debug_printf(...)

Bring FPGA-Test-Shield tests into Mbed-os master. #10820

Bring FPGA-Test-Shield tests into Mbed-os master. #10820

Uh oh!

Conversation

mprse commented Jun 13, 2019

Description

Pull request type

Reviewers

Uh oh!

ciarmcom commented Jun 13, 2019

Uh oh!

adbridge commented Jun 20, 2019

Uh oh!

jamesbeyond left a comment

Choose a reason for hiding this comment

Uh oh!

c1728p9 commented Jun 28, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mprse commented Jun 29, 2019

Uh oh!

mprse commented Jul 3, 2019

Uh oh!

mprse commented Jul 3, 2019

Uh oh!

0xc0170 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!

0xc0170 commented Jul 3, 2019

Uh oh!

mprse commented Jul 3, 2019

Uh oh!

mprse commented Jul 3, 2019

Uh oh!

jamesbeyond commented Jul 3, 2019

Uh oh!

mprse commented Jul 3, 2019

Uh oh!

jamesbeyond left a comment

Choose a reason for hiding this comment

Uh oh!

mprse commented Jul 4, 2019

Uh oh!

0xc0170 commented Jul 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fkjagodzinski Jul 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 Jul 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

fkjagodzinski Jul 4, 2019 •

edited

Loading

0xc0170 Jul 4, 2019 •

edited

Loading