Add test header files and defined behavior for APIs tested using FPGA-Test-Shield #11032

mprse · 2019-07-12T08:31:08Z

Description

This PR adds header files for FPGA tests with given/when/then description for the test cases and proposition for defined/undefined behavior for the tested APIs.

Please fill free to propose the modification in the defined behavior since this first version has been created mainly based on the description of the functions.

When we agree on the final version of the defined/undefined behavior for the tested APIs, then in the next step we can update the tests (increase coverage) and add a mapping between defined behavior and test cases.

Pull request type

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

Reviewers

@maciejbocianski @fkjagodzinski @c1728p9 @0xc0170 @jamesbeyond

Notes

BTW I'm OoO from 15/07 to 28/07.

ciarmcom · 2019-07-12T11:00:45Z

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

c1728p9

Great work so far @mprse!

I recommend linking all the defined behavior to tests or indicating "Not Verified" as done in other APIs, such as the RTC API. I also left some comments inline

hal/analogin_api.h

@@ -35,9 +35,29 @@ typedef struct analogin_s analogin_t;

 /**
 * \defgroup hal_analogin Analogin hal functions
+ *
+ * # Defined behaviour
+ * * The function ::analogin_init initializes the analogin peripheral


hal/analogin_api.h

+ * # Defined behaviour
+ * * The function ::analogin_init initializes the analogin peripheral
+ * * The function ::analogin_read reads the input voltage, represented as a float in the range [0.0, 1.0]
+ * * The function ::analogin_read_u16 reads the value from analogin pin, represented as an unsigned 16bit value


hal/analogout_api.h

+ *
+ * # Defined behaviour
+ * * The function ::analogout_init initializes the analogin peripheral
+ * * The function ::analogout_write sets the output voltage, specified as a percentage (float) in range [0.0, 1.0]


hal/i2c_api.h

@@ -70,9 +70,72 @@ extern "C" {

 /**
 * \defgroup hal_GeneralI2C I2C Configuration Functions
+ *


c1728p9 · 2019-07-12T14:10:27Z

hal/spi_api.h

+ * * ::spi_irq_handler_asynch checks for transfer termination conditions, such as buffer overflows or transfer complete
+ * * ::spi_irq_handler_asynch returns event flags if a transfer termination condition was met, otherwise 0
+ * * ::spi_abort_asynch aborts an on-going async transfer
+ * * ::spi_active returns non-zero if the SPI port is active or zero if it is not


It may be good to re-use the relevant existing SPI API defined and undefined behavior from the feature branch here:
https://github.com/ARMmbed/mbed-os/blob/feature-hal-spec-spi/hal/spi_api.h#L40-L125

@mprse This was resolved?

Yes I created these requirements based on the SPI API defined and undefined behavior from the feature branch.

c1728p9 · 2019-07-12T14:13:21Z

hal/serial_api.h

+ * * The callback given to ::serial_rx_asynch is invoked when the transfer completes
+ * * ::serial_tx_asynch specifies the logical OR of events to be registered
+ * * The ::serial_tx_asynch function may use the `DMAUsage` hint to select the appropriate async algorithm
+ * * ::serial_tx_asynch specifies the character in range 0-254 to be matched


Why not 255?

Looks like 255 is a reserved value:

mbed-os/hal/serial_api.h

Line 63 in 7d15882

#define SERIAL_RESERVED_CHAR_MATCH (255)

c1728p9 · 2019-07-12T14:15:19Z

hal/serial_api.h

+ * * ::serial_tx_abort_asynch flushes the TX hardware buffer if TX FIFO is used
+ * * ::serial_rx_abort_asynch aborts the ongoing RX transaction
+ * * ::serial_rx_abort_asynch disables the enabled interupt for RX
+ * * ::serial_rx_abort_asynch flushes the TX hardware buffer if RX FIFO is used


Some of these functions are used from interrupts. It may be good to define and test for maximum function call duration. That would ensure interrupt latency stays low.

c1728p9 · 2019-07-12T14:21:01Z

hal/serial_api.h

@@ -108,9 +108,69 @@ extern "C" {

 /**
 * \defgroup hal_GeneralSerial Serial Configuration Functions
+ *
+ * # Defined behaviour


It would also be good to add interrupt latency requirements to defined and/or undefined behavior.

Correct operation guaranteed when interrupt latency is < 8 / baudrate_frequency without flow control

Correct operation guaranteed regardless of interrupt latency with flow control

For reference USB HAL spec has some timing requirements
https://github.com/ARMmbed/mbed-os/blob/master/usb/device/USBPhy/USBPhy.h#L33-L34:
https://github.com/ARMmbed/mbed-os/blob/master/usb/device/USBPhy/USBPhy.h#L58-L61

fkjagodzinski · 2019-07-15T11:39:47Z

TESTS/mbed_hal_fpga_ci_test_shield/pwm/pwm_fpga_test.h

+ * Then the operation is successfull.
+ *
+ */
+void fpga_pwm_period_fill_test(PinName pin);


