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
  • Very good! Thanks for the response. I did go ahead and make the changes I posted above. I did not create (yet) multiple advertising sets, but when I step through the code in these handlers the information is as expected and it appears to do the right thing.

    Cheers!

  • jsheaney said:
    Thanks for the response.

    No problem at all - we very much appreciate the feedback! :) 

    jsheaney said:
    I step through the code in these handlers the information is as expected and it appears to do the right thing.

    Great! Since the current_slave_link_conn_handle is currently only being used for that one particular check in the library, you could pretty much implement what ever functionality / checks you find fitting for it, without fear of breaking the driver (as long as you keep the single existing check in mind).
     
    Please do not hesitate to open a new ticket if you should encounter any other issues or questions in the future.

    Good luck with your development!

    Best regards,
    Karl

Reply
  • jsheaney said:
    Thanks for the response.

    No problem at all - we very much appreciate the feedback! :) 

    jsheaney said:
    I step through the code in these handlers the information is as expected and it appears to do the right thing.

    Great! Since the current_slave_link_conn_handle is currently only being used for that one particular check in the library, you could pretty much implement what ever functionality / checks you find fitting for it, without fear of breaking the driver (as long as you keep the single existing check in mind).
     
    Please do not hesitate to open a new ticket if you should encounter any other issues or questions in the future.

    Good luck with your development!

    Best regards,
    Karl

Children
Related