Skip to content

BLE: suppress scan timeout if we disabled scanning #9058

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

Merged
merged 2 commits into from
Jan 4, 2019

Conversation

paul-szczepanek-arm
Copy link
Member

Description

Currently a scan timeout will be called even if we have stopped scanning due to a stopScan call or connect. This suppresses the call.

Pull request type

[x] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Copy link
Member

@pan- pan- 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 the changes however I wonder if it wouldn't be more clear to set the flag only in case of success rather than:

  • setting expected flag value
  • execute action
  • revert flag value.

@paul-szczepanek-arm
Copy link
Member Author

This PR is now limited to only scan stop

Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

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

Thanks, @paul-szczepanek-arm the changes looks good 👍 .

However I think it would be interesting to not limit state maintenance to the devices that support extended advertising as we will eventually add an external getter to query the state of the local device and this would have to work with all controllers.

@paul-szczepanek-arm
Copy link
Member Author

paul-szczepanek-arm commented Dec 27, 2018

But this problem only affect extended advertising so I think this is a valuable fix.

This fix does not preclude later improvements.

@cmonr
Copy link
Contributor

cmonr commented Jan 3, 2019

@paul-szczepanek-arm Is there a reason that the timeout wasn't instead disabled, to prevent from a context switch from being entered?

@paul-szczepanek-arm
Copy link
Member Author

I don't understand the question so I'll try to answer other questions:

  • events are serialised through event queue on user thread (ble is not thread safe)
  • timeout happens on the controller - we can't remove it

@pan-
Copy link
Member

pan- commented Jan 3, 2019

@cmonr If extended scanning is supported, the Bluetooth controller is supposed to send an event when scanning timeout. Unfortunately, the Cordio host stack behaviour is a bit different: The timeout event is generated whenever scanning stop; even if scanning has been stopped by the application. The timeout we are talking about has nothing to do with the Timeout class.

To prevent generation of timeout events when the application has stopped the scan process we need to keep that information locally and filter timeout event if the scan process has already been stopped. The only thing that can be argued about this PR is whether the filtering is made at the right level or not (pal or generic).

@cmonr
Copy link
Contributor

cmonr commented Jan 4, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Jan 4, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 1
Build artifacts

@0xc0170 0xc0170 merged commit 0f7381d into ARMmbed:master Jan 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants