Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

sdp: cmake: soc-specific files #19095

Merged
merged 9 commits into from
Feb 4, 2025
Merged

sdp: cmake: soc-specific files #19095

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
a423bee
cmake: sdp: add common wrapper for all assembly related cmake
masz-nordic Nov 25, 2024
4b34360
cmake: sdp: generate and check separate files for each SoC
masz-nordic Nov 25, 2024
c6fd911
cmake: sdp: docs for CMake functions
masz-nordic Jan 17, 2025
4285be9
applications: sdp: gpio: align to cmake wrapper
masz-nordic Nov 25, 2024
91550c8
applications: sdp: mspi: align to cmake wrapper
masz-nordic Dec 13, 2024
859691c
tests: drivers: sdp_asm: align to cmake wrapper
masz-nordic Jan 17, 2025
2fcb2b8
applications: sdp: gpio: align to per-SoC assembly file
masz-nordic Nov 25, 2024
24a9627
applications: sdp: mspi: align to per-SoC assembly file
masz-nordic Dec 13, 2024
d2fa56c
tests: drivers: align to per-SoC assembly file
masz-nordic Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions applications/sdp/gpio/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,15 +9,10 @@ cmake_minimum_required(VERSION 3.20.0)
find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
project(sdp_gpio)

sdp_assembly_generate("${CMAKE_SOURCE_DIR}/src/hrt/hrt.c")
sdp_assembly_check("${CMAKE_SOURCE_DIR}/src/hrt/hrt.c")
sdp_assembly_prepare_install("${CMAKE_SOURCE_DIR}/src/hrt/hrt.c")
sdp_assembly_install(app "${CMAKE_SOURCE_DIR}/src/hrt/hrt.c")

target_sources(app PRIVATE src/main.c)
target_sources(app PRIVATE src/hrt/hrt.s)

target_sources_ifdef(CONFIG_SDP_GPIO_BACKEND_ICMSG app PRIVATE src/backend/backend_icmsg.c)
target_sources_ifdef(CONFIG_SDP_GPIO_BACKEND_ICBMSG app PRIVATE src/backend/backend_icmsg.c)
target_sources_ifdef(CONFIG_SDP_GPIO_BACKEND_MBOX app PRIVATE src/backend/backend_mbox.c)

add_dependencies(app asm_check)
0 applications/sdp/gpio/src/hrt/hrt.s → applications/sdp/gpio/src/hrt/hrt-nrf54l15.s
View file
Open in desktop
File renamed without changes.
7 changes: 1 addition & 6 deletions applications/sdp/mspi/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,6 @@ cmake_minimum_required(VERSION 3.20.0)
find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
project(sdp_mspi)

sdp_assembly_generate("${CMAKE_SOURCE_DIR}/src/hrt/hrt.c")
sdp_assembly_check("${CMAKE_SOURCE_DIR}/src/hrt/hrt.c")
sdp_assembly_prepare_install("${CMAKE_SOURCE_DIR}/src/hrt/hrt.c")
sdp_assembly_install(app "${CMAKE_SOURCE_DIR}/src/hrt/hrt.c")

target_sources(app PRIVATE src/main.c)
target_sources(app PRIVATE src/hrt/hrt.s)

add_dependencies(app asm_check)
0 applications/sdp/mspi/src/hrt/hrt.s → applications/sdp/mspi/src/hrt/hrt-nrf54l15.s
View file
Open in desktop
File renamed without changes.
44 changes: 36 additions & 8 deletions cmake/sdp.cmake
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

When addressing review comments, then please squash the changes into the commit which originally introduced the code that was commented.

For example, the commit:
0d52442 still has: function(sdp_assembly_install hrt_srcs)
but later commit: 05afd9a changes this to: function(sdp_assembly_install target hrt_srcs)

This makes reviewing PRs commit-by-commit harder because I don't know if the code in commit A which i'm currently reviewing and commenting will be modified later at commit Z.

Not mentioning that making two commits in a PR (introduce function + changes the same function) instead of a single commit makes reviewing slower. (Multiple commit are good when they have different kind of changes, that is not touching the same codelines).

Note: Non blocking comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, my intention was to make separate commits for each functional change. Will squash them.

Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,35 @@
# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
#

function(sdp_assembly_generate hrt_srcs)
# This function takes as an argument target to be built and path to .c source file(s)
# whose .s output must be verified against .s file tracked in repository. It then:
# - adds a pre-build dependency that generates .s files in build directory
# - adds a dependency on specified target to call sdp_asm_check.cmake with appropriate arguments
# - adds a custom target that calls sdp_asm_install.cmake with appropriate arguments
# - adds .s files from build directory to target sources
#
# Arguments:
# target - target to which the dependencies are to be applied
# hrt_srcs - path to the .c source file(s) to verify
function(sdp_assembly_install target hrt_srcs)
sdp_assembly_generate(${CONFIG_SOC} "${hrt_srcs}")
sdp_assembly_check(${CONFIG_SOC} "${hrt_srcs}")
sdp_assembly_prepare_install(${CONFIG_SOC} "${hrt_srcs}")
sdp_assembly_target_sources(${CONFIG_SOC} ${target} "${hrt_srcs}")

Copy link
Contributor

Choose a reason for hiding this comment

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

Why the empty line here, and before other functions ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just my formatting preference, removed.

add_dependencies(${target} asm_check)
endfunction()

function(sdp_assembly_target_sources soc target hrt_srcs)
foreach(hrt_src ${hrt_srcs})
get_filename_component(hrt_dir ${hrt_src} DIRECTORY) # directory
get_filename_component(hrt_src_file ${hrt_src} NAME_WE) # filename without extension
set(hrt_s_file "${hrt_dir}/${hrt_src_file}-${soc}.s")
target_sources(${target} PRIVATE ${hrt_s_file})
endforeach()
endfunction()

function(sdp_assembly_generate soc hrt_srcs)
set(hrt_msg "Generating ASM files for Hard Real Time files.")
set(hrt_opts -g0 -fno-ident -fno-verbose-asm)

Expand Down Expand Up @@ -38,9 +66,9 @@ function(sdp_assembly_generate hrt_srcs)
get_filename_component(src_filename ${hrt_src} NAME_WE) # filename without extension
add_custom_command(TARGET asm_gen
PRE_BUILD
BYPRODUCTS ${src_filename}-temp.s
COMMAND ${CMAKE_C_COMPILER} ${compiler_options} ${hrt_opts} -S ${hrt_src} -o ${src_filename}-temp.s
COMMAND ${PYTHON_EXECUTABLE} ${ZEPHYR_NRF_MODULE_DIR}/scripts/sdp/remove_comments.py ${src_filename}-temp.s
BYPRODUCTS ${src_filename}-${soc}-temp.s
COMMAND ${CMAKE_C_COMPILER} ${compiler_options} ${hrt_opts} -S ${hrt_src} -o ${src_filename}-${soc}-temp.s
COMMAND ${PYTHON_EXECUTABLE} ${ZEPHYR_NRF_MODULE_DIR}/scripts/sdp/remove_comments.py ${src_filename}-${soc}-temp.s
COMMAND_EXPAND_LISTS
COMMENT "Generating ASM file for ${hrt_src}"
)
Expand All @@ -50,7 +78,7 @@ function(sdp_assembly_generate hrt_srcs)

endfunction()

function(sdp_assembly_check hrt_srcs)
function(sdp_assembly_check soc hrt_srcs)
set(asm_check_msg "Checking if ASM files for Hard Real Time have changed.")

if(TARGET asm_check)
Expand All @@ -62,15 +90,15 @@ function(sdp_assembly_check hrt_srcs)
string(REPLACE ";" "$<SEMICOLON>" hrt_srcs_semi "${hrt_srcs}")

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

add_dependencies(asm_check asm_gen)

endfunction()

function(sdp_assembly_prepare_install hrt_srcs)
function(sdp_assembly_prepare_install soc hrt_srcs)
set(asm_install_msg "Updating ASM files for Hard Real Time.")

if(TARGET asm_install)
Expand All @@ -82,7 +110,7 @@ function(sdp_assembly_prepare_install hrt_srcs)
string(REPLACE ";" "$<SEMICOLON>" hrt_srcs_semi "${hrt_srcs}")

add_custom_target(asm_install
COMMAND ${CMAKE_COMMAND} -D hrt_srcs="${hrt_srcs_semi}" -P ${ZEPHYR_NRF_MODULE_DIR}/cmake/sdp_asm_install.cmake
COMMAND ${CMAKE_COMMAND} -D hrt_srcs="${hrt_srcs_semi}" -D soc="${soc}" -P ${ZEPHYR_NRF_MODULE_DIR}/cmake/sdp_asm_install.cmake
COMMENT ${asm_install_msg}
)

Expand Down
16 changes: 12 additions & 4 deletions cmake/sdp_asm_check.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,24 +4,32 @@
# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
#

# Function checking if .s file generated during build is identical to one tracked in repository
#
# Arguments:
# hrt_srcs - path to the .c file(s) to verify
# soc - name of the SoC the code is being built for
function(asm_check)
if(NOT DEFINED soc)
message(FATAL_ERROR "asm_check missing soc argument.")
endif()

foreach(hrt_src ${hrt_srcs})
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 --ignore-eol
${src_dir}/${asm_filename}.s ${asm_filename}-temp.s
${src_dir}/${asm_filename}-${soc}.s ${asm_filename}-${soc}-temp.s
RESULT_VARIABLE compare_result)

