Nrfx 6388 assembly mgmt west build #18018

magp-nordic · 2024-10-18T16:48:00Z

Add two new CMake targets: asm_check and asm_update.

asm_check will check if content of the already existing ASM file and the new generated by asm_gen are the same. If they are the same, the newly generated is removed. If they are different, a warning is printed to update ASM file, if the new one should be included in build.

asm_update will replace the already existing ASM file with the new one generated by asm_gen. If the new ASM file does not exist, a warning is printed.

I created two new CMake scripts (sdp_asm_check and sdp_asm_update) for these targets because I could not find a way to pass the logic to add_custom_target directly/in line. If there is a way of doing that without creating new CMake scripts, please let me know.

In the last commit asm_check is added as a dependency for flpr_egpio application. It is done in order to check ASM files automatically at the start of the build, no matter if a sysbuild application including eGPIO is build or eGPIO firmware only.

~~Based on #17638~~ Rebased on main

NordicBuilder · 2024-10-18T16:50:56Z

CI Information

^{To view the history of this post, clich the 'edited' button above}
Build number: 17

Inputs:

Sources:

sdk-nrf: PR head: b4b3ec1a3ff321bf9d965e0c6010fd41e160aa1d

more details

sdk-nrf:

PR head: b4b3ec1a3ff321bf9d965e0c6010fd41e160aa1d
merge base: 11645184a54d02921bb0691a57e1e86c141393da
target head (main): 74ec8d27eb9ac759e32080753dc883fd82402159
Diff

Github labels

Enabled	Name	Description
	ci-disabled	Disable the ci execution
	ci-all-test	Run all of ci, no test spec filtering will be done
	ci-force-downstream	Force execution of downstream even if twister fails
	ci-run-twister	Force run twister
	ci-run-zephyr-twister	Force run zephyr twister

List of changed files detected by CI (4)

applications
│  ├── sdp
│  │  ├── gpio
│  │  │  │ CMakeLists.txt
cmake
│  ├── sdp.cmake
│  ├── sdp_asm_check.cmake
│  │ sdp_asm_install.cmake

Outputs:

Toolchain

Version: add720b6d9
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:add720b6d9_912848a074

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

◻️ Toolchain - Skipped: existing toolchain is used
✅ Build twister - Skipped: Skipping Build & Test as it succeeded in a previous run: 15
✅ Integration tests
- ✅ test-sdk-audio - Skipped: Job was skipped as it succeeded in a previous run
- ✅ desktop52_verification - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-boot - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-apps - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test_ble_nrf_config - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-ble_mesh - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-ble_samples - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-chip - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nfc - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_cloud - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_libmodem-nrf - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_zephyr_lwm2m - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_samples - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_lwm2m - Skipped: Job was skipped as it succeeded in a previous run
- ✅ doc-internal - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_thingy91 - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf_crypto - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-proprietary_esb - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-rpc - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-rs - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-fem - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-tfm - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-thread - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-zigbee - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-sdk-find-my - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_mosh - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_positioning - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-sdk-sidewalk - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-sdk-wifi - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-low-level - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-nrf-iot_nrf_provisioning - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-sdk-pmic-samples - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-sdk-mcuboot
- ✅ test-sdk-dfu - Skipped: Job was skipped as it succeeded in a previous run
- ✅ test-fw-nrfconnect-ps - Skipped: Job was skipped as it succeeded in a previous run

Note: This message is automatically posted and updated by the CI

NordicBuilder · 2024-10-18T17:05:12Z

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

magp-nordic · 2024-10-22T11:23:32Z

Since hrt.s was removed from #17638 , these changes will break building anything with eGPIO until #17984 is merged and this PR is rebased.

magp-nordic · 2024-10-23T11:37:47Z

@nrfconnect/ncs-co-build-system @nrfconnect/ncs-code-owners @nrfconnect/ncs-ll-ursus please, review, PR is already rebased on main

cmake/sdp_asm_update.cmake

cmake/sdp.cmake

tejlmand · 2024-10-23T21:23:34Z

please rebase against main, hrt.s is missing in this PR (exists on main), which causes the following warning to be printed on each build:

[1/2] Running utility command for asm_gen
File hrt-temp.s has been cleaned of comments.
[2/2] Checking if ASM files for Hard Real Time have changed.
CMake Warning at /projects/github/review/nrf/cmake/sdp_asm_check.cmake:19 (message):
  hrt.s ASM file content has changed.  If you want to include the new ASM in
  build, please run `ninja asm_update` in FLPR build directory and build
  again
Call Stack (most recent call first):
  /projects/github/review/nrf/cmake/sdp_asm_check.cmake:29 (asm_check)

tejlmand · 2024-10-23T21:35:26Z

cmake/sdp.cmake

+  add_custom_target(asm_check
+    COMMAND ${CMAKE_COMMAND} -D hrt_srcs=${hrt_srcs} -P ${ZEPHYR_NRF_MODULE_DIR}/cmake/sdp_asm_check.cmake
+    COMMAND_EXPAND_LISTS
+    COMMENT ${asm_check_msg}
+  )


please ensure that this check has is only carried out when there are actually changes to check.
With the current approach, then the asm_check is always considered out-of-date, and thus the target is always invoked.

For example doing an incremental build on main after a successful build gives:

$ ninja ... -- Zephyr version: 3.7.99 (/projects/github/review/zephyr), build: v3.7.0-4866-ge516ad354c34 [96/96] Linking C executable zephyr/zephyr.elf Memory region Used Size Region Size %age Used RAM: 7080 B 12 KB 57.62% IDT_LIST: 0 GB 2 KB 0.00% Generating files from /projects/github/review/nrf/applications/sdp/gpio/build/zephyr/zephyr.elf for board: nrf54l15dk $ ninja ninja: no work to do.

however, with this PR, the following is seen:

$ ninja ... [98/98] Linking C executable zephyr/zephyr.elf Memory region Used Size Region Size %age Used RAM: 7080 B 12 KB 57.62% IDT_LIST: 0 GB 2 KB 0.00% Generating files from /projects/github/review/nrf/applications/sdp/gpio/build/zephyr/zephyr.elf for board: nrf54l15dk $ ninja [1/2] Running utility command for asm_gen File hrt-temp.s has been cleaned of comments. [2/2] Checking if ASM files for Hard Real Time have changed. File hrt.s has not changed. $ ninja [1/2] Running utility command for asm_gen File hrt-temp.s has been cleaned of comments. [2/2] Checking if ASM files for Hard Real Time have changed. File hrt.s has not changed.

Feel free to make dm on how to fix this.

@tejlmand is this something that can also be fixed later, after NCS release? I think this is just a minor issue, and I cannot find a solution for it. Unless you have some quick fix in mind.

Created ticket: https://nordicsemi.atlassian.net/browse/NRFX-6664

tejlmand · 2024-10-23T21:39:18Z

cmake/sdp_asm_check.cmake

+    get_filename_component(asm_filename ${hrt_src} NAME_WE)  # filename without extension
+    get_filename_component(src_dir ${hrt_src} DIRECTORY)
+
+    execute_process(COMMAND ${CMAKE_COMMAND} -E compare_files ${src_dir}/${asm_filename}.s ${asm_filename}-temp.s


has it been verified that the hrt.s (and future friends) which are committed to git has the same line endings on windows when generated by gcc on windows ?

If it has been verified, then everything is fine, else please verify.
And if line endings differs, then compare_files supports an --ignore-eol flag.

tejlmand · 2024-10-23T21:50:29Z

cmake/sdp_asm_check.cmake

this comment covers both this file and sdp_asm_update.cmake.

As you have already made functions in each script, then I think t would be nice to combine the two scripts into a single one.

You can then call the CMake script as: cmake -Dcompare-files=${hrt_srcs} -P sdp_asm.cmake or cmake -Dinstall-files=${hrt_srcs} -P sdp_asm.cmake and based on the argument then call te respective internal script function.

Note, this would be nice for this PR, but is not blocking. If not refactoring during PR. then please create a followup ticket.

Created ticket: https://nordicsemi.atlassian.net/browse/NRFX-6658

tejlmand · 2024-10-23T22:23:23Z

applications/sdp/gpio/CMakeLists.txt

@@ -10,8 +10,12 @@ find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
 project(emulated_gpio)

 sdp_assembly_generate("${CMAKE_SOURCE_DIR}/src/hrt/hrt.c")
+sdp_assembly_check("${CMAKE_SOURCE_DIR}/src/hrt/hrt.c")
+sdp_assembly_update("${CMAKE_SOURCE_DIR}/src/hrt/hrt.c")


First, this call is misleading / bad naming.

It gives the impression that the newly generated assembly file will actually be updated, which is not the case.
This call will only create a build target which can be called by the user to update the assembly files.

Another problem with the approach is that all files will be updated, and not only those which differs.
Or the user cannot choose to update only a single file.

This is not a big issue when only a single file is in use, but will become a problem in future.

For now, please improve the naming.
For future, please create a ticket to addre

I changed sdp_assembly_update to sdp_assembly_prepare_install.
Also, I created a ticket for modifying asm_update (asm_install now): https://nordicsemi.atlassian.net/browse/NRFX-6657

tejlmand · 2024-10-23T22:17:53Z

cmake/sdp.cmake

+    COMMAND ${CMAKE_COMMAND} -D hrt_srcs=${hrt_srcs} -P ${ZEPHYR_NRF_MODULE_DIR}/cmake/sdp_asm_check.cmake
+    COMMAND_EXPAND_LISTS


this does not work with lists.

First thing is that ; will be replaced with <space>, thus resulting in: -D hrt_srcs=foo.c bar.c, and not -D hrt_srcs="foo.c;bar.c".

To fix this, then first COMMAND_EXPAND_LISTS must be removed.
Secondly, ; must be replaced by the $<SEMICOLON> generator expression.
and finally, the argument list must be quoted using ".

tejlmand · 2024-10-23T22:24:27Z

cmake/sdp.cmake

+function(sdp_assembly_update hrt_srcs)
+  set(asm_update_msg "Updating ASM files for Hard Real Time.")
+
+  if(TARGET asm_update)


A common naming convention for such targets is install-<something>, not update, so perhaps: install-sdp-asm / install-hrt-asm are better names.

I changed it to asm_install to stay consistent with other target names.

tejlmand · 2024-10-23T22:25:42Z

cmake/sdp.cmake

+  endif()
+
+  add_custom_target(asm_update
+    COMMAND ${CMAKE_COMMAND} -D hrt_srcs=${hrt_srcs} -P ${ZEPHYR_NRF_MODULE_DIR}/cmake/sdp_asm_update.cmake


see other comment regarding this code not working with lists.

Add two new targets regarding ASM code for SDP: asm_check that checks if ASM file has changed, and asm_update that updates ASM files in source directory with the new generated. Signed-off-by: Magdalena Pastula <[email protected]>

Add asm_check target as a depency for flpr_egpio application. This will generate new ASM file for every source file in hrt folder and check if the new ones are the same as the one saved in git. For every file that differs a warning is printed. By making asm_check a dependency of flpr_egpio application asm_check will be run both for sysbuild application including eGPIO and for stand-alone SDP application before the main build starts. Signed-off-by: Magdalena Pastula <[email protected]>

tejlmand

approved, as there are issues to track followup work

magp-nordic requested review from a team as code owners October 18, 2024 16:48

magp-nordic marked this pull request as draft October 18, 2024 16:48

magp-nordic mentioned this pull request Oct 18, 2024

applications: sdp: Add asm_gen target #17638

Merged

masz-nordic added this to the 2.8.0 milestone Oct 21, 2024

magp-nordic marked this pull request as ready for review October 21, 2024 14:14

magp-nordic requested review from masz-nordic and jaz1-nordic October 21, 2024 14:15

magp-nordic force-pushed the NRFX-6388-assembly-mgmt-west-build branch 2 times, most recently from e737c06 to b00d36d Compare October 22, 2024 11:06

magp-nordic force-pushed the NRFX-6388-assembly-mgmt-west-build branch 3 times, most recently from a468a22 to 7204983 Compare October 23, 2024 10:57

github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Oct 23, 2024

masz-nordic reviewed Oct 23, 2024

View reviewed changes

cmake/sdp_asm_update.cmake Outdated Show resolved Hide resolved

cmake/sdp.cmake Outdated Show resolved Hide resolved

magp-nordic force-pushed the NRFX-6388-assembly-mgmt-west-build branch from 7204983 to 02a7fb2 Compare October 23, 2024 14:05

masz-nordic approved these changes Oct 23, 2024

View reviewed changes

jaz1-nordic approved these changes Oct 23, 2024

View reviewed changes

tejlmand requested changes Oct 23, 2024

View reviewed changes

magp-nordic force-pushed the NRFX-6388-assembly-mgmt-west-build branch from 02a7fb2 to 874f156 Compare October 24, 2024 09:31

magp-nordic requested a review from tejlmand October 24, 2024 09:53

magp-nordic force-pushed the NRFX-6388-assembly-mgmt-west-build branch from 874f156 to 1e37564 Compare October 24, 2024 10:00

magp-nordic added 2 commits October 25, 2024 09:17

cmake: add two new targets for SDP

58ed105

Add two new targets regarding ASM code for SDP: asm_check that checks if ASM file has changed, and asm_update that updates ASM files in source directory with the new generated. Signed-off-by: Magdalena Pastula <[email protected]>

magp-nordic force-pushed the NRFX-6388-assembly-mgmt-west-build branch from 1e37564 to b4b3ec1 Compare October 25, 2024 07:17

masz-nordic added the backport v2.8-branch auto-create a PR with same commits to v2.8-branch label Oct 25, 2024

tejlmand approved these changes Oct 25, 2024

View reviewed changes

carlescufi merged commit b24793a into nrfconnect:main Oct 25, 2024
15 checks passed

NordicBuilder mentioned this pull request Oct 25, 2024

[Backport v2.8-branch] Nrfx 6388 assembly mgmt west build #18221

Merged

magp-nordic deleted the NRFX-6388-assembly-mgmt-west-build branch October 25, 2024 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Nrfx 6388 assembly mgmt west build #18018

Nrfx 6388 assembly mgmt west build #18018

magp-nordic commented Oct 18, 2024 •

edited

Loading

NordicBuilder commented Oct 18, 2024 •

edited

Loading

NordicBuilder commented Oct 18, 2024

magp-nordic commented Oct 22, 2024 •

edited

Loading

magp-nordic commented Oct 23, 2024

tejlmand commented Oct 23, 2024

tejlmand Oct 23, 2024

magp-nordic Oct 24, 2024

magp-nordic Oct 25, 2024

tejlmand Oct 23, 2024

tejlmand Oct 23, 2024

magp-nordic Oct 24, 2024

tejlmand Oct 23, 2024

magp-nordic Oct 24, 2024

tejlmand Oct 23, 2024

tejlmand Oct 23, 2024

magp-nordic Oct 24, 2024

tejlmand Oct 23, 2024

tejlmand left a comment

		COMMAND ${CMAKE_COMMAND} -D hrt_srcs=${hrt_srcs} -P ${ZEPHYR_NRF_MODULE_DIR}/cmake/sdp_asm_check.cmake
		COMMAND_EXPAND_LISTS

Nrfx 6388 assembly mgmt west build #18018

Nrfx 6388 assembly mgmt west build #18018

Conversation

magp-nordic commented Oct 18, 2024 • edited Loading

NordicBuilder commented Oct 18, 2024 • edited Loading

CI Information

Inputs:

Sources:

Github labels

Outputs:

Toolchain

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

NordicBuilder commented Oct 18, 2024

magp-nordic commented Oct 22, 2024 • edited Loading

magp-nordic commented Oct 23, 2024

tejlmand commented Oct 23, 2024

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

tejlmand left a comment

Choose a reason for hiding this comment

magp-nordic commented Oct 18, 2024 •

edited

Loading

NordicBuilder commented Oct 18, 2024 •

edited

Loading

magp-nordic commented Oct 22, 2024 •

edited

Loading