sdh: simplifications and improvements #388

lemrey · 2025-09-23T12:38:59Z

Introduce observer priority levels, e.g. HIGHEST, HIGH, USER, USER_LOW, LOWEST
Remove NRF_SDH_BLE (no longer configurable)
Remove nrf_sdh_app_ram_start_get(), unused and not useful
Remove nrf_sdh_is_enabled() in favor of native SoftDevice function
Merge request observers with state observers
Remove nrf_sdh_request_continue()
Improve functions for stringifying events and export them
Split random seed request handling to a dedicated file
Put some Kconfigs behind menus

github-actions · 2025-09-23T12:40:21Z

You can find the documentation preview for this PR here.

anhmolt · 2025-09-26T12:14:38Z

subsys/softdevice_handler/Kconfig

+endmenu
+
+config NRF_SDH_SOC_RAND_SEED
+	bool "Automatically seed SoftDevice random seeds requests"


Suggested change

bool "Automatically seed SoftDevice random seeds requests"

bool "Automatically respond to SoftDevice random seed requests"

Ok, reworded

anhmolt · 2025-09-26T12:42:21Z

subsys/softdevice_handler/Kconfig

+	bool "Automatically seed SoftDevice random seeds requests"
+	depends on NRF_SECURITY
+	depends on MBEDTLS_PSA_CRYPTO_C
+	depends on PSA_WANT_GENERATE_RANDOM


I realize now that PSA_WANT_GENERATE_RANDOM is not required after we changed from using psa_generate_random() to using cracen_get_trng(). (PRNG to TRNG.)
So the PSA_WANT_GENERATE_RANDOM dependency should be removed from here.

Thanks, removed it

anhmolt · 2025-09-26T13:29:46Z

subsys/softdevice_handler/Kconfig

-	  Make prints nicer by printing the stringified version of certain enumerations.
-	  Disable to save non-volatile memory.
+	  Compile a table of stringified SoftDevice events for nrf_sdh_ble_evt_tostr() and
+	  nrf_sdh_soc_evt_tostr().  Disable to save non-volatile memory.


Nit

Suggested change

nrf_sdh_soc_evt_tostr(). Disable to save non-volatile memory.

nrf_sdh_soc_evt_tostr(). Disable to save non-volatile memory.

anhmolt · 2025-09-29T08:15:59Z

subsys/softdevice_handler/rand_seed.c

+
+	status = cracen_get_trng(seed, sizeof(seed));
+	if (status != PSA_SUCCESS) {
+		LOG_ERR("Failed to generate true random number, psa_status %#x", status);


PSA error values uses 0 for success and negative integers for errors. Similar to errno.
I think format %d will be more readable.

Suggested change

LOG_ERR("Failed to generate true random number, psa_status %#x", status);

LOG_ERR("Failed to generate true random number, psa_status %d", status);

You are right, I think I checked and saw they were hex values but I checked again and they are not. So I am reverting this to %d as it was before.

anhmolt · 2025-09-29T08:21:37Z

subsys/softdevice_handler/rand_seed.c

+	/* Discard seed immediately */
+	memset(seed, 0, sizeof(seed));
+
+	if (nrf_err) {


Could be more explicit when checking the error here.

Suggested change

if (nrf_err) {

if (nrf_err != NRF_SUCCESS) {

ok, just for this specific case since we have PSA errors as well, better be explicit I guess.

anhmolt · 2025-09-30T13:42:37Z

include/nrf_sdh.h

- * that did not acknowledge have the responsibility to restart it by calling
- * @ref nrf_sdh_request_continue when they are ready for the SoftDevice to change state.
+ * Disable the SoftDevice and send state events to registered observers.
+ * An observer may halt the SoftDevice state change by returnin non-zero when receiving


Suggested change

* An observer may halt the SoftDevice state change by returnin non-zero when receiving

* An observer may halt the SoftDevice state change by returning non-zero when receiving

anhmolt · 2025-09-30T14:04:53Z

lib/bm_storage/sd/bm_storage_sd.c

-	state.paused = true;
-
-	return (state.type == BM_STORAGE_SD_STATE_IDLE);
+	case NRF_SDH_STATE_EVT_BLE_ENABLED:


I guess it can make sense to explicitly list all events from enum nrf_sdh_state_evt here. Though, having a case for NRF_SDH_STATE_EVT_BLE_ENABLED that does the same as default looks a bit strange and redundant.

I'll leave this as-is if it's alright

Ok with me 👍

anhmolt · 2025-09-30T15:00:45Z

subsys/softdevice_handler/nrf_sdh.c

 	if (IS_ENABLED(CONFIG_NRF_SDH_STR_TABLES)) {
-		LOG_INF("State change request: %s", req_tostr(req));
+		LOG_DBG("State change: %s", state_tostr(state));
 	} else {
-		LOG_INF("State change request: %#x", req);
+		LOG_DBG("State change: %#x", state);
 	}


Should we give this and the state_tostr() function the same treatment as the nrf_sdh_ble_evt_tostr() and nrf_sdh_soc_evt_tostr()? Or are you happy with how it looks currently?

Not sure we need to export this one, if needed we can take it later.
Here we are not stringifying the events, just printing "enabled" / "disabled".

anhmolt · 2025-09-30T15:38:16Z

subsys/softdevice_handler/nrf_sdh.c

-			LOG_DBG("Notify observer %p => ready", obs);
-		} else {
+	TYPE_SECTION_FOREACH(struct nrf_sdh_state_evt_observer, nrf_sdh_state_evt_observers, obs) {
+		busy = obs->handler(state, obs->context);


The state event observers are also called from nrf_sdh_ble_enable().

Should we add (void) in front here: https://github.com/lemrey/sdk-nrf-bm/blob/749680f702c3ec718f30aef0795d283555d1fa00/subsys/softdevice_handler/nrf_sdh_ble.c#L252v
to show that it returns a value but we choose to ignore it.

Ok, I should have fixed this by calling to a common function now.

anhmolt · 2025-09-30T16:05:16Z

subsys/softdevice_handler/nrf_sdh.c

+		busy = obs->handler(state, obs->context);
+		if (busy) {
 			/* Do not let SoftDevice change state now */
 			LOG_DBG("Notify observer %p => busy", obs);
 			return -EBUSY;
 		}


This does not seem right.

If returning 1 from a state event observer as a response to NRF_SDH_STATE_EVT_ENABLED or NRF_SDH_STATE_EVT_DISABLED, busy would be set to 1 here and we return -EBUSY. If there is more than one registered state event observer, this could led to one or more state event observers not being called, because we return early.

Or do we depend on the user to ensure that 0 is always returned when handling NRF_SDH_STATE_EVT_ENABLED or NRF_SDH_STATE_EVT_DISABLED state events?

I'm able to observe this if adding a second state event observer in the hello_softdevice sample, then log from both and returning 1 from both. This does not make sense for NRF_SDH_STATE_EVT_ENABLED and NRF_SDH_STATE_EVT_DISABLED.

Another thing that we should maybe document or do something about is the fact that the state event observers can be invoked several times with the same event if one or more modules would return busy and later invoke nrf_sdh_request_continue(), which would resend the event to all observers that have already returned not busy.

Consider three observers, where the first and third in the observer list would return zero while the second one in the observer list would return busy and later call nrf_sdh_request_continue(). How would this look for each of the three?

Wrote this and saw later that you are thinking of removing the nrf_sdh_request_continue function.

I see, let me adjust this and add some asserts.

nordicjm · 2025-10-02T10:56:55Z

include/nrf_sdh.h

+
+/* Helper macros to check for equality */
+
+#define _NRF_SDH_OBSERVER_PRIO_HIGHEST_HIGHEST 1


don't use underscores for first character

Alright, will rename

nordicjm · 2025-10-02T10:57:47Z

include/nrf_sdh_soc.h

+ * If :option:`CONFIG_NRF_SDH_STR_TABLES` is enabled, returns the event name.
+ * Otherwise, returns the supplied integer as a string.
+ *
+ * @param evt A NRF_SOC_SVCS enumeration value.


@ before symbols links to it

@nordicjm Can you please provide some context on this suggestion? Thanks.

nordicjm · 2025-10-02T10:59:19Z

subsys/softdevice_handler/nrf_sdh.c

 	};

-	if (sdh_enabled) {
+	(void) sd_softdevice_is_enabled(&sd_is_enabled);


Suggested change

(void) sd_softdevice_is_enabled(&sd_is_enabled);

(void)sd_softdevice_is_enabled(&sd_is_enabled);

fix in whole PR

Add a placeholder entry for sdh to be able to reference it in the changelog. Signed-off-by: Emanuele Di Santo <[email protected]>

This change aims to simplify the configuration options. This is currently a core component, and can't really be optional. If it were to be turned off, no BLE library would work. Signed-off-by: Emanuele Di Santo <[email protected]>

It is more correct to use an SoC observer to do this, rather than doing it directly in nrf_sdh_soc (the stack observer). Make the feature optional, and leave it enabled by default. Signed-off-by: Emanuele Di Santo <[email protected]>

Introduce an allowed set of priorities for observers, with validity checks. These are: HIGHEST, HIGH, USER, USER_LOW, LOWEST. These could be extended ala SYS_INIT to include a priority within a level, but it seemed a bit overkill for now. Each library or piece of code in the SDK shall have an hardcoded priority, because just changing the priority of one component would break the others. In general, a component's observer must have a lower priority than the observers in its dependencies. Signed-off-by: Emanuele Di Santo <[email protected]>

Add GATTS and GATTC event strings to the table. Renamed gap_evt_tostr() to ble_evt_tostr() to reflect that it now can print all BLE events. Signed-off-by: Emanuele Di Santo <[email protected]>

Add a Kconfig menu for clock configuration, to make the menu nicer. Signed-off-by: Emanuele Di Santo <[email protected]>

Add a Kconfig menu for BLE configuration, to make the menu nicer. Signed-off-by: Emanuele Di Santo <[email protected]>

Rework the functions in nrf_sdh_ble and nrf_sdh_soc that mapped BLE and SOC events to their stringified version to make them public and align their names. Instead of printing "unknown" if the event is not known, default to printing its numerical value in hexadecimal. That is also the default behavior if CONFIG_NRF_SDH_STR_TABLES is unset, and the table is not compiled. This is better than toggling the availability of the whole function, because we avoid conditional compilation around it. Signed-off-by: Emanuele Di Santo <[email protected]>

Update the assert message at the end of the polling loops to print which kind of event we failed to pull, e.g. SoC or BLE. Signed-off-by: Emanuele Di Santo <[email protected]>

There was a distinction between observers that could stop the SoftDevice state changes, and those that didn't. The former were called "request observers" and the latter "state observers". Since both observers are interested in the SoftDevice state, this commit merges the two, keeping only "state observers", for the purpose of simplifying things a bit. Now, state observers can return non-zero to these events: - NRF_SDH_STATE_EVT_ENABLE_PREPARE and - NRF_SDH_STATE_EVT_DISABLE_PREPARE to halt the state change. The return value from the observer is ignored for other events. Signed-off-by: Emanuele Di Santo <[email protected]>

These can return -EBUSY, which was missing. Signed-off-by: Emanuele Di Santo <[email protected]>

This API is not useful in NCS BM. Signed-off-by: Emanuele Di Santo <[email protected]>

This function shifts the responsibility of changing the state of the SoftDevice to the observer that halted its state change. This means that potentially all observers that halt the state change not only become responsible of the process (which may not be ideal) but also have to maintain some state and logic to restart that process. Instead, let the component who requested the state change in the first place retry the operation. Signed-off-by: Emanuele Di Santo <[email protected]>

Use the native SoftDevice function sd_softdevice_is_enabled() instead. Signed-off-by: Emanuele Di Santo <[email protected]>

Only check if the SoftDevice handler is suspended, not if the SoftDevice is enabled. Signed-off-by: Emanuele Di Santo <[email protected]>

Rename an internal variable. Signed-off-by: Emanuele Di Santo <[email protected]>

Remove these, and force users to put a semicolon themselves. Signed-off-by: Emanuele Di Santo <[email protected]>

lemrey · 2025-10-03T12:54:52Z

Added changlog entries

eivindj-nordic · 2025-10-03T12:57:43Z

doc/nrf-bm/libraries/nrf_sdh.rst

+SoftDevice handler
+##################
+
+The SoftDevice handler is a library that handles SoftDevice initialization tasks, and pulls and dispatches the SoftDevice events to registered components.


Would be good with more skin on the bone, though I guess that can be added later as well.

MirkoCovizzi · 2025-10-03T13:21:05Z

doc/nrf-bm/release_notes/release_notes_changelog.rst

-No changes since the latest nRF Connect SDK Bare Metal release.
+* Removed:
+
+  * The ``NRF_SDH_BLE`` Kconfig option.


I think it should be like this:

Suggested change

* The ``NRF_SDH_BLE`` Kconfig option.

* The ``CONFIG_NRF_SDH_BLE`` Kconfig option.

MirkoCovizzi · 2025-10-03T13:22:25Z

doc/nrf-bm/release_notes/release_notes_changelog.rst


+* Added:
+
+  * The :option:`NRF_SDH_SOC_RAND_SEED` Kconfig option to control whether to automatically respond to SoftDevice random seed requests.


Suggested change

* The :option:`NRF_SDH_SOC_RAND_SEED` Kconfig option to control whether to automatically respond to SoftDevice random seed requests.

* The :kconfig:option:`CONFIG_NRF_SDH_SOC_RAND_SEED` Kconfig option to control whether to automatically respond to SoftDevice random seed requests.

MirkoCovizzi · 2025-10-03T13:31:44Z

include/nrf_sdh.h

+ * @{
+ */
+
+


Extra newline.

MirkoCovizzi · 2025-10-03T13:35:00Z

include/nrf_sdh.h

+ * A SoftDevice observer has a defined priority, which determines the order with
+ * which the observer receives relevant events compared to other observers.
+ *
+ * Five priority levels are defined, highest, high, user, user low, lowest.


Suggested change

* Five priority levels are defined, highest, high, user, user low, lowest.

* Five priority levels are defined: highest, high, user, user low, and lowest.

MirkoCovizzi · 2025-10-03T13:42:31Z

subsys/softdevice_handler/nrf_sdh_ble.c

 LOG_MODULE_DECLARE(nrf_sdh, CONFIG_NRF_SDH_LOG_LEVEL);

-const char *gap_evt_tostr(int evt)
+const char *ble_evt_tostr(int evt)


Suggested change

const char *ble_evt_tostr(int evt)

const char *ble_evt_to_str(int evt)

MirkoCovizzi · 2025-10-03T14:00:52Z

subsys/softdevice_handler/nrf_sdh_ble.c

 	case BLE_GAP_EVT_CONNECTED:
 		return "BLE_GAP_EVT_CONNECTED";


I think I prefer the current implementation. Otherwise we start devolving into too much macro magic.

eivindj-nordic · 2025-10-21T12:11:15Z

@lemrey Please rebase :)

lemrey requested review from a team as code owners September 23, 2025 12:38

lemrey requested a review from a team September 23, 2025 12:39

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

lemrey force-pushed the sdh branch 4 times, most recently from b6616a3 to 9cf07f8 Compare September 24, 2025 07:06

lemrey requested a review from a team as a code owner September 24, 2025 07:06

lemrey force-pushed the sdh branch 3 times, most recently from 5fd1e4e to 749680f Compare September 24, 2025 09:53

anhmolt reviewed Oct 1, 2025

View reviewed changes

nordicjm requested changes Oct 2, 2025

View reviewed changes

lemrey force-pushed the sdh branch 2 times, most recently from 88c1e77 to 5032ba2 Compare October 3, 2025 10:50

lemrey added 13 commits October 3, 2025 14:48

doc: add an entry for sdh

2b9ee6a

Add a placeholder entry for sdh to be able to reference it in the changelog. Signed-off-by: Emanuele Di Santo <[email protected]>

sdh: remove NRF_SDH_BLE

1e893a9

This change aims to simplify the configuration options. This is currently a core component, and can't really be optional. If it were to be turned off, no BLE library would work. Signed-off-by: Emanuele Di Santo <[email protected]>

sdh: ble: add GATTS and GATTC event strings to the table

c200b77

Add GATTS and GATTC event strings to the table. Renamed gap_evt_tostr() to ble_evt_tostr() to reflect that it now can print all BLE events. Signed-off-by: Emanuele Di Santo <[email protected]>

sdh: add a menu for clock configuration

4229682

Add a Kconfig menu for clock configuration, to make the menu nicer. Signed-off-by: Emanuele Di Santo <[email protected]>

sdh: add a menu for BLE configuration

307d8e5

Add a Kconfig menu for BLE configuration, to make the menu nicer. Signed-off-by: Emanuele Di Santo <[email protected]>

sdh: update the assert message in the polling loop

ab5e941

Update the assert message at the end of the polling loops to print which kind of event we failed to pull, e.g. SoC or BLE. Signed-off-by: Emanuele Di Santo <[email protected]>

sdh: update the doxygen for _enable_request() and _disable_request()

565a9cf

These can return -EBUSY, which was missing. Signed-off-by: Emanuele Di Santo <[email protected]>

sdh: remove nrf_sdh_ble_app_ram_start_get()

093c9f1

This API is not useful in NCS BM. Signed-off-by: Emanuele Di Santo <[email protected]>

lemrey added 4 commits October 3, 2025 14:54

sdh: remove nrf_sdh_is_enabled()

a0bd9e2

Use the native SoftDevice function sd_softdevice_is_enabled() instead. Signed-off-by: Emanuele Di Santo <[email protected]>

sdh: let nrf_sdh_is_suspended() only check for sdh state

39652c2

Only check if the SoftDevice handler is suspended, not if the SoftDevice is enabled. Signed-off-by: Emanuele Di Santo <[email protected]>

sdh: rename variable

e07d39b

Rename an internal variable. Signed-off-by: Emanuele Di Santo <[email protected]>

sdh: remove semicolons at the end of observer macros

381bff5

Remove these, and force users to put a semicolon themselves. Signed-off-by: Emanuele Di Santo <[email protected]>

lemrey force-pushed the sdh branch from 5032ba2 to 381bff5 Compare October 3, 2025 12:54

lemrey requested a review from a team as a code owner October 3, 2025 12:54

github-actions bot added the doc-required PR must not be merged without tech writer approval. label Oct 3, 2025

eivindj-nordic reviewed Oct 3, 2025

View reviewed changes

MirkoCovizzi reviewed Oct 3, 2025

View reviewed changes

eivindj-nordic added this to the v1.0.0 milestone Oct 13, 2025

	bool "Automatically seed SoftDevice random seeds requests"
	bool "Automatically respond to SoftDevice random seed requests"

	nrf_sdh_soc_evt_tostr(). Disable to save non-volatile memory.
	nrf_sdh_soc_evt_tostr(). Disable to save non-volatile memory.

	LOG_ERR("Failed to generate true random number, psa_status %#x", status);
	LOG_ERR("Failed to generate true random number, psa_status %d", status);

	* An observer may halt the SoftDevice state change by returnin non-zero when receiving
	* An observer may halt the SoftDevice state change by returning non-zero when receiving


		/* Helper macros to check for equality */

		#define _NRF_SDH_OBSERVER_PRIO_HIGHEST_HIGHEST 1

	(void) sd_softdevice_is_enabled(&sd_is_enabled);
	(void)sd_softdevice_is_enabled(&sd_is_enabled);

	* The ``NRF_SDH_BLE`` Kconfig option.
	* The ``CONFIG_NRF_SDH_BLE`` Kconfig option.


		* Added:

		* The :option:`NRF_SDH_SOC_RAND_SEED` Kconfig option to control whether to automatically respond to SoftDevice random seed requests.

	* The :option:`NRF_SDH_SOC_RAND_SEED` Kconfig option to control whether to automatically respond to SoftDevice random seed requests.
	* The :kconfig:option:`CONFIG_NRF_SDH_SOC_RAND_SEED` Kconfig option to control whether to automatically respond to SoftDevice random seed requests.

	* Five priority levels are defined, highest, high, user, user low, lowest.
	* Five priority levels are defined: highest, high, user, user low, and lowest.

	const char *ble_evt_tostr(int evt)
	const char *ble_evt_to_str(int evt)

sdh: simplifications and improvements #388

Are you sure you want to change the base?

sdh: simplifications and improvements #388

Uh oh!

Conversation

lemrey commented Sep 23, 2025

Uh oh!

github-actions bot commented Sep 23, 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!

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!

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!

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!

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!

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!

lemrey commented Oct 3, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment