Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make connection update process timeout variable #781

Closed
wants to merge 2 commits into from
Closed

Make connection update process timeout variable #781

wants to merge 2 commits into from

Conversation

zacwbond
Copy link
Contributor

This is a fix for #780, an issue I submitted earlier today.

I am connecting as a central to a remote peripheral using a connection interval of 120, latency of 90, and timeout of 3000. Due to the hard-coded 40 second timeout, it is impossible to perform a connection parameter update on this connection. Trying to do so results in a timeout followed by a disconnect 100% of the time.

My attempt to fix the problem is to compute the timeout based on the current connection parameters instead of just hard-coding a constant value.

@apache-mynewt-bot
Copy link

Style check summary

No suggestions at this time!

Copy link
Contributor

@rymanluk rymanluk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for rising the issue as this is valid one. We had some issues with this timer already before. However I would suggest some other solution for that issue.

Some background:
When controller does not support Connection Parameter procedure over LL, we are doing it over L2CAP. When we are doing it over L2CAP we delete this timer as soon as we receive over response L2CAP and not when actually parameters will changed. We did it because we had issues with timing as well and we decided to now wait for the controller to update parameters as it is actually his decision to update parameters or not. For some reason we keep timer in the case where Connection parameter procedure happens over LL but maybe it is time for change as it clearly can cause link drop when we don't want it.

So as said above, imho this timer shall not cause link termination when connection parameter update will not happen, because it is controller to decide if parameters shall be changed based on host suggestion.

I still think we need a timer to at least make sure that app does not send connection parameter updates too often, so I'm not saying we should remove it.

I would vote for different handling of this timer when it fires

a) remove ble_gap_update_notify(conn_handle, BLE_HS_ETIMEOUT); from ble_gap_update_timer(void) so we never notify app about timeout, but instead, when connection parameter finally happen, application will receive the event.

b) remove link termination on BLE_HS_TIMEOUT in ble_gap_update_notify

I would vote for a) @sjanc @andrzej-kaczmarek @ccollins476ad comments?

@zacwbond
Copy link
Contributor Author

@rymanluk Your a) makes sense to me. If I get a timeout, I'd tend to assume that the event that timed out is never going to happen, so it would be odd to send a timeout event if the procedure is eventually going to succeed.

Another problem I've noticed is that I frequently see a call to my reset_cb with error code 19 when the gap connection parameter update procedure is initiated. I think it must be associated with the function ble_hs_hci_wait_for_ack(). That makes it sound like an HCI problem and not a Nimble problem, but it only ever happens with the parameter update procedure, and only intermittently.

@rymanluk
Copy link
Contributor

@zacwbond could you update your PR so it implements a) ?

the other problem you mentioned is something with your controller I believe.

@zacwbond
Copy link
Contributor Author

Closing because I mangled my forks somehow. Will re-open momentarily.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants