softdevice: Move all SoftDevice files to separate folder #228

hermabe · 2025-08-01T14:16:22Z

Easier automated deploy
Clearer separation between sdk and SoftDevice

seko-nordic · 2025-08-07T14:57:44Z

tests/lib/ble_qwr/CMakeLists.txt

-cmock_handle(${ZEPHYR_NRF_BM_MODULE_DIR}/include/s115/ble.h)
-cmock_handle(${ZEPHYR_NRF_BM_MODULE_DIR}/include/s115/ble_gatts.h
+set(SOFTDEVICE_VARIANT "s115")
+set(SOFTDEVICE_INCLUDE_DIR "${ZEPHYR_NRF_BM_MODULE_DIR}/components/softdevice/${SOFTDEVICE_VARIANT}/${SOFTDEVICE_VARIANT}_API/include")


will it be easier to put API to components/softdevice/s115/API

We still need to know the variant to choose the correct include folder, so no big difference. But it might look nicer to not have the variant repeated as many times.

hermabe · 2025-08-08T12:48:39Z

@nrfconnect/ncs-bm @nrfconnect/ncs-co-build-system This is now ready for review

eivindj-nordic · 2025-08-10T12:17:15Z

subsys/softdevice/CMakeLists.txt

 #
 # SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
 #
+


Should this be moved to components/softdevice as well?
(together with Kconfig file)

I prefer to keep it outside of the components directory since all of it will be replaced on each update of the SoftDevice.

That said, there might be a better place to put the files than subsys/softdevice. I just kept them where they were for now.

Why replace the entire directory? Is it not enough to touch the componentes/softdevice/s115 directory? At least you should not touch more than componentes/softdevice, but if we stick to touch only the variant folders we can keep the Kconfig and CMake files here as well. I find it a bit strange and less intuitive to the user that those are in subsys while the rest is in components.

It will just replace components/softdevice, other components should not be touched. I want to replace the entire folder in case of future variants in addition to s115. It is possible to work around additional files in components/softdevice that shouldnt be replaced, replacing the whole thing is just simpler.

My idea for the components folder was that it is just a copy of external components and anything sdk-nrf-bm specific should be outside of it.

We already have a workaround to not replace CMakeLists.txt or Kconfig files for https://github.com/nrfconnect/sdk-nrfxlib/blob/main/softdevice_controller/CMakeLists.txt, so it is possible to keep the files in components/softdevice.

hermabe · 2025-08-12T13:51:02Z

@nrfconnect/ncs-co-build-system Please review

eivindj-nordic · 2025-08-13T06:27:35Z

@nrfconnect/ncs-bm-doc @nrfconnect/ncs-code-owners Please review

nordicjm · 2025-08-13T06:37:12Z

subsys/softdevice/CMakeLists.txt

 # SPDX-License-Identifier: LicenseRef-Nordic-5-Clause
 #
+
 if (CONFIG_SOFTDEVICE_S115)


no spaces between if and bracket in cmake

nordicjm · 2025-08-13T06:37:28Z

subsys/softdevice/CMakeLists.txt

+
 if (CONFIG_SOFTDEVICE_S115)
-  zephyr_include_directories(${ZEPHYR_NRF_BM_MODULE_DIR}/include/s115/)
+  set(SOFTDEVICE_VARIANT "s115")


this should come from Kconfig

Added a prompless kconfig that gives a variant string

nordicjm · 2025-08-13T06:38:19Z

tests/lib/ble_qwr/CMakeLists.txt


-cmock_handle(${ZEPHYR_NRF_BM_MODULE_DIR}/include/s115/ble.h)
-cmock_handle(${ZEPHYR_NRF_BM_MODULE_DIR}/include/s115/ble_gatts.h
+set(SOFTDEVICE_VARIANT "s115")


and should not duplicate variables unless there's a good reason

Kconfigs dont work for unittests since the dependencies are not satisfied. Kept it as is

- Easier automated deploy - Clearer separation between sdk and SoftDevice Signed-off-by: Herman Berget <[email protected]>

carlescufi

Why have the SoftDevice in components/ and then also in subsys/?

Why not just have it:
subsys/bluetooth/softdevice/ and have everything in there instead? This would be closer to the Zephyr/NCS structure

hermabe · 2025-08-13T08:24:11Z

Why have the SoftDevice in components/ and then also in subsys/?

The stuff in subsys is just the cmake. I havent found a better place to put it, agree that it is not ideal. Keeping the files where they are for now.

Why not just have it: subsys/bluetooth/softdevice/ and have everything in there instead? This would be closer to the Zephyr/NCS structure

Putting it in subsys/bluetooth would confuse things with the zephyr/ncs bluetooth subsystem which is completely separate. The SoftDevice is also not a zephyr subsystem so having it in subsys is a bit misleading. The components name is taken from nrf5 sdk.

carlescufi · 2025-08-13T13:02:41Z

Why not just have it: subsys/bluetooth/softdevice/ and have everything in there instead? This would be closer to the Zephyr/NCS structure

Putting it in subsys/bluetooth would confuse things with the zephyr/ncs bluetooth subsystem which is completely separate. The SoftDevice is also not a zephyr subsystem so having it in subsys is a bit misleading. The components name is taken from nrf5 sdk.

But this is NCS BM, so I wonder if it makes sense to consider it as it is, a different codebase that has its own subsys/. Or maybe just have bluetooth/ at the top or softdevice/ at the top? Not sure I am convinced we should follow the "components" scheme we had before.

hermabe · 2025-08-13T13:12:49Z

Why not just have it: subsys/bluetooth/softdevice/ and have everything in there instead? This would be closer to the Zephyr/NCS structure

Putting it in subsys/bluetooth would confuse things with the zephyr/ncs bluetooth subsystem which is completely separate. The SoftDevice is also not a zephyr subsystem so having it in subsys is a bit misleading. The components name is taken from nrf5 sdk.

But this is NCS BM, so I wonder if it makes sense to consider it as it is, a different codebase that has its own subsys/. Or maybe just have bluetooth/ at the top or softdevice/ at the top? Not sure I am convinced we should follow the "components" scheme we had before.

We dont have to follow the "components" scheme, it was just a convenient place to collect all the SoftDevice files. My goals with this PR were to collect all the files in one place for automating the update process, and to separate them from the rest of the sdk since these files are build outputs from a separate repo and not regular files that should be changed in this repo.

Unless there is a technical reason for the files to be placed somewhere else I would like to merge this PR so that we can continue with the automation work. The files can always be moved later if a reason for moving them shows up.

eivindj-nordic · 2025-08-14T14:12:23Z

Unless there is a technical reason for the files to be placed somewhere else I would like to merge this PR so that we can continue with the automation work. The files can always be moved later if a reason for moving them shows up

Ok by me.
@nordicjm @seko-nordic

MirkoCovizzi

Please note that AFAIK this will cause users to have to include #include <components/softdevice/s115/s115_API/include/nrf_error.h> if they want to use NRF_ERRORs, but that's ok for now I guess.

Edit:

Yes, when not enabling CONFIG_SOFTDEVICE (e.g. some peripheral libraries/samples).

hermabe · 2025-08-14T14:40:06Z

Please note that AFAIK this will cause users to have to include #include <components/softdevice/s115/s115_API/include/nrf_error.h> if they want to use NRF_ERRORs, but that's ok for now I guess.

Not sure what you mean? components/softdevice/s115/s115_API/include/ is added to the include path, so they should be able to just #include <nrf_error.h>? Is this related to #191?

eivindj-nordic · 2025-08-14T14:41:32Z

Please note that AFAIK this will cause users to have to include #include <components/softdevice/s115/s115_API/include/nrf_error.h> if they want to use NRF_ERRORs, but that's ok for now I guess.

Not sure what you mean? components/softdevice/s115/s115_API/include/ is added to the include path, so they should be able to just #include <nrf_error.h>? Is this related to #191?

@hermabe We have peripheral samples using nrf_error.h that does not have the SoftDevice enabled.

hermabe · 2025-08-14T15:01:44Z

Please note that AFAIK this will cause users to have to include #include <components/softdevice/s115/s115_API/include/nrf_error.h> if they want to use NRF_ERRORs, but that's ok for now I guess.

Not sure what you mean? components/softdevice/s115/s115_API/include/ is added to the include path, so they should be able to just #include <nrf_error.h>? Is this related to #191?

@hermabe We have peripheral samples using nrf_error.h that does not have the SoftDevice enabled.

There are currently no users in-tree so we can add something when it is needed. Potential solutions:

Add s115 headers to include path even if CONFIG_SOFTDEVICE=n.
Add a replacement nrf_error.h header that is in the include path if CONFIG_SOFTDEVICE=n
Dont use SoftDevice headers when the SoftDevice is not used

MirkoCovizzi · 2025-08-14T15:03:33Z

Please note that AFAIK this will cause users to have to include #include <components/softdevice/s115/s115_API/include/nrf_error.h> if they want to use NRF_ERRORs, but that's ok for now I guess.

Not sure what you mean? components/softdevice/s115/s115_API/include/ is added to the include path, so they should be able to just #include <nrf_error.h>? Is this related to #191?

@hermabe We have peripheral samples using nrf_error.h that does not have the SoftDevice enabled.

There are currently no users in-tree so we can add something when it is needed. Potential solutions:
1. Add s115 headers to include path even if `CONFIG_SOFTDEVICE=n`.

2. Add a replacement `nrf_error.h` header that is in the include path if `CONFIG_SOFTDEVICE=n`

3. Dont use SoftDevice headers when the SoftDevice is not used

We were considering point 2.

eivindj-nordic · 2025-08-14T15:33:38Z

Please note that AFAIK this will cause users to have to include #include <components/softdevice/s115/s115_API/include/nrf_error.h> if they want to use NRF_ERRORs, but that's ok for now I guess.

Not sure what you mean? components/softdevice/s115/s115_API/include/ is added to the include path, so they should be able to just #include <nrf_error.h>? Is this related to #191?

@hermabe We have peripheral samples using nrf_error.h that does not have the SoftDevice enabled.

There are currently no users in-tree so we can add something when it is needed. Potential solutions:
1. Add s115 headers to include path even if `CONFIG_SOFTDEVICE=n`.

2. Add a replacement `nrf_error.h` header that is in the include path if `CONFIG_SOFTDEVICE=n`

3. Dont use SoftDevice headers when the SoftDevice is not used
We were considering point 2.

Will go with a variant of option 2. Will add to storage PR if this is ready to merge before that. If not we'll need to add it here.

eivindj-nordic · 2025-08-15T06:30:22Z

It struck my mind if moving the SoftDevice includes from the include folder is really a good idea, will discuss offline.

eivindj-nordic · 2025-08-15T08:42:21Z

It struck my mind if moving the SoftDevice includes from the include folder is really a good idea, will discuss offline.

Discussed offline. We will continue with this approach and revisit later if needed.

hermabe force-pushed the softdevice/move_it branch 2 times, most recently from d05090d to 2405d50 Compare August 7, 2025 14:50

hermabe marked this pull request as ready for review August 7, 2025 14:50

hermabe requested review from a team as code owners August 7, 2025 14:50

seko-nordic reviewed Aug 7, 2025

View reviewed changes

hermabe changed the title ~~softdevice: Move all SoftDevice files to separate folder [DNM] [DNR]~~ softdevice: Move all SoftDevice files to separate folder Aug 8, 2025

eivindj-nordic reviewed Aug 10, 2025

View reviewed changes

hermabe added this to the v0.8.0 milestone Aug 12, 2025

anhmolt approved these changes Aug 12, 2025

View reviewed changes

nordicjm requested changes Aug 13, 2025

View reviewed changes

hermabe force-pushed the softdevice/move_it branch 2 times, most recently from 6f2adc1 to ebedea6 Compare August 13, 2025 07:53

softdevice: Move all SoftDevice files to separate folder

3455c3e

- Easier automated deploy - Clearer separation between sdk and SoftDevice Signed-off-by: Herman Berget <[email protected]>

hermabe force-pushed the softdevice/move_it branch from ebedea6 to 3455c3e Compare August 13, 2025 08:00

carlescufi reviewed Aug 13, 2025

View reviewed changes

hermabe requested a review from nordicjm August 13, 2025 08:24

eivindj-nordic approved these changes Aug 14, 2025

View reviewed changes

b-gent approved these changes Aug 14, 2025

View reviewed changes

MirkoCovizzi approved these changes Aug 14, 2025

View reviewed changes

eivindj-nordic self-requested a review August 15, 2025 06:26

nordicjm approved these changes Aug 15, 2025

View reviewed changes

eivindj-nordic merged commit 9be9681 into nrfconnect:main Aug 15, 2025
9 checks passed

Uh oh!

softdevice: Move all SoftDevice files to separate folder #228

softdevice: Move all SoftDevice files to separate folder #228

Uh oh!

Conversation

hermabe commented Aug 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hermabe commented Aug 8, 2025

Uh oh!

eivindj-nordic Aug 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hermabe commented Aug 12, 2025

Uh oh!

eivindj-nordic commented Aug 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carlescufi left a comment

Choose a reason for hiding this comment

Uh oh!

hermabe commented Aug 13, 2025

Uh oh!

carlescufi commented Aug 13, 2025

Uh oh!

hermabe commented Aug 13, 2025

Uh oh!

eivindj-nordic commented Aug 14, 2025

Uh oh!

MirkoCovizzi left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hermabe commented Aug 14, 2025

Uh oh!

eivindj-nordic commented Aug 14, 2025

Uh oh!

hermabe commented Aug 14, 2025

Uh oh!

MirkoCovizzi commented Aug 14, 2025

Uh oh!

eivindj-nordic commented Aug 14, 2025

Uh oh!

eivindj-nordic commented Aug 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eivindj-nordic commented Aug 15, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

eivindj-nordic Aug 10, 2025 •

edited

Loading

MirkoCovizzi left a comment •

edited

Loading

eivindj-nordic commented Aug 15, 2025 •

edited

Loading