Suggested change

void fpga_pwm_period_fill_test(PinName pin);

void fpga_pwm_init_free(PinName pin);

Wrong function name.

fkjagodzinski · 2019-07-15T11:53:18Z

TESTS/mbed_hal_fpga_ci_test_shield/uart/main.cpp

@@ -270,7 +270,7 @@ static void uart_test_common(int baudrate, int data_bits, SerialParity parity, i
    tester.reset();
 }

-void test_init_free(PinName tx, PinName rx, PinName cts = NC, PinName rts = NC)
+void fpga_uart_init_free_test(PinName tx, PinName rx, PinName cts = NC, PinName rts = NC)


Suggested change

void fpga_uart_init_free_test(PinName tx, PinName rx, PinName cts = NC, PinName rts = NC)

void fpga_uart_init_free_test(PinName tx, PinName rx, PinName cts, PinName rts)

Defaults moved to the header file.

This comment is still valid. Defaults should always go in the header file.

SeppoTakalo · 2019-07-17T12:01:40Z

Conflicts with the base, please rebase.

fkjagodzinski · 2019-07-26T15:43:06Z

I've updated the defined and undefined behavior section of the serial HAL header. The patch is available here: fkjagodzinski@e1c3da6 (I don't have a permission to push to this branch).

@mprse, could you cherry-pick fkjagodzinski@e1c3da6 here before you do any rebase/update?

Please note, I've left a few FIXME notes -- these points still need clarification.
I also added the missing parts regarding IRQ behavior mentioned in #11001.

mprse · 2019-07-30T08:49:52Z

PR has been rebased and updated (review comments have been addressed). @c1728p9 @fkjagodzinski Thanks for the review!

@mprse, could you cherry-pick fkjagodzinski/mbed-os@e1c3da6 here before you do any rebase/update?
Please note, I've left a few FIXME notes -- these points still need clarification.

@fkjagodzinski Thanks for helping with the requirements for the serial API. I have cherry-picked your changes as you suggested and resolved FIXME notes in the following commit: 9aea87c.

I recommend linking all the defined behavior to tests or indicating "Not Verified" as done in other APIs, such as the RTC API.

This will be done in the next step. I assume that when this is merged authors of the FPGA tests will review the tests, add missing tests cases for the requirements that can be verified and link the defined behavior to tests.

This one is ready for CI/further review.

mbed-ci · 2019-08-01T16:24:33Z

Test run: SUCCESS

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

fkjagodzinski · 2019-08-12T13:03:08Z

I think we should establish a naming convention for the test functions. Currently we have a few naming patterns here:

fpga_<HAL_API>_<test_name>_test:
- fpga_analogin_init_test,
- fpga_gpio_irq_test,
- fpga_pwm_period_fill_test,
- fpga_uart_init_free_test,
fpga_test_<test_name>:
- fpga_test_basic_input_output,
fpga_test_<HAL_API>_<test_name>:
- fpga_test_i2c_init_free,
fpga_<HAL_API>_test_<test_name>:
- fpga_i2c_test_byte_read,
- fpga_spi_test_init_free,
- fpga_uart_test_common
fpga_<HAL_API>_<test_name>:
- fpga_pwm_init_free.

I think we could go with fpga_test_<HAL_API>_<test_name> for all the FPGA tests and test_<HAL_API>_<test_name> for non-FPGA ones. What do you think?

mprse · 2019-08-13T07:14:06Z

I think we should establish a naming convention for the test functions. Currently we have a few naming patterns here:

fpga_<HAL_API>_<test_name>_test:

fpga_analogin_init_test,

fpga_gpio_irq_test,

fpga_pwm_period_fill_test,

fpga_uart_init_free_test,

fpga_test_<test_name>:

fpga_test_basic_input_output,

fpga_test_<HAL_API>_<test_name>:

fpga_test_i2c_init_free,

fpga_<HAL_API>_test_<test_name>:

fpga_i2c_test_byte_read,

fpga_spi_test_init_free,

fpga_uart_test_common

fpga_<HAL_API>_<test_name>:

fpga_pwm_init_free.

I think we could go with fpga_test_<HAL_API>_<test_name> for all the FPGA tests and test_<HAL_API>_<test_name> for non-FPGA ones. What do you think?

I agree that this should be unified, but generally, it is not a problem while the test functions have different names in all test files. This is required to add a mapping to the defined behavior. E.G. in case of spi we had basic test and fpga test and some test functions had the same names. This is why I added fpga_ prefix to the fpga test functions.
When this is merged (defined behavior is ready), then the next step is to add mappings and optionally missing test cases for the untested requirements. This will be a good moment to unify the test names.

mprse · 2019-08-13T07:30:58Z

Rebased on the master to resolve conflicts.

0xc0170 · 2019-09-19T11:39:48Z

@mprse Shall this get our attention now (rebase needed first) ?

mprse · 2019-09-19T13:14:18Z

Yes, I will rebase this PR tomorrow.

mprse · 2019-10-28T12:11:56Z

Rebased on the master to solve i2c test conflict.
Ready for CI.

0xc0170 · 2019-10-28T12:20:13Z

CI started

mprse · 2019-10-28T14:45:05Z

Please review @maciejbocianski @jamesbeyond .

mbed-ci · 2019-10-28T15:58:14Z

Test run: FAILED

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

Failed test jobs:

jenkins-ci/mbed-os-ci_build-GCC_ARM

mprse · 2019-10-29T07:16:27Z

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.

0xc0170 · 2019-10-29T08:59:35Z

CI restarted

mprse · 2019-10-29T13:08:52Z

Recent updates:

Added analogin_free(), gpio_free(), i2c_free() functions to HAL,
Added defined behaviour for free() functions,
Added weak default implementation of analogin_free(), gpio_free(), i2c_free(),
Modified the FPGA tests to make use of the free() functions.

Tested on K64F/FPGA-Test-Shield:

mbedgt: test suite report:
| target       | platform_name | test suite                                  | result | elapsed_time (sec) | copy_method |
|--------------|---------------|---------------------------------------------|--------|--------------------|-------------|
| K64F-GCC_ARM | K64F          | tests-mbed_hal_fpga_ci_test_shield-analogin | OK     | 22.18              | default     |
| K64F-GCC_ARM | K64F          | tests-mbed_hal_fpga_ci_test_shield-gpio     | OK     | 24.15              | default     |
| K64F-GCC_ARM | K64F          | tests-mbed_hal_fpga_ci_test_shield-gpio_irq | OK     | 23.45              | default     |
| K64F-GCC_ARM | K64F          | tests-mbed_hal_fpga_ci_test_shield-i2c      | OK     | 22.34              | default     |
| K64F-GCC_ARM | K64F          | tests-mbed_hal_fpga_ci_test_shield-pwm      | OK     | 52.05              | default     |
| K64F-GCC_ARM | K64F          | tests-mbed_hal_fpga_ci_test_shield-spi      | OK     | 25.52              | default     |
| K64F-GCC_ARM | K64F          | tests-mbed_hal_fpga_ci_test_shield-uart     | OK     | 28.04              | default     |
mbedgt: test suite results: 7 OK

mbed-ci · 2019-10-29T13:19:41Z

Test run: FAILED

Summary: 3 of 11 test jobs failed
Build number : 8
Build artifacts

Failed test jobs:

jenkins-ci/mbed-os-ci_mbed2-build-GCC_ARM
jenkins-ci/mbed-os-ci_mbed2-build-IAR
jenkins-ci/mbed-os-ci_mbed2-build-ARM

0xc0170 · 2019-10-29T13:19:54Z

CI aborted , will restart

mprse · 2019-10-29T13:20:27Z

Fixed style.

mbed-ci · 2019-10-29T16:39:08Z

Test run: SUCCESS

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

mprse · 2019-10-30T08:05:23Z

CI passed I think that this one is ready to go in.

ciarmcom requested review from 0xc0170, c1728p9, fkjagodzinski, jamesbeyond, maciejbocianski and a team July 12, 2019 11:00

ciarmcom added the needs: review label Jul 12, 2019

c1728p9 reviewed Jul 12, 2019

View reviewed changes

fkjagodzinski suggested changes Jul 15, 2019

View reviewed changes

SeppoTakalo added the needs: work label Jul 17, 2019

SeppoTakalo removed the needs: review label Jul 22, 2019

fkjagodzinski mentioned this pull request Jul 26, 2019

FPGA CI shield assumes UART behaviour it shouldn't #11001

Closed

mprse force-pushed the bring_fpga_tests_master_spec branch from e95ef36 to 31a1335 Compare July 30, 2019 08:39

SeppoTakalo added needs: CI needs: review and removed needs: work labels Aug 1, 2019

SeppoTakalo removed the needs: CI label Aug 2, 2019

mprse force-pushed the bring_fpga_tests_master_spec branch from b4c32e5 to 741044c Compare August 13, 2019 07:30

0xc0170 added needs: CI and removed needs: review labels Oct 28, 2019

0xc0170 approved these changes Oct 28, 2019

View reviewed changes

mprse mentioned this pull request Oct 29, 2019

AnalogIn test issue #11746

Closed

mprse added 3 commits October 29, 2019 10:48

Analogin: Add analogin_free() function to HAL

05d8e1b

GPIO: Add gpio_free() function to HAL

f19681f

I2C: Add i2c_free() function to HAL

b8a6792

mprse added 2 commits October 29, 2019 14:18

Add weak implementations of xxx_free() functions

7a6cf2c

Make use of xxx_free() in FPGA tests

2ff3112

mprse force-pushed the bring_fpga_tests_master_spec branch from b7332f5 to 2ff3112 Compare October 29, 2019 13:19

0xc0170 added ready for merge and removed needs: CI labels Oct 30, 2019

0xc0170 merged commit 6f53899 into ARMmbed:master Oct 30, 2019

0xc0170 removed the ready for merge label Oct 30, 2019

kjbracey mentioned this pull request Nov 14, 2019

STM32L152RE deepsleep state power too high #11853

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

jeromecoutant mentioned this pull request Sep 21, 2020

Fix AnalogIn implementation on STM32G4 series #13601

Merged

	void fpga_pwm_period_fill_test(PinName pin);
	void fpga_pwm_init_free(PinName pin);

	void fpga_uart_init_free_test(PinName tx, PinName rx, PinName cts = NC, PinName rts = NC)
	void fpga_uart_init_free_test(PinName tx, PinName rx, PinName cts, PinName rts)

Add test header files and defined behavior for APIs tested using FPGA-Test-Shield #11032

Add test header files and defined behavior for APIs tested using FPGA-Test-Shield #11032

Conversation

mprse commented Jul 12, 2019

Description

Pull request type

Reviewers

Notes

ciarmcom commented Jul 12, 2019

c1728p9 left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

This comment was marked as resolved.

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

SeppoTakalo commented Jul 17, 2019

fkjagodzinski commented Jul 26, 2019

mprse commented Jul 30, 2019

mbed-ci commented Aug 1, 2019

fkjagodzinski commented Aug 12, 2019

mprse commented Aug 13, 2019 • edited Loading

mprse commented Aug 13, 2019

0xc0170 commented Sep 19, 2019

mprse commented Sep 19, 2019

mprse commented Oct 28, 2019

0xc0170 commented Oct 28, 2019

mprse commented Oct 28, 2019

mbed-ci commented Oct 28, 2019

mprse commented Oct 29, 2019

0xc0170 commented Oct 29, 2019

mprse commented Oct 29, 2019

mbed-ci commented Oct 29, 2019

0xc0170 commented Oct 29, 2019

mprse commented Oct 29, 2019

mbed-ci commented Oct 29, 2019

mprse commented Oct 30, 2019

mprse commented Aug 13, 2019 •

edited

Loading