tests: Convert SPI Loopback test to ZTEST and clean up structure of file #86383

decsny · 2025-02-26T21:33:56Z

Since I am working a lot with this test lately, I am following the boy scout rule and cleaning up this test file. This change-set mostly only addresses code structure and makes minimal functional changes to the test. More information is in the commit descriptions, and I tried to make each patch as reviewable as possible since this test is used by many platforms.

I plan to make a follow-up PR to introduce more test cases to this test to avoid many of the bugs that NXP has discovered existing when using the LPSPI driver that were not caught by any testing.

mmahadevan108 · 2025-02-26T23:55:27Z

tests/drivers/spi/spi_loopback/src/spi.c

 }
-
-ZTEST_SUITE(spi_loopback, NULL, spi_loopback_setup, NULL, NULL, NULL);


Is this not required to have this macro?
ZTEST_SUITE(spi_loopback, NULL, NULL, NULL, NULL, NULL);

Not sure what you mean by this comment, This PR is updating the test to use multiple Ztest cases and suites

hakehuang · 2025-02-28T08:17:28Z

@decsny , this fails on frdm_k64f with below log

*** Booting Zephyr OS build v4.1.0-rc2-103-g2cf6b7ee643f ***
SPI test on buffers TX/RX 0x20000fe0/0x20000fc0, frame size = 8, DMA enabled
Polling...Running TESTSUITE spi_extra_api_features
===================================================================
START - test_spi_lock_release
 PASS - test_spi_lock_release in 0.001 seconds
===================================================================
TESTSUITE spi_extra_api_features succeeded
Running TESTSUITE spi_extra_api_features
===================================================================
START - test_spi_lock_release
 PASS - test_spi_lock_release in 0.001 seconds
===================================================================
TESTSUITE spi_extra_api_features succeeded
Running TESTSUITE spi_loopback
===================================================================
Testing loopback spec: SLOW
START - test_spi_async_call
 PASS - test_spi_async_call in 0.026 seconds
===================================================================
START - test_spi_complete_large_transfers
 PASS - test_spi_complete_large_transfers in 0.025 seconds
===================================================================
START - test_spi_complete_loop
 PASS - test_spi_complete_loop in 0.001 seconds
===================================================================
START - test_spi_complete_multiple
 PASS - test_spi_complete_multiple in 0.002 seconds
===================================================================
START - test_spi_null_tx_buf
 PASS - test_spi_null_tx_buf in 0.001 seconds
===================================================================
START - test_spi_rx_bigger_than_tx

    Assumption failed at WEST_TOPDIR/zephyr/tests/drivers/spi/spi_loopback/src/spi.c:274: spi_loopback_test_spi_rx_bigger_than_tx: IS_ENABLED(CONFIG_SPI_STM32_DMA) || IS_ENABLED(CONFIG_DSPI_MCUX_EDMA) is true

 SKIP - test_spi_rx_bigger_than_tx in 0.019 seconds
===================================================================
START - test_spi_rx_every_4

    Assumption failed at WEST_TOPDIR/zephyr/tests/drivers/spi/spi_loopback/src/spi.c:251: spi_loopback_test_spi_rx_every_4: IS_ENABLED(CONFIG_SPI_STM32_DMA) || IS_ENABLED(CONFIG_DSPI_MCUX_EDMA) is true

 SKIP - test_spi_rx_every_4 in 0.018 seconds
===================================================================
START - test_spi_rx_half_end
 PASS - test_spi_rx_half_end in 0.001 seconds

decsny · 2025-02-28T15:28:37Z

@decsny , this fails on frdm_k64f with below log

That's weird, because frdm_k64f is the main one I was using to develop this PR, do you have any special modification? I'm not seeing a failing on my board, I also tried with DSPI DMA testcase which looks like you used here. and still passes. Also is what you shared the whole log or is there more, right now I'm assuming your log is saying it hanged since it is incomplete?

hakehuang · 2025-03-01T11:16:54Z

That's weird, because frdm_k64f is the main one I was using to develop this PR, do you have any special modification? I'm not seeing a failing on my board, I also tried with DSPI DMA testcase which looks like you used here. and still passes. Also is what you shared the whole log or is there more, right now I'm assuming your log is saying it hanged since it is incomplete?

Maybe this is my board issue, as it is OK on your board, and all other platforms in my board farm.

djiatsaf-st · 2025-03-03T16:37:29Z

Hello @decsny
I have failures on several stm32 boards on the following test scenarios

tests/drivers/spi/spi_loopback/drivers.spi.loopback, nucleo_f207zg/stm32f207xx
tests/drivers/spi/spi_loopback/drivers.spi.loopback, nucleo_f429zi/stm32f429xx
tests/drivers/spi/spi_loopback/drivers.spi.loopback, nucleo_f746zg/stm32f746xx
tests/drivers/spi/spi_loopback/drivers.spi.loopback, nucleo_h743zi/stm32h743xx
tests/drivers/spi/spi_loopback/drivers.spi.loopback, nucleo_wb55rg/stm32wb55xx
tests/drivers/spi/spi_loopback/drivers.spi.loopback, nucleo_wl55jc/stm32wl55xx
tests/drivers/spi/spi_loopback/drivers.spi.loopback, stm32f3_disco@B/stm32f303xc
tests/drivers/spi/spi_loopback/drivers.spi.loopback, stm32h573i_dk/stm32h573xx

Main error :

Testing loopback spec: SLOW
START - test_spi_async_call

    Assumption failed at WEST_TOPDIR/zephyr/tests/drivers/spi/spi_loopback/src/spi.c:367: spi_loopback_test_spi_async_call: ret != -ENOTSUP is false

 SKIP - test_spi_async_call in 0.015 seconds

tests/drivers/spi/spi_loopback/drivers.spi.loopback.internal, stm32f3_disco@B/stm32f303xc (
tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_16bits_frames.loopback, nucleo_h743zi/stm32h743xx
tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_16bits_frames_dma.loopback, nucleo_h743zi/stm32h743xx
tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_16bits_frames_dma_dt_nocache_mem.loopback, nucleo_h743zi/stm32h743xx
tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_dma.loopback, nucleo_c071rb/stm32c071xx
tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_dma.loopback, nucleo_f207zg/stm32f207xx
tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_dma.loopback, nucleo_f429zi/stm32f429xx
tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_dma.loopback, nucleo_f746zg/stm32f746xx
tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_dma.loopback, nucleo_g474re/stm32g474xx
tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_dma.loopback, nucleo_h743zi/stm32h743xx
tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_dma.loopback, nucleo_wb55rg/stm32wb55xx
tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_dma.loopback, nucleo_wl55jc/stm32wl55xx
tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_dma.loopback, stm32h573i_dk/stm32h573xx
tests/drivers/spi/spi_loopback/drivers.spi.stm32_spi_dma_dt_nocache_mem.loopback, nucleo_h743zi/stm32h743xx

Main error:

   
  Assumption failed at WEST_TOPDIR/zephyr/tests/drivers/spi/spi_loopback/src/spi.c:274: spi_loopback_test_spi_rx_bigger_than_tx: IS_ENABLED(CONFIG_SPI_STM32_DMA) || IS_ENABLED(CONFIG_DSPI_MCUX_EDMA) is true 

SKIP - test_spi_rx_bigger_than_tx in 0.019 seconds 

=================================================================== 

START - test_spi_rx_every_4 

    Assumption failed at WEST_TOPDIR/zephyr/tests/drivers/spi/spi_loopback/src/spi.c:251: spi_loopback_test_spi_rx_every_4: IS_ENABLED(CONFIG_SPI_STM32_DMA) || IS_ENABLED(CONFIG_DSPI_MCUX_EDMA) is true 

SKIP - test_spi_rx_every_4 in 0.018 seconds 

=================================================================== 

START - test_spi_rx_half_end 

    Assumption failed at WEST_TOPDIR/zephyr/tests/drivers/spi/spi_loopback/src/spi.c:232: spi_loopback_test_spi_rx_half_end: IS_ENABLED(CONFIG_SPI_STM32_DMA) is true

I am analyzing to understand what introduces these regressions

decsny · 2025-03-03T18:07:45Z

Hello @decsny I have failures on several stm32 boards on the following test scenarios

I am analyzing to understand what introduces these regressions

AFAIK, this is not a failure, or a regression, the test hardcoded skipped these tests before, the only new thing is now we use zassume api to skip the test instead of just returning, so ZTest logs the skip properly and the reason for the skip.

Note "assumption failed" is not the same as "assertion failed", failing an assumption is a skip, just like your log shows, that is intentional. If you don't want to skip those tests for STM32 DMA, then that is outside the scope of this PR, I am just keeping what is there right now but converting it to use ZTest APIs properly.

If you weren't aware the platform was skipping those tests before, that's another advantage to use ZTest I guess because it makes it clear in the log.

djiatsaf-st · 2025-03-04T09:29:31Z

Hello @decsny I have failures on several stm32 boards on the following test scenarios
I am analyzing to understand what introduces these regressions

AFAIK, this is not a failure, or a regression, the test hardcoded skipped these tests before, the only new thing is now we use zassume api to skip the test instead of just returning, so ZTest logs the skip properly and the reason for the skip.

Note "assumption failed" is not the same as "assertion failed", failing an assumption is a skip, just like your log shows, that is intentional. If you don't want to skip those tests for STM32 DMA, then that is outside the scope of this PR, I am just keeping what is there right now but converting it to use ZTest APIs properly.

If you weren't aware the platform was skipping those tests before, that's another advantage to use ZTest I guess because it makes it clear in the log.

Thanks for your explanation, it's very clear. It seems good for the scope of this PR.

It could be that twister considers "assumption failed" is the same as "assertion failed" when report tests status.

decsny · 2025-03-04T19:03:17Z

@djiatsaf-st it's no problem if we should figure out why does a skip cause a twister fail before this merges, no rush from my point of view, maybe some change can be made on the PR

decsny · 2025-03-04T19:43:54Z

BTW @tbursztyka this PR will require your review

decsny · 2025-03-13T18:16:04Z

I don't know why is twister / ztest considering the skips as fails, but don't have time to look into this and it's pretty minor, so I just converted the zassume calls into just a conditional and TC_PRINT and return, so they should pass now.

@djiatsaf-st @hakehuang @tbursztyka @teburd please help review, I'd like to get this change in sooner than later to facilitate more testing additions during this cycle

djiatsaf-st

Othewise LGTM

djiatsaf-st · 2025-03-17T13:41:48Z

tests/drivers/spi/spi_loopback/src/spi.c

-		LOG_ERR("Code %d", ret);
-		zassert_false(ret, "SPI transceive failed");
-		return -1;
+	if (ret != -ENOTSUP) {


I think it should remain ret == -ENOTSUP or handle the case when this condition is met.

Without this, we don't handle the case where ret is equal to -ENOTSUP (-134), and the test will fail on this assertion: zassert_false(ret, "SPI transceive failed, code %d", ret); because ret is true.

The test failed on boards like Nucleo_F207ZG, Nucleo_F429ZI, Nucleo_F746ZG, Nucleo_H743ZI, Nucleo_WB55RG, etc., which don't support async call with the log :

Assertion failed at WEST_TOPDIR/zephyr/tests/drivers/spi/spi_loopback/src/spi.c:384: spi_loopback_test_spi_async_call: (ret is true) SPI transceive failed, code -134

woops, this definitely was a major typo, that would have skipped the whole test case for everybody except the ones who were meant to skip it, thanks for catching this

tbursztyka

This test was indeed very much cluttered and the output not as normalized at ZTEST can get.

That's a really nice one, thank you!

JarmouniA

And while we are at it, we should remove the __used attribute instances, it is actually for functions not variables.

JarmouniA · 2025-03-18T21:42:05Z

tests/drivers/spi/spi_loopback/src/spi.c

+	printf("SPI test on buffers TX/RX %p/%p, frame size = %d"
+#ifdef CONFIG_DMA
+		", DMA enabled"
+#ifdef CONFIG_NOCACHE_MEMORY


Suggested change

#ifdef CONFIG_NOCACHE_MEMORY

#ifndef CONFIG_NOCACHE_MEMORY

EmilioCBen

LGTM!

Following the boy scout rule, since I am using this test a lot lately and going to improve it, the first step will be to clean up the structure of the file. The following changes are made: - Remove unused #includes - Condense some #if #else macro defines or make into single line, to simplify the readability of the preprocessor code - Move definitions around so that related things are all next to each other, instead of randomly scattered around the file - Create section header comments for broadly related things in the file to improve developer navigation experience - Introduce macro for copy pasted print buf size calculation - A few minor comment edits Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

There was a lot of copy paste of this code: int ret; ret = spi_transceive_dt(spec, &tx, &rx); if (ret) { LOG_ERR("Code %d", ret); zassert_false(ret, "SPI transceive failed"); return ret; } This is quite redundant and can be simplified. Also, return is not needed because a failed zassert will not continue to return anyways. Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

More boy scout rule, I noticed every single test case was using different variations of C syntax to declare the buffers, this is inconvenient to the reader, make a standard way to declare buffers used for the test. Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

Instead of having a bunch of giant if conditions and just running all the test cases from one actual ZTest case, convert all the test cases into proper ZTest test cases. Removing the returns is now required because otherwise there will be compiler warnings, and they were never doing anything anyways in the event of zassert fail. ZTest cases are meant to report pass or fail with ztest paradigm, not with return values and log messages. Also move the test of the spi lock/release to a separate test suite since it is not really testing an actual bus transfer, but rather a that a feature in the SPI API is respected. Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

Since we are using ZTest, if we use the ZTest paradigm properly, we don't need a logging module, since ZTest already logs what it does. Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

decsny · 2025-03-20T22:05:03Z

rebased and took suggestions from @JarmouniA to fix the print statement

zephyrbot added the area: SPI SPI bus label Feb 26, 2025

zephyrbot requested review from tbursztyka and teburd February 26, 2025 21:34

zephyrbot assigned tbursztyka Feb 26, 2025

decsny requested a review from ubieda February 26, 2025 21:35

mmahadevan108 reviewed Feb 26, 2025

View reviewed changes

decsny force-pushed the fix/spi_loopback_ztest branch 2 times, most recently from 470a367 to 7cfa9a9 Compare February 27, 2025 03:26

decsny requested a review from hakehuang February 27, 2025 11:55

decsny force-pushed the fix/spi_loopback_ztest branch 2 times, most recently from 49b78c0 to 2cf6b7e Compare February 27, 2025 23:19

hakehuang previously approved these changes Mar 1, 2025

View reviewed changes

JarmouniA added the platform: STM32 ST Micro STM32 label Mar 2, 2025

JarmouniA requested a review from erwango March 2, 2025 04:57

erwango requested a review from djiatsaf-st March 3, 2025 08:16

djiatsaf-st mentioned this pull request Mar 4, 2025

twister : tests report: project execution failed without any testsuites failed #86611

Open

decsny added the DNM This PR should not be merged (Do Not Merge) label Mar 4, 2025

decsny dismissed hakehuang’s stale review via 3c541c8 March 13, 2025 18:14

decsny force-pushed the fix/spi_loopback_ztest branch from 2cf6b7e to 3c541c8 Compare March 13, 2025 18:14

decsny removed the DNM This PR should not be merged (Do Not Merge) label Mar 13, 2025

decsny force-pushed the fix/spi_loopback_ztest branch from 3c541c8 to ca098fb Compare March 13, 2025 18:19

hakehuang previously approved these changes Mar 14, 2025

View reviewed changes

djiatsaf-st requested changes Mar 17, 2025

View reviewed changes

decsny dismissed hakehuang’s stale review via 4517935 March 17, 2025 16:11

decsny force-pushed the fix/spi_loopback_ztest branch from ca098fb to 4517935 Compare March 17, 2025 16:11

djiatsaf-st previously approved these changes Mar 18, 2025

View reviewed changes

tbursztyka previously approved these changes Mar 18, 2025

View reviewed changes

JarmouniA suggested changes Mar 18, 2025

View reviewed changes

JarmouniA added the DNM This PR should not be merged (Do Not Merge) label Mar 18, 2025

EmilioCBen previously approved these changes Mar 20, 2025

View reviewed changes

decsny added 5 commits March 20, 2025 14:59

tests: spi_loopback: Remove logging module

3068d01

Since we are using ZTest, if we use the ZTest paradigm properly, we don't need a logging module, since ZTest already logs what it does. Signed-off-by: Declan Snyder <declan.snyder@nxp.com>

decsny dismissed stale reviews from EmilioCBen, tbursztyka, and djiatsaf-st via 3068d01 March 20, 2025 22:02

decsny force-pushed the fix/spi_loopback_ztest branch from 4517935 to 3068d01 Compare March 20, 2025 22:02

decsny removed the DNM This PR should not be merged (Do Not Merge) label Mar 20, 2025

decsny requested review from JarmouniA, hakehuang, tbursztyka, EmilioCBen, djiatsaf-st and mmahadevan108 March 20, 2025 22:04

JarmouniA approved these changes Mar 20, 2025

View reviewed changes

tbursztyka approved these changes Mar 21, 2025

View reviewed changes

djiatsaf-st approved these changes Mar 21, 2025

View reviewed changes

nashif merged commit 3a477c5 into zephyrproject-rtos:main Mar 21, 2025
16 of 17 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tests: Convert SPI Loopback test to ZTEST and clean up structure of file #86383

tests: Convert SPI Loopback test to ZTEST and clean up structure of file #86383

decsny commented Feb 26, 2025

mmahadevan108 Feb 26, 2025

decsny Feb 27, 2025 •

edited

Loading

hakehuang commented Feb 28, 2025

decsny commented Feb 28, 2025 •

edited

Loading

hakehuang commented Mar 1, 2025

djiatsaf-st commented Mar 3, 2025

decsny commented Mar 3, 2025 •

edited

Loading

djiatsaf-st commented Mar 4, 2025

decsny commented Mar 4, 2025 •

edited

Loading

decsny commented Mar 4, 2025

decsny commented Mar 13, 2025 •

edited

Loading

djiatsaf-st left a comment

djiatsaf-st Mar 17, 2025

decsny Mar 17, 2025

tbursztyka left a comment

JarmouniA left a comment

JarmouniA Mar 18, 2025

decsny Mar 18, 2025

EmilioCBen left a comment

decsny commented Mar 20, 2025 •

edited

Loading

		}

		ZTEST_SUITE(spi_loopback, NULL, spi_loopback_setup, NULL, NULL, NULL);

tests: Convert SPI Loopback test to ZTEST and clean up structure of file #86383

tests: Convert SPI Loopback test to ZTEST and clean up structure of file #86383

Conversation

decsny commented Feb 26, 2025

mmahadevan108 Feb 26, 2025

Choose a reason for hiding this comment

decsny Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

hakehuang commented Feb 28, 2025

decsny commented Feb 28, 2025 • edited Loading

hakehuang commented Mar 1, 2025

djiatsaf-st commented Mar 3, 2025

decsny commented Mar 3, 2025 • edited Loading

djiatsaf-st commented Mar 4, 2025

decsny commented Mar 4, 2025 • edited Loading

decsny commented Mar 4, 2025

decsny commented Mar 13, 2025 • edited Loading

djiatsaf-st left a comment

Choose a reason for hiding this comment

djiatsaf-st Mar 17, 2025

Choose a reason for hiding this comment

decsny Mar 17, 2025

Choose a reason for hiding this comment

tbursztyka left a comment

Choose a reason for hiding this comment

JarmouniA left a comment

Choose a reason for hiding this comment

JarmouniA Mar 18, 2025

Choose a reason for hiding this comment

decsny Mar 18, 2025

Choose a reason for hiding this comment

EmilioCBen left a comment

Choose a reason for hiding this comment

decsny commented Mar 20, 2025 • edited Loading

decsny Feb 27, 2025 •

edited

Loading

decsny commented Feb 28, 2025 •

edited

Loading

decsny commented Mar 3, 2025 •

edited

Loading

decsny commented Mar 4, 2025 •

edited

Loading

decsny commented Mar 13, 2025 •

edited

Loading

decsny commented Mar 20, 2025 •

edited

Loading