This post is older than 2 years and might not be relevant anymore
More Info: Consider searching for newer posts

Bugs in ble_advertising event handlers?

The ble_advertising module supports the concept of advertising sets. Each set is connected to the soft device events by that compile time registration feature that routes ble events. It seems to me that when you receive an event then all of the registered handlers are going to get called, so the code should only change advertising state data if the event is associated with a particular advertising set. The on_disconnected() handler, for example, checks the event connection handle against the current_slave_link_conn_handle stored in the advertising set, which is stored in the on_connected() handler. That makes sense, except I don't see where the stored connection handle is set back to BLE_CONN_HANDLE_INVALID, which I think it should do whenever the connection handle matches.

if (p_ble_evt->evt.gap_evt.conn_handle == p_advertising->current_slave_link_conn_handle)
{
	p_advertising->current_slave_link_conn_handle = BLE_CONN_HANDLE_INVALID;
	p_advertising->whitelist_temporarily_disabled = false;

	if (p_advertising->adv_modes_config.ble_adv_on_disconnect_disabled == false)
	{
		ret = ble_advertising_start(p_advertising, BLE_ADV_MODE_DIRECTED_HIGH_DUTY);
		if ((ret != NRF_SUCCESS) && (p_advertising->error_handler != NULL))
		{
			p_advertising->error_handler(ret);
		}
	}
}

The other problem is the on_connected() handler stores the connection handle unconditionally, so I think the same connection handle would be stored in every advertising set on every connection. I think the answer is to check the adv_handle.

if (p_ble_evt->evt.gap_evt.params.connected.role == BLE_GAP_ROLE_PERIPH
	&& p_ble_evt->evt.gap_evt.params.connected.adv_handle == p_advertising->adv_handle)
{
	p_advertising->current_slave_link_conn_handle = p_ble_evt->evt.gap_evt.conn_handle;
}
	
	

It's the same in the on_terminated() handler, which is another case where advertising ends. Currently, the handler aborts if the event doesn't match, which I think is redundant. The event is why the handler is called in the first place.

if (p_ble_evt->header.evt_id != BLE_GAP_EVT_ADV_SET_TERMINATED)
{
    // Nothing to do.
    return;
}

Rather, I think the adv_handle should be checked.

if (p_ble_evt->evt.gap_evt.params.adv_set_terminated.adv_handle != p_advertising->adv_handle)
{
    // Nothing to do.
    return;
}

Is anyone using multiple advertising sets? I'm not, so I don't know if this is a real issue. It just looks like it is.

nRF5_SDK_17.0.2_d674dde

Parents
  • Hello,

    I have reviewed the code, and I concur with your assessment - the connection handle is only ever set, and never reset if the link is terminated or broken.
    The documentation for the variable is also pretty clear, the variable is intended to hold the active connection's handle.
    Since this is not the intended function, and I have created an internal ticket with the developers so that they may examine this more closely, and implement a fix. I have attached your ticket to the internal ticket, so the developers can read your comments and suggestions to this.

    I believe the reason why this has not come up until now is that the variable is only used by the driver to check whether the disconnection event matches the current connection, to restart advertisement - as you mention.
    Thus, this has probably flown under the radar of most users. Keenly spotted!

    Thank you for bringing this to our attention! :) 

    Best regards,
    Karl

Reply
  • Hello,

    I have reviewed the code, and I concur with your assessment - the connection handle is only ever set, and never reset if the link is terminated or broken.
    The documentation for the variable is also pretty clear, the variable is intended to hold the active connection's handle.
    Since this is not the intended function, and I have created an internal ticket with the developers so that they may examine this more closely, and implement a fix. I have attached your ticket to the internal ticket, so the developers can read your comments and suggestions to this.

    I believe the reason why this has not come up until now is that the variable is only used by the driver to check whether the disconnection event matches the current connection, to restart advertisement - as you mention.
    Thus, this has probably flown under the radar of most users. Keenly spotted!

    Thank you for bringing this to our attention! :) 

    Best regards,
    Karl

Children
No Data
Related