Master(central) Not sending EDIV nor IRK during pairing phase. Also possible bug in peer_manager_init implementation.

[trunk]

It took me a few days to determine this issue, so I wanted to share it with the community if you come across this issue. If your pairing process is not sharing the IRK/ Resolvable address and your can't figure out why, you need to downgrade your address privacy. I did this in peer_manager_init()

err_code = pm_sec_params_set(&m_sec_param);
APP_ERROR_CHECK(err_code);

// -- added start

pm_privacy_params_t privacy_params;
memset(&privacy_params, 0, sizeof(pm_privacy_params_t));

privacy_params.privacy_mode = BLE_GAP_PRIVACY_MODE_DEVICE_PRIVACY;
privacy_params.private_addr_type = BLE_GAP_ADDR_TYPE_RANDOM_PRIVATE_RESOLVABLE;
privacy_params.private_addr_cycle_s = 900; // Change address every 15 minutes

err_code = pm_privacy_set(&privacy_params);
// -- added end

[SDK BUG] In addition, in peer_manager_init, it appears  the local sec_param is set to a pointer to be used outside of the scope of the function. This variable is local and although the function is static, it doesn't mean the local variable will be static. I made mine as a global or it should be explicitly defined as static within the peer_manager_init.

 

[Einar Thorsrud]

Hi,

Thank you for sharing and letting us know.

Regarding the first part the code you added is for using privacy (resolvable random address). That is not used by any examples, but you can use it as you explained/showed.

However, regarding the second part I am I do not understand the bug you are describing. Do you mind uploading a diff showing your changes, and perhaps an extended explanation for why it is better?

[trunk]

It seems GCC is making local variables static within the static function even though it's not expressly defined. This is behavior is not guaranteed. A local variable within a static function may not be made static with other compilers. 

Below local variable sec_param is being used outside of the scope of this function and if it's placed on the stack it will get corrupted. The fix is to explicitly assign it as static. 

static void peer_manager_init(void)
{
    
    ret_code_t err_code;
-    ble_gap_sec_params_t sec_param;
+    static ble_gap_sec_params_t sec_param;

    err_code = pm_init();
    APP_ERROR_CHECK(err_code);

local variable sec_param assigned in pm_sec_params_set->sm_sec_params_set to a global. This is then used out of scope of peer_manager_init. 

ret_code_t sm_sec_params_set(ble_gap_sec_params_t * p_sec_params)
{
...
m_sec_params     = *p_sec_params;
mp_sec_params    = &m_sec_params;

Fix: sec_param  should explicitly be set to a static in peer_manager_init

[Einar Thorsrud]

Hi,

Why would that be needed? I am not sure which SDK version you are using, but most samples that use the peer manger do it as you have described here. However, this does not look like a bug. The ble_gap_sec_params_t instance is not needed outside of this function. Referring to SDK 17.1.0, the flow is like this:

1. Most examples have a peer_manager_init() with a (non-static) ble_gap_sec_params_t instance called sec_param;
2. sec_param is passed as a pointer to pm_sec_params_set()
3. The pointer is passed further to sm_sec_params_set()
4. In the implementation of sm_sec_params_set() the content is copied to a local static variable m_sec_params (line 696 in security_manager.c). So not this data resides in static memory.

This all happens before the peer_manager_init() function finishes and the sec_param variables goes out of scope.

[trunk]

You're correct. I tend not to do struct copies by assignment so missed that is a valid c copy.  Sorry to waste your time.