Skip to content

Conversation

@de-nordic
Copy link
Contributor

Two commits:

  • changing flash_area_get_sectors test to just verify layout against device layout, without doing read/write to device
  • move SHA calculation to separate main file, so that only SHA is tested in SHA test scenarios and there are not basic tests doing writes/erases

@de-nordic de-nordic added this to the v4.1.0 milestone Nov 7, 2024
@de-nordic de-nordic self-assigned this Nov 7, 2024
@de-nordic de-nordic requested a review from kartben November 7, 2024 17:11
@zephyrbot zephyrbot added the area: Storage Storage subsystem label Nov 7, 2024
@de-nordic de-nordic requested review from butok and tomi-font November 7, 2024 17:11
@de-nordic de-nordic force-pushed the flash-map-reduce-wear branch from 5eab7f8 to 1e66479 Compare November 7, 2024 18:24
@de-nordic de-nordic requested a review from aescolar November 7, 2024 18:25
aescolar
aescolar previously approved these changes Nov 7, 2024
@de-nordic
Copy link
Contributor Author

Found one more erase to remove.

@de-nordic de-nordic requested a review from aescolar November 7, 2024 19:31
@tomi-font tomi-font changed the title Readuce flash wear in flash map tests Reduce flash wear in flash map tests Nov 8, 2024
Copy link
Contributor

@tomi-font tomi-font left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Commit nit: needles => needless

Remove needless writes/read and erase in flash_area_get_sectors test
scenario, by replacing it with comparison with layout directly obtained
from device.

Signed-off-by: Dominik Ermel <[email protected]>
The commit moves SHA calculation to separate test scenario, so that
it can be run separately from other flash map tests.
This reduces flash wear by not running basic flash map tests each
time different SHA backend is tested.

Signed-off-by: Dominik Ermel <[email protected]>
@de-nordic de-nordic closed this Nov 8, 2024
@de-nordic de-nordic force-pushed the flash-map-reduce-wear branch from 8a4ee69 to d4b7bf9 Compare November 8, 2024 09:14
@de-nordic de-nordic reopened this Nov 8, 2024
@de-nordic
Copy link
Contributor Author

Commit nit: needles => needless

Fixed. I am so happy that this has not removed acks.

@tomi-font
Copy link
Contributor

Fixed. I am so happy that this has not removed acks.

👍 As long as the diff remains the same or it's just a rebase without conflict the acks are not discarded.

@nashif nashif merged commit 06c0b76 into zephyrproject-rtos:main Nov 16, 2024
25 of 27 checks passed
@butok
Copy link
Contributor

butok commented Nov 19, 2024

Hi @de-nordic

Just tried it on real HW.
After this PR, the test by default runs only 1 test, instead of 6. Is it intentional behavior?

It is cause by:

if(NOT CONFIG_FLASH_AREA_CHECK_INTEGRITY_PSA AND NOT CONFIG_FLASH_AREA_CHECK_INTEGRITY_MBEDTLS)
  FILE(GLOB app_sources src/main.c)
else()
  FILE(GLOB app_sources src/main_sha.c)
endif()

As CONFIG_FLASH_AREA_CHECK_INTEGRITY_PSA or CONFIG_FLASH_AREA_CHECK_INTEGRITY_MBEDTLS are set to Y, the cmake always takes main_sha.c which only runs one "test test_flash_area_check_int_sha256" test.

Log for west build -p always -b frdm_mcxn947/mcxn947/cpu0 tests/subsys/storage/flash_map.
After PR:

*** Booting Zephyr OS build v4.0.0-405-g2efc8598e39f ***
Running TESTSUITE flash_map_sha
===================================================================
START - test_flash_area_check_int_sha256
 PASS - test_flash_area_check_int_sha256 in 0.132 seconds
===================================================================
TESTSUITE flash_map_sha succeeded

------ TESTSUITE SUMMARY START ------

SUITE PASS - 100.00% [flash_map_sha]: pass = 1, fail = 0, skip = 0, total = 1 duration = 0.132 seconds
 - PASS - [flash_map_sha.test_flash_area_check_int_sha256] duration = 0.132 seconds

------ TESTSUITE SUMMARY END ------

===================================================================
PROJECT EXECUTION SUCCESSFUL

Before:

*** Booting Zephyr OS build v3.7.0-190-gf09ea4753feb ***
Running TESTSUITE flash_map
===================================================================
START - test_fixed_partition_node_macros
 PASS - test_fixed_partition_node_macros in 0.001 seconds
===================================================================
START - test_flash_area_check_int_sha256
 PASS - test_flash_area_check_int_sha256 in 0.127 seconds
===================================================================
START - test_flash_area_disabled_device
 PASS - test_flash_area_disabled_device in 0.001 seconds
===================================================================
START - test_flash_area_erase_and_flatten
Flash area info:
        pointer:         0x30002ea4
        offset:  1081344
        size:    1015808
 PASS - test_flash_area_erase_and_flatten in 7.912 seconds
===================================================================
START - test_flash_area_erased_val
 PASS - test_flash_area_erased_val in 0.001 seconds
===================================================================
START - test_flash_area_get_sectors
 PASS - test_flash_area_get_sectors in 0.886 seconds
===================================================================
TESTSUITE flash_map succeeded
 
------ TESTSUITE SUMMARY START ------
 
SUITE PASS - 100.00% [flash_map]: pass = 6, fail = 0, skip = 0, total = 6 duration = 8.928 seconds
 - PASS - [flash_map.test_fixed_partition_node_macros] duration = 0.001 seconds
 - PASS - [flash_map.test_flash_area_check_int_sha256] duration = 0.127 seconds
 - PASS - [flash_map.test_flash_area_disabled_device] duration = 0.001 seconds
 - PASS - [flash_map.test_flash_area_erase_and_flatten] duration = 7.912 seconds
 - PASS - [flash_map.test_flash_area_erased_val] duration = 0.001 seconds
 - PASS - [flash_map.test_flash_area_get_sectors] duration = 0.886 seconds
 
------ TESTSUITE SUMMARY END ------
 
===================================================================
PROJECT EXECUTION SUCCESSFUL

@de-nordic
Copy link
Contributor Author

Hello @butok

I have intended to separately run SHA from other test to not wear down devices, where only sha is supposed to be tested, with unneeded write/erase operations.
The SHA tests is supposed to run once for mbedTLS and once for PSA, separately from other tests scenarios.

If that is somehow broken please report.

@butok
Copy link
Contributor

butok commented Nov 19, 2024

Hello @butok

I have intended to separately run SHA from other test to not wear down devices, where only sha is supposed to be tested, with unneeded write/erase operations. The SHA tests is supposed to run once for mbedTLS and once for PSA, separately from other tests scenarios.

If that is somehow broken please report.

OK. But there is no a test configuration in testcase.yaml to run the main.c tests.

@de-nordic
Copy link
Contributor Author

OK. But there is no a test configuration in testcase.yaml to run the main.c tests.

The default one does not run anymore?

@butok
Copy link
Contributor

butok commented Nov 19, 2024

OK. But there is no a test configuration in testcase.yaml to run the main.c tests.

The default one does not run anymore?

The default behavior has been described. Otherwise I wouldn't have added the comment ;)
#81091 (comment)

@de-nordic
Copy link
Contributor Author

OK. But there is no a test configuration in testcase.yaml to run the main.c tests.

The default one does not run anymore?

The default behavior has been described. Otherwise I wouldn't have added the comment ;) #81091 (comment)

I will take a look, but I am quite sure that I have 3 scenarios running each time a test the dir, which includes the base one and two separate sha tests.

@butok
Copy link
Contributor

butok commented Nov 19, 2024

OK. But there is no a test configuration in testcase.yaml to run the main.c tests.

The default one does not run anymore?

The default behavior has been described. Otherwise I wouldn't have added the comment ;) #81091 (comment)

I will take a look, but I am quite sure that I have 3 scenarios running each time a test the dir, which includes the base one and two separate sha tests.

CONFIG_FLASH_AREA_CHECK_INTEGRITY_MBEDTLS is Y for the default case, for me.

@de-nordic
Copy link
Contributor Author

The default behavior has been described. Otherwise I wouldn't have added the comment ;) #81091 (comment)

You are right, the fix is here: #81642

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Storage Storage subsystem

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants