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

linux.hci: fix stop advertise on new connection #49

Merged
merged 1 commit into from
Jan 10, 2020

Conversation

rkjdid
Copy link

@rkjdid rkjdid commented Aug 2, 2019

this line was a no-op, thus allowing multiple simultaneous connections, since it was just re-sending 1 (h.params.advEnable equals 1 from line above)

@rkjdid
Copy link
Author

rkjdid commented Jan 10, 2020

For #49, I think the better fix is to only re-enable advertising upon disconnection. A completed connection should not re-enable advertising since only one central connection is allowed. Note that the spec allows for multiple central connections but as far as I have seen, this is not well supported or often used.

I think we're on the same page, and this is what I tried to fix here.

From what I understand on how it's supposed to work, and how it does work with this patch:

Without this patch, advertising is enabled both on connection and disconnection (resulting in the host being always available for connection).

I've just tested it without the patch, tbh it seems to be working fine with at least 2 simultaneous connections. I don't know the spec, but it didn't suit my specific usecase since I only want one connection at a time, and reading at the code & comment I thought it was also the intent minus a bug. But maybe another approach, since simul connections seem to be working fine (and is allowed per spec?), is to add a setting specifying max nb of live conn allowed, and stop advertising if reached.

@estutzenberger
Copy link

estutzenberger commented Jan 10, 2020

Ahh, I see now looking more closely at the code. I think the behavior change by this MR makes sense for the time being and was the intended behavior of the original authors.

@estutzenberger estutzenberger merged commit 67ae735 into go-ble:master Jan 10, 2020
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.

2 participants