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

improve & fix BLEScan when wantDuplicates #3995

Merged
merged 3 commits into from
Nov 2, 2020

Conversation

saknarak
Copy link
Contributor

  • when wantDuplicates, no need to check duplicate and no more insert into vector
  • delete advertisedDevice when not insert into vector, fix memory leak
  • add showParse when you just want raw advertised data

- when wantDuplicates, no need to check duplicate and no more insert into vector
- delete advertisedDevice when not insert into vector, fix memory leak
- add showParse when you just want raw advertised data
advertisedDevice->setScan(this);
advertisedDevice->setAddressType(param->scan_rst.ble_addr_type);

if (!found) { // If we have previously seen this device, don't record it again.
if (!m_wantDuplicates && !found) { // If we have previously seen this device, don't record it again.
Copy link
Contributor

@chegewara chegewara May 13, 2020

Choose a reason for hiding this comment

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

What will happen if you want duplicates?
m_wantDuplicates = true
In that case this will always return false and device will never be inserted to a vector:
if (!m_wantDuplicates && !found)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when start continuous scan with callback. vector add allocate without de-allocatted that cause ESP restart.

when not to keep in vector

  1. callback was defined.
    or 2. wantDuplicates = true
    or 3. already in vector (found = true)

saknarak and others added 2 commits May 13, 2020 18:39
then no need to keep it in vector

if has callback define
  and not want duplicate
  and not already in vector
then record it
@me-no-dev me-no-dev merged commit dccb4e8 into espressif:master Nov 2, 2020
@Ben79543
Copy link
Contributor

Ben79543 commented Feb 6, 2021

on #4627 I explain why this commit is an issue for me: in a croweded BLE environment (100s devices), a 5sec scan would crash the ESP even in the first scan. So I needed to limit the number of detected devices and I did it with a counter in the callback.

But with this commit, when a callback is defined there is no record to vector no matter what, and the scan returns no result.

Easiest is to re-allow to call the callback and populate the vector. Not sure why this case was removed.

@saknarak @me-no-dev can this be reverted or changed?

@chegewara
Copy link
Contributor

I agree, we need this to be fixed or reverted.

Ben79543 added a commit to Ben79543/arduino-esp32-1 that referenced this pull request Jun 3, 2021
Proposed fix to espressif#4627 as a remediation to PR espressif#3995
espressif#3995 introduced that a device detected on BLE would not be entered in vector if a callback has been defined. 
By doing so, it was not possible anymore to have a counter in a call back (AND device in vector) to limit the number of detected device in a single scan, which could crash ESP32 as a result.
@Ben79543 Ben79543 mentioned this pull request Jun 3, 2021
@Ben79543
Copy link
Contributor

Ben79543 commented Jun 3, 2021

I have created a PR for this : #5241
My first PR hopefully done well...

me-no-dev pushed a commit that referenced this pull request Jun 9, 2021
Proposed fix to #4627 as a remediation to PR #3995
#3995 introduced that a device detected on BLE would not be entered in vector if a callback has been defined. 
By doing so, it was not possible anymore to have a counter in a call back (AND device in vector) to limit the number of detected device in a single scan, which could crash ESP32 as a result.
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.

5 participants