lib: peer_manager: refactor Kconfig options #379

MirkoCovizzi · 2025-09-19T12:20:20Z

Refactor Peer Manager Kconfig options.

github-actions · 2025-09-19T12:21:34Z

You can find the documentation preview for this PR here.

lemrey · 2025-09-22T07:46:56Z

lib/peer_manager/Kconfig

+	default y if NRF_SDH_BLE_SERVICE_CHANGED
+	default n


This feature depends on the service changed characteristic being enabled in the SoftDevice, which happens automatically when NRF_SDH_BLE_SERVICE_CHANGED is set.

So this should rather be:
depends on NRF_SDH_BLE_SERVICE_CHANGED
default y

Otherwise this could still be enabled when the NRF_SDH_BLE_SERVICE_CHANGED is not enabled, and that would be an error.

nordicjm

Seems commits might need squashing?

nordicjm · 2025-10-02T11:00:41Z

lib/peer_manager/Kconfig


-config PM_SERVICE_CHANGED_ENABLED
+config PM_SERVICE_CHANGED
 	bool "Enable/disable the service changed management for GATT server in Peer Manager."


Suggested change

bool "Enable/disable the service changed management for GATT server in Peer Manager."

bool "Service changed management for GATT server in Peer Manager."

Please have another look, this was addressed in a separate commit.

nordicjm · 2025-10-02T11:01:04Z

lib/peer_manager/modules/gatt_cache_manager.c

 }

-#if CONFIG_PM_SERVICE_CHANGED_ENABLED
+#if CONFIG_PM_SERVICE_CHANGED


these are defines, change them all to #ifdef or #if defined(CONFIG_...

this is not fixed

nordicjm · 2025-10-02T11:01:18Z

include/bluetooth/peer_manager/peer_manager.h

 * @retval NRF_ERROR_INTERNAL       If an internal error occurred.
 * @retval NRF_ERROR_NOT_SUPPORTED  If peer rank functionality has been disabled via the @ref
- *                                  PM_PEER_RANKS_ENABLED configuration option.
+ *                                  PM_PEER_RANKS configuration option.


@ before will link to it

Please have another look, this was addressed in a separate commit.

nordicjm · 2025-10-02T11:01:38Z

lib/peer_manager/Kconfig


-config PM_PEER_RANKS_ENABLED
+config PM_PEER_RANKS
 	bool "Enable/disable the peer rank management in Peer Manager."


fix above issues in whole PR, never start a Kconfig prompt with enable

Please have another look, this was addressed in a separate commit.

nordicjm · 2025-10-02T11:01:50Z

lib/peer_manager/Kconfig

-config PM_LESC_ENABLED
+config PM_LESC
 	bool "Enable/disable LESC support in Peer Manager."
 	default n


Suggested change

default n

MirkoCovizzi · 2025-10-03T07:36:10Z

Seems commits might need squashing?

I don't see why they should be squashed. It's very clear what each commit does and how they separate each atomic change.

nordicjm · 2025-10-03T07:59:40Z

lib/peer_manager/modules/gatt_cache_manager.c

 }

-#if CONFIG_PM_SERVICE_CHANGED_ENABLED
+#if CONFIG_PM_SERVICE_CHANGED


this is not fixed

anhmolt

Could you add a commit where you change CONFIG_REQ to PM_EVT_CONN_SEC_CONFIG_REQ here

sdk-nrf-bm/lib/peer_manager/modules/security_dispatcher.c

Line 420 in 3033a26

* @brief Function for sending a CONFIG_REQ event.

The CONFIG_ namespace is for Kconfigs.

lib/peer_manager/Kconfig

include/bluetooth/peer_manager/peer_manager.h

anhmolt · 2025-10-03T09:00:11Z

lib/peer_manager/modules/peer_database.c

 extern void pm_pdb_evt_handler(pm_evt_t *p_event);
 extern void sm_pdb_evt_handler(pm_evt_t *p_event);
-#if !defined(CONFIG_PM_SERVICE_CHANGED_ENABLED) || (CONFIG_PM_SERVICE_CHANGED_ENABLED == 1)
+#if !defined(CONFIG_PM_SERVICE_CHANGED) || (CONFIG_PM_SERVICE_CHANGED == 1)


This does not make sense with the (new) Kconfig system. This logic probably took care of the "default" behavior in the nRF5 SDK, when the config was not defined.

Suggested change

#if !defined(CONFIG_PM_SERVICE_CHANGED) || (CONFIG_PM_SERVICE_CHANGED == 1)

#if defined(CONFIG_PM_SERVICE_CHANGED)

Here and others.

eivindj-nordic · 2025-10-06T08:50:44Z

I don't see why they should be squashed. It's very clear what each commit does and how they separate each atomic change.

I see your point, though I think many of the changes are too small to deserve its own commit when the other commits are quite similar. For issues outside of these commits it also helps with bisectability to squash the commits, as it could remove the need for a couple of steps in the bisect process.

eivindj-nordic · 2025-10-06T10:03:37Z

doc/nrf-bm/release_notes/release_notes_changelog.rst

+
+   * Updated:
+
+     * The ``CONFIG_PM_SERVICE_CHANGED_ENABLED`` Kconfig option to :kconfig:option:`CONFIG_PM_SERVICE_CHANGED`.


Here and others

Suggested change

* The ``CONFIG_PM_SERVICE_CHANGED_ENABLED`` Kconfig option to :kconfig:option:`CONFIG_PM_SERVICE_CHANGED`.

* The ``CONFIG_PM_SERVICE_CHANGED_ENABLED`` Kconfig option is renamed to :kconfig:option:`CONFIG_PM_SERVICE_CHANGED`.

eivindj-nordic · 2025-10-06T10:06:29Z

include/bluetooth/peer_manager/peer_manager.h

 *
- * @note This function is deprecated. LESC keys are now handled internally if @ref PM_LESC_ENABLED
- *       is true. If @ref PM_LESC_ENABLED is false, this function works as before.
+ * @note This function is deprecated. LESC keys are now handled internally if the @c CONFIG_PM_LESC


Use @deprecated?

Suggested change

* @note This function is deprecated. LESC keys are now handled internally if the @c CONFIG_PM_LESC

* @deprecated LESC keys are now handled internally if the @c CONFIG_PM_LESC

eivindj-nordic · 2025-10-06T10:08:21Z

include/bluetooth/peer_manager/peer_manager.h

+ *       Kconfig option is enabled. If the @c CONFIG_PM_LESC Kconfig option is disabled, this
+ *       function works as before.


What do you mean by this last sentence?

If the @c CONFIG_PM_LESC Kconfig option is disabled, this function works as before.

If this is still meant to be used when CONFIG_PM_LESC Kconfig option is disabled, this is not deprecated.

Not sure, this is from the original Doxygen. I merely changed the Kconfig reference.

MirkoCovizzi · 2025-10-06T11:37:17Z

I don't see why they should be squashed. It's very clear what each commit does and how they separate each atomic change.

I see your point, though I think many of the changes are too small to deserve its own commit when the other commits are quite similar. For issues outside of these commits it also helps with bisectability to squash the commits, as it could remove the need for a couple of steps in the bisect process.

I squashed the mechanical commits that can make sense to squash.

* Renames the `PM_SERVICE_CHANGED_ENABLED`, `PM_PEER_RANKS_ENABLED`, `PM_LESC_ENABLED`, and `PM_RA_PROTECTION_ENABLED` kconfig options to align with Zephyr's coding style. * Improves prompts and descriptions of Kconfig options. * Moves secondary `PM_RA_PROTECTION` Kconfigs inside conditional block. * Removes explicit `default n`. * Fixes doxygen sections related to Kconfig options. * Refactors the Peer Manager to use the `#if defined` (or `#if !defined`) preprocessor pattern where appropriate. Signed-off-by: Mirko Covizzi <[email protected]>

Conditionally compile Auth Status Tracker. Signed-off-by: Mirko Covizzi <[email protected]>

If not using a GATT server, or using a server without a service changed characteristic, this Kconfig option should be disabled to save code space. Refactors this Kconfig option in order to represent this relationship accordingly. Signed-off-by: Mirko Covizzi <[email protected]>

* Fixes a typo. * Removes extra newlines. Signed-off-by: Mirko Covizzi <[email protected]>

MirkoCovizzi self-assigned this Sep 19, 2025

MirkoCovizzi requested review from a team as code owners September 19, 2025 12:20

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

MirkoCovizzi changed the title ~~lib: peer_manager: rename PM_SERVICE_CHANGED_ENABLED~~ lib: peer_manager: refactor Kconfig options Sep 19, 2025

MirkoCovizzi marked this pull request as draft September 19, 2025 12:21

MirkoCovizzi added the DNM Do not merge label Sep 19, 2025

lemrey reviewed Sep 22, 2025

View reviewed changes

MirkoCovizzi added this to the v1.0.0 milestone Sep 29, 2025

MirkoCovizzi force-pushed the peer-manager-k branch 3 times, most recently from 2e92b15 to 24b546d Compare September 30, 2025 08:24

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

MirkoCovizzi force-pushed the peer-manager-k branch from a522a29 to fe0a2c0 Compare October 2, 2025 08:15

MirkoCovizzi marked this pull request as ready for review October 2, 2025 08:16

MirkoCovizzi requested a review from a team as a code owner October 2, 2025 08:16

MirkoCovizzi removed the DNM Do not merge label Oct 2, 2025

MirkoCovizzi force-pushed the peer-manager-k branch from fe0a2c0 to 00ffe64 Compare October 2, 2025 08:16

MirkoCovizzi requested a review from lemrey October 2, 2025 08:17

b-gent approved these changes Oct 2, 2025

View reviewed changes

nordicjm requested changes Oct 2, 2025

View reviewed changes

nordicjm requested changes Oct 3, 2025

View reviewed changes

anhmolt reviewed Oct 3, 2025

View reviewed changes

lib/peer_manager/Kconfig Outdated Show resolved Hide resolved

lib/peer_manager/Kconfig Outdated Show resolved Hide resolved

lib/peer_manager/Kconfig Outdated Show resolved Hide resolved

include/bluetooth/peer_manager/peer_manager.h Outdated Show resolved Hide resolved

MirkoCovizzi force-pushed the peer-manager-k branch 2 times, most recently from a3d73d2 to 228427e Compare October 3, 2025 08:48

anhmolt reviewed Oct 3, 2025

View reviewed changes

MirkoCovizzi force-pushed the peer-manager-k branch from 5bf2713 to c211035 Compare October 3, 2025 11:01

MirkoCovizzi requested a review from nordicjm October 3, 2025 11:07

MirkoCovizzi requested a review from anhmolt October 3, 2025 11:07

MirkoCovizzi force-pushed the peer-manager-k branch from c211035 to e5924c8 Compare October 3, 2025 13:07

eivindj-nordic reviewed Oct 6, 2025

View reviewed changes

MirkoCovizzi force-pushed the peer-manager-k branch 2 times, most recently from 89466e2 to 3eb8aea Compare October 6, 2025 11:35

MirkoCovizzi force-pushed the peer-manager-k branch from 3eb8aea to afcd030 Compare October 6, 2025 12:14

MirkoCovizzi requested a review from eivindj-nordic October 6, 2025 12:14

MirkoCovizzi force-pushed the peer-manager-k branch 2 times, most recently from 5c34624 to 5272011 Compare October 6, 2025 12:17

eivindj-nordic approved these changes Oct 6, 2025

View reviewed changes

nordicjm approved these changes Oct 6, 2025

View reviewed changes

MirkoCovizzi added 4 commits October 6, 2025 16:56

lib: peer_manager: conditionally compile Auth Status Tracker

62f2e7c

Conditionally compile Auth Status Tracker. Signed-off-by: Mirko Covizzi <[email protected]>

lib: peer_manager: fix typo and cleanup

5e94761

* Fixes a typo. * Removes extra newlines. Signed-off-by: Mirko Covizzi <[email protected]>

MirkoCovizzi force-pushed the peer-manager-k branch from 5272011 to 5e94761 Compare October 6, 2025 14:57

eivindj-nordic merged commit 6879627 into nrfconnect:main Oct 7, 2025
10 checks passed

	bool "Enable/disable the service changed management for GATT server in Peer Manager."
	bool "Service changed management for GATT server in Peer Manager."

	#if !defined(CONFIG_PM_SERVICE_CHANGED) \|\| (CONFIG_PM_SERVICE_CHANGED == 1)
	#if defined(CONFIG_PM_SERVICE_CHANGED)


		* Updated:

		* The ``CONFIG_PM_SERVICE_CHANGED_ENABLED`` Kconfig option to :kconfig:option:`CONFIG_PM_SERVICE_CHANGED`.

	* @note This function is deprecated. LESC keys are now handled internally if the @c CONFIG_PM_LESC
	* @deprecated LESC keys are now handled internally if the @c CONFIG_PM_LESC

		* Kconfig option is enabled. If the @c CONFIG_PM_LESC Kconfig option is disabled, this
		* function works as before.

lib: peer_manager: refactor Kconfig options #379

lib: peer_manager: refactor Kconfig options #379

Conversation

MirkoCovizzi commented Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Sep 19, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nordicjm left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

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

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

MirkoCovizzi commented Oct 3, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anhmolt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

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!

MirkoCovizzi Oct 6, 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!

MirkoCovizzi commented Oct 6, 2025

Uh oh!

Uh oh!

Uh oh!

MirkoCovizzi commented Sep 19, 2025 •

edited

Loading

MirkoCovizzi Oct 3, 2025 •

edited

Loading

MirkoCovizzi Oct 3, 2025 •

edited

Loading

MirkoCovizzi Oct 3, 2025 •

edited

Loading

eivindj-nordic commented Oct 6, 2025 •

edited

Loading

MirkoCovizzi Oct 6, 2025 •

edited

Loading