if( compare_result EQUAL 0)
message("File ${asm_filename}.s has not changed.")
message("File ${asm_filename}-${soc}.s has not changed.")
elseif( compare_result EQUAL 1)
message(WARNING "${asm_filename}.s ASM file content has changed.\
message(WARNING "${asm_filename}-${soc}.s ASM file content has changed.\
If you want to include the new ASM in build, \
please run `ninja asm_install` in FLPR build directory and build again")
else()
message("Something went wrong when comparing ${asm_filename}.s and ${asm_filename}-temp.s")
message("Something went wrong when comparing ${asm_filename}-${soc}.s and ${asm_filename}-${soc}-temp.s")
endif()
endforeach()

Expand Down
14 changes: 11 additions & 3 deletions cmake/sdp_asm_install.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,26 @@
# SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
#

# Function that replaces .s file tracked in repository with updated version from build
#
# Arguments:
# hrt_srcs - path to the .c file(s) to verify
# soc - name of the SoC the code is being built for
function(asm_install)
if(NOT DEFINED soc)
message(FATAL_ERROR "asm_install missing soc argument.")
endif()

foreach(hrt_src ${hrt_srcs})
get_filename_component(asm_filename ${hrt_src} NAME_WE) # filename without extension
get_filename_component(src_dir ${hrt_src} DIRECTORY)

file(RENAME ${asm_filename}-temp.s ${src_dir}/${asm_filename}.s RESULT rename_result)
file(RENAME ${asm_filename}-${soc}-temp.s ${src_dir}/${asm_filename}-${soc}.s RESULT rename_result)

if (rename_result EQUAL 0)
message("Updated ${asm_filename}.s ASM file.")
message("Updated ${asm_filename}-${soc}.s ASM file.")
else()
message(WARNING "${asm_filename}.s cannot be updated, new ASM does not exist. Please run ninja asm_gen to create one.")
message(WARNING "${asm_filename}-${soc}.s cannot be updated, new ASM does not exist. Please run ninja asm_gen to create one.")
endif()

endforeach()
Expand Down
17 changes: 8 additions & 9 deletions tests/drivers/sdp_asm/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,14 @@
cmake_minimum_required(VERSION 3.20.0)

find_package(Zephyr REQUIRED HINTS $ENV{ZEPHYR_BASE})
project(hello_world)
project(sdp_asm)

sdp_assembly_generate("${CMAKE_CURRENT_SOURCE_DIR}/src/add_1.c;${CMAKE_CURRENT_SOURCE_DIR}/src/add_10.c;${CMAKE_CURRENT_SOURCE_DIR}/src/add_100.c")
sdp_assembly_check("${CMAKE_CURRENT_SOURCE_DIR}/src/add_1.c;${CMAKE_CURRENT_SOURCE_DIR}/src/add_10.c;${CMAKE_CURRENT_SOURCE_DIR}/src/add_100.c")
sdp_assembly_prepare_install("${CMAKE_CURRENT_SOURCE_DIR}/src/add_1.c;${CMAKE_CURRENT_SOURCE_DIR}/src/add_10.c;${CMAKE_CURRENT_SOURCE_DIR}/src/add_100.c")
set(sdp_files
${CMAKE_CURRENT_SOURCE_DIR}/src/add_1.c
${CMAKE_CURRENT_SOURCE_DIR}/src/add_10.c
${CMAKE_CURRENT_SOURCE_DIR}/src/add_100.c
)

target_sources(app PRIVATE src/main.c)
target_sources(app PRIVATE src/add_1.s)
target_sources(app PRIVATE src/add_10.s)
target_sources(app PRIVATE src/add_100.s)
sdp_assembly_install(app "${sdp_files}")

add_dependencies(app asm_check)
target_sources(app PRIVATE src/main.c)
6 changes: 3 additions & 3 deletions tests/drivers/sdp_asm/pytest/test_sdp_asm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,9 +48,9 @@ def test_sdp_asm(dut: DeviceAdapter):
logger.info("# Confirm that build log contains warnings")
with open(f"{build_log_file}", errors="ignore") as log_file:
log_file_content = log_file.read()
assert "add_1.s ASM file content has changed" in log_file_content
assert "File add_10.s has not changed" in log_file_content
assert "add_100.s ASM file content has changed" in log_file_content
assert "add_1-nrf54l15.s ASM file content has changed" in log_file_content
assert "File add_10-nrf54l15.s has not changed" in log_file_content
assert "add_100-nrf54l15.s ASM file content has changed" in log_file_content

for _ in range(2):
logger.info("# Install newly compiled .S files")
Expand Down
0 tests/drivers/sdp_asm/src/add_1.s → tests/drivers/sdp_asm/src/add_1-nrf54l15.s
View file
Open in desktop
File renamed without changes.
0 tests/drivers/sdp_asm/src/add_10.s → tests/drivers/sdp_asm/src/add_10-nrf54l15.s
View file
Open in desktop
File renamed without changes.
0 tests/drivers/sdp_asm/src/add_100.s → tests/drivers/sdp_asm/src/add_100-nrf54l15.s
View file
Open in desktop
File renamed without changes.