-
Notifications
You must be signed in to change notification settings - Fork 29
Add scan library #446
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
Add scan library #446
Conversation
|
You can find the documentation preview for this PR here. |
87751eb to
02a21e4
Compare
76f656c to
ced7dac
Compare
af74266 to
8fcd041
Compare
| CONFIG_BLE_SCAN_BUFFER_SIZE=31 | ||
| CONFIG_BLE_SCAN_NAME_MAX_LEN=32 | ||
| CONFIG_BLE_SCAN_SHORT_NAME_MAX_LEN=32 | ||
| CONFIG_BLE_SCAN_NAME_COUNT=1 | ||
| CONFIG_BLE_SCAN_APPEARANCE_COUNT=1 | ||
| CONFIG_BLE_SCAN_ADDRESS_COUNT=1 | ||
| CONFIG_BLE_SCAN_SHORT_NAME_COUNT=2 | ||
| CONFIG_BLE_SCAN_UUID_COUNT=1 | ||
| CONFIG_BLE_SCAN_INTERVAL=160 | ||
| CONFIG_BLE_SCAN_DURATION=0 | ||
| CONFIG_BLE_SCAN_WINDOW=80 | ||
| CONFIG_BLE_SCAN_SLAVE_LATENCY=0 | ||
| CONFIG_BLE_SCAN_MIN_CONNECTION_INTERVAL=6 | ||
| CONFIG_BLE_SCAN_MAX_CONNECTION_INTERVAL=24 | ||
| CONFIG_BLE_SCAN_SUPERVISION_TIMEOUT=3200 | ||
| CONFIG_BLE_SCAN_FILTER=1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are those added as compile definitions when Kconfig is available ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are not enabling the libraries (and all its dependencies) in the unit tests, only adding the target sources itself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that still doesn't explain why you're not using Kconfig.
You shouldn't be using compile definitions for Kconfig settings.
Instead, use the proper prj.conf, and if you need to loosen dependencies on a given Kconfig symbol for test purposes, then you can do this in the Kconfig file for the test.
For example like this:
# tests/lib/bluetooth/ble_scan/Kconfig
# Clear dependencies for BLE_SCAN and enable it to allow
# testing the features without enabling the library.
config BLE_SCAN
default y
source "Kconfig.zephyr"
If you copied this principle from elsewhere, then please create a jira and requesting the wrong principle to be cleaned up because other people might copy a bad example.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kconfigs are added as compile options in many other unit tests, both this repo, sdk-nrf and sdk-zephyr (search for -DCONFIG_ or add_compile_definitions(CONFIG_ in CMakeLists.txt).
I've updated this test, and will try to apply the same changes to the rest of the tests in this repository.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #519 to address comment in other tests.
| "__STATIC_INLINE") | ||
| cmock_handle(${SOFTDEVICE_INCLUDE_DIR}/ble_gatts.h | ||
| WORD_EXCLUDE | ||
| "__STATIC_INLINE") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if line wrapping the function then the end ) goes on its own line.
| "__STATIC_INLINE") | |
| "__STATIC_INLINE" | |
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These could be removed with SUPPRESS_INLINE_IMPLEMENTATION in the compile definitions.
| ) | ||
|
|
||
| set(SOFTDEVICE_VARIANT "s145") | ||
| set(SOFTDEVICE_INCLUDE_DIR "${ZEPHYR_NRF_BM_MODULE_DIR}/components/softdevice/${SOFTDEVICE_VARIANT}/${SOFTDEVICE_VARIANT}_API/include") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please line wrap. Line exceeds 100 chars/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
tejlmand
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix Kconfig.
8ed58d0 to
fe132b5
Compare
anhmolt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nits before I approve.
include/bm/bluetooth/ble_scan.h
Outdated
| /** | ||
| * @brief Initialize the Scan library. | ||
| * | ||
| * @param[out] scan Scan library instance. This structure must be supplied |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting. Also elsewhere.
86b38bc to
2a60858
Compare
Add scan library. Co-authored-by: Sondre Pettersen <[email protected]> Co-authored-by: Asil Zogby <[email protected]> Signed-off-by: Eivind Jølsgard <[email protected]>
|
|
||
| PREDEFINED = __DOXYGEN__ \ | ||
| CONFIG_BLE_QWR_MAX_ATTR=1 \ | ||
| CONFIG_BLE_SCAN_FILTER=1 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why are these needed?
|
|
||
| You can configure this library in one of the following modes: | ||
|
|
||
| * Simple mode without using filters or the whitelist. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
*allow list, whitelist is a non-inclusive term https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html#rule-a-2-inclusive-language
|
|
||
| #if defined(CONFIG_BLE_SCAN_FILTER) | ||
|
|
||
| #if (CONFIG_BLE_SCAN_NAME_COUNT > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the brackets here are not needed
| * | ||
| * @return true if whitelist is used. | ||
| */ | ||
| bool is_whitelist_used(struct ble_scan const *const scan_ctx); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as above
| * @ref ble_scan_filter_type_t. | ||
| * @retval BLE_ERROR_GAP_INVALID_BLE_ADDR If the BLE address type is invalid. | ||
| */ | ||
| int ble_scan_filter_set(struct ble_scan *const scan_ctx, uint8_t type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would you name this set if it adds one, name it add? Then it works with remove as well? Set means "this replaces the existing configuration" it does not mean to append
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the naming from nRF5 API. Will rename to _add.
| # | ||
|
|
||
| menuconfig BLE_SCAN | ||
| bool "BLE Scan" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| bool "BLE Scan" | |
| bool "BLE Scan [EXPERIMENTAL]" |
please ensure all experimental/deprecated symbols have this style with EXPERIMENTAL/DEPRECATED in the text
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, @anhmolt we need to do this for other libraries as well.
| @@ -0,0 +1,6 @@ | |||
| # Clear dependencies for BLE_SCAN and enable it to allow | |||
| # testing the features without enabling the library. | |||
| config BLE_SCAN | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as per other PR
|
Closing in favor of #514 that combines this with db discovery and hrs c. |
No description provided.