Jira 896, BLE scan withDuplicates logic inversion, git 476 #491
Conversation
SidLeung
requested review from
bigdinotech,
eriknyquist,
russmcinnis and
noelpaz
|
@sgbihu @bigdinotech @eriknyquist , please review the code change. @russmcinnis @noelpaz , please perform system testing. |
| @@ -576,10 +574,32 @@ BLEDevice BLEDeviceManager::peripheral() | |||
| return temp; | |||
| } | |||
|
|
|||
| bool BLEDeviceManager::startScanning() | |||
| void BLEDeviceManager::_clearAdvertiseBuffer() | |||
There was a problem hiding this comment.
The BLEDeviceManager.* doesn't need change. Those changes are for scan response. Jira876.
There was a problem hiding this comment.
I don't know how this get in here... Need to investigate my source...
|
@sgbihu @bigdinotech @eriknyquist @russmcinnis @noelpaz The initial commit was done incorrectly. The committed code contained mods for another ticket (Jira 876). The issue was corrected. Please use the latest commit, 6b64ce3, for code review and system testing. |
libraries/CurieBLE/src/BLEDevice.h
Outdated
| */ | ||
| void scanForName(String name); | ||
| void scan(bool withDuplicates = true); |
There was a problem hiding this comment.
@SidLeung could we please change the withDuplicates default arg to false here.
Remember the overall goal is to have something similar to the Apple CoreBluetooth API: https://developer.apple.com/reference/corebluetooth/cbcentralmanager/1518986-scanforperipherals
which by default doesn't allow duplicates: https://developer.apple.com/reference/corebluetooth/cbcentralmanagerscanoptionallowduplicateskey
There was a problem hiding this comment.
After I read the apple's parameter description, I found the duplicated object is ADV packets. Our implementation is duplicate filter. So I suggest rename the withDuplicates as allowDuplicatePackets to make the parameter easy to be understood.
There was a problem hiding this comment.
Hi @SidLeung,
What's your idea?
libraries/CurieBLE/src/BLEDevice.h
Outdated
| */ | ||
| void scanForName(String name, bool withDuplicates); | ||
| void scanForName(String name, bool withDuplicates = true); |
There was a problem hiding this comment.
Again, please change the default value for withDuplicates to false.
libraries/CurieBLE/src/BLEDevice.h
Outdated
| */ | ||
| void scanForUuid(String uuid); | ||
| void scanForUuid(String uuid, bool withDuplicates = true); |
There was a problem hiding this comment.
Again, please change the default value for withDuplicates to false.
libraries/CurieBLE/src/BLEDevice.h
Outdated
| */ | ||
| void scanForUuid(String uuid, bool withDuplicates); | ||
|
|
||
| void scanForAddress(String macaddr, bool withDuplicates = true); |
There was a problem hiding this comment.
Again, please change the default value for withDuplicates to false.
|
Made the default behavior of scan() to report a detected Peripheral once. @sandeepmistry @sgbihu @eriknyquist @bigdinotech Please review the latest code change, 5242153. @russmcinnis @noelpaz , please make sure the above commit is used for system testing. Thanks. |
Commit error: Committed files contained code mods for another ticket in addition to the those for Jira 896.
Bug fixed:
This feature was implemented in PR #457, merged, and released
in Deneb.RC1. However, it was discovered that the logic was
inverted and was report as Git issue #476. BLE scan() and
input parameter withDuplicates is true means reporting all
Peripherals detected regardless how many times it was reported.
Code mods:
Peripherals if withDuplicates is true. Otherwise,
just scan for new ones.
libraries/CurieBLE/src/internal/BLEDeviceManager.cpp
withDuplictes correct meaning.