Skip to content

Conversation

magp-nordic
Copy link
Contributor

No description provided.

@magp-nordic magp-nordic force-pushed the NRFX-8369-align-sdk-zephyr-to-bsp branch 2 times, most recently from 15d3d2b to f08b2ae Compare September 10, 2025 13:26
Copy link
Contributor

@tejlmand tejlmand left a comment

Choose a reason for hiding this comment

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

Don't start to apply compile definitions all over the place.

This is not an acceptable pattern.

Comment on lines 25 to 29
zephyr_compile_definitions(NRFX_BSP_NRF_PATH="${CONFIG_SOC_NORDIC_BSP_NAME}/mdk/nrf.h")
zephyr_compile_definitions(NRFX_BSP_ERRATAS_PATH="${CONFIG_SOC_NORDIC_BSP_NAME}/mdk/nrf_erratas.h")
zephyr_compile_definitions(NRFX_BSP_SOC_IRQS_PATH="${CONFIG_SOC_NORDIC_BSP_NAME}/soc/nrfx_irqs.h")
zephyr_compile_definitions(NRFX_BSP_NRFX_EXT_PATH="${CONFIG_SOC_NORDIC_BSP_NAME}/nrfx_ext.h")
zephyr_compile_definitions(NRFX_BSP_NRFX_COREDEP_PATH="${CONFIG_SOC_NORDIC_BSP_NAME}/soc/nrfx_coredep_defs.h")
Copy link
Contributor

@tejlmand tejlmand Sep 12, 2025

Choose a reason for hiding this comment

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

no, we should not start passing a bunch of compiler definitions to all source files.

This means even main.c is now compiled with:

$ /opt/zephyr-sdk-0.16.9/arm-zephyr-eabi/bin/arm-zephyr-eabi-gcc ...
 -DNRFX_BSP_ERRATAS_PATH=\"stable/mdk/nrf_erratas.h\"
 -DNRFX_BSP_NRFX_COREDEP_PATH=\"stable/soc/nrfx_coredep_defs.h\"
 -DNRFX_BSP_NRFX_EXT_PATH=\"stable/nrfx_ext.h\"
 -DNRFX_BSP_NRF_PATH=\"stable/mdk/nrf.h\"
 -DNRFX_BSP_SOC_IRQS_PATH=\"stable/soc/nrfx_irqs.h\"
  ... -c .../samples/hello_world/src/main.c

not to mention the fact that if you change the value of a compile definition, then all sources will rebuild on an incremental build because they now all have the compile definition applied.

Copy link
Contributor

Choose a reason for hiding this comment

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

This should not be an issue for targets in-tree, those defines have defaults that evaluate to stable path.
@magp-nordic can you add condition that does not set them if CONFIG_SOC_NORDIC_BSP_NAME is stable?

For out of tree targets, those defines will be needed and that is a design decision. Since development on those targets will be limited, it should be acceptable for that audience to live with a few additional defines.

Incremental rebuild should not be an issue since those Kconfigs are set once and never changed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@magp-nordic magp-nordic force-pushed the NRFX-8369-align-sdk-zephyr-to-bsp branch from 0f57108 to 73243ab Compare September 12, 2025 15:15
@magp-nordic magp-nordic force-pushed the NRFX-8369-align-sdk-zephyr-to-bsp branch 2 times, most recently from 73243ab to c2ec77a Compare September 17, 2025 14:15
magp-nordic and others added 4 commits September 24, 2025 11:02
Align paths after introducing BSP.

Upstream PR #: 96160

Signed-off-by: Magdalena Pastula <[email protected]>
Align paths after introducing BSP.

Upstream PR #: 96160

Signed-off-by: Magdalena Pastula <[email protected]>
Temporarily change path from hal_nordic to nrfx in BICR CMake.
This is needed, because BICR is in soc directory which is
included before modules, where NRFX_DIR symbol is defined.

Upstream PR #: 96160

Signed-off-by: Magdalena Pastula <[email protected]>
nrfx_uarte_rx is deprecated and will be soon removed.
Replaced by new API.

Upstream PR #: 96153

Signed-off-by: Michał Stasiak <[email protected]>
@magp-nordic magp-nordic force-pushed the NRFX-8369-align-sdk-zephyr-to-bsp branch from c2ec77a to df31c4a Compare September 24, 2025 09:02
@masz-nordic masz-nordic merged commit 0c9648f into nrfconnect:collab-nrfx-4.0 Sep 26, 2025
16 of 17 checks passed
@magp-nordic magp-nordic deleted the NRFX-8369-align-sdk-zephyr-to-bsp branch September 26, 2025 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants