-
Notifications
You must be signed in to change notification settings - Fork 24
lib: add ble_radio_notification and sample #351
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
Conversation
74df57c to
2f5eb93
Compare
|
You can find the documentation preview for this PR here. |
2f5eb93 to
e110194
Compare
e110194 to
afdbe0a
Compare
8c2a381 to
974e8c4
Compare
974e8c4 to
15c45c2
Compare
CODEOWNERS
Outdated
| /tests/lib/bm_zms/ @nrfconnect/ncs-bm @rghaddab | ||
| /tests/lib/ble_qwr/ @nrfconnect/ncs-bm | ||
| /tests/lib/ble_racp/ @nrfconnect/ncs-bm | ||
| /tests/lib/ble_radio_notif/ @nrfconnect/ncs-bm |
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.
alignment is off
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.
Corrected.
| #define STATIC static | ||
| #endif | ||
|
|
||
| LOG_MODULE_REGISTER(ble_radio_ntf, CONFIG_BLE_RADIO_NTF_LOG_LEVEL); |
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.
| LOG_MODULE_REGISTER(ble_radio_ntf, CONFIG_BLE_RADIO_NTF_LOG_LEVEL); | |
| LOG_MODULE_REGISTER(ble_radio_notification, CONFIG_BLE_RADIO_NOTIFICATION_LOG_LEVEL); |
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.
Renamed
lib/ble_radio_notification/Kconfig
Outdated
| IRQ Priority level 0 and 4 are reserved by the SoftDevice. | ||
|
|
||
|
|
||
| module=BLE_RADIO_NTF |
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.
| module=BLE_RADIO_NTF | |
| module=BLE_RADIO_NOTIFICATION |
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.
Renamed.
| range 50 5500 | ||
| default 800 | ||
|
|
||
| module=BLE_RADIO_NTF_SAMPLE |
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.
fix names as above
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.
Fixed
| void setUp(void) | ||
| { | ||
| } | ||
| void tearDown(void) | ||
| { | ||
| } | ||
|
|
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.
| void setUp(void) | |
| { | |
| } | |
| void tearDown(void) | |
| { | |
| } |
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.
Removed.
15c45c2 to
cf7859c
Compare
| /* Application event handler for handling Radio Notification events. */ | ||
| static ble_radio_notification_evt_handler_t evt_handler; | ||
|
|
||
| STATIC void radio_notification_isr(const void *arg) |
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 we using STATIC here?
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.
Se definition above. It is to be able to call the event handler from the unit test.
| } | ||
| } | ||
|
|
||
| uint32_t ble_radio_notification_init(uint32_t distance, |
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.
int ?
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.
Not when we are returning nrf_error.
| type = NRF_RADIO_NOTIFICATION_TYPE_INT_ON_INACTIVE; | ||
| #endif | ||
|
|
||
| evt_handler = _evt_handler; |
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.
Should the user be able to unset the callback? If not, we could check against NULL here, and avoid doing at every interrupt before invoking the callback. Otherwise, If it should be possible to unset the callback, we could provide an explicit de-initialization function
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, will move the check for evt_handler to the initialization. I don't think having a callback-unset would provide much value, unless that is also disabling the IRQ. Though, in that case, if disabling while the radio is active, the radio active state might be messed up.
| #if defined CONFIG_BLE_RADIO_NOTIFICATION_ON_BOTH | ||
| radio_active = !radio_active; | ||
| #endif |
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.
nit: I would use the macro IS_ENABLED() here
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 don't do that anywhere else, only for runtime if's.
cf7859c to
a448920
Compare
a448920 to
45f9ef4
Compare
45f9ef4 to
93ae69f
Compare
|
@nordicjm Please have another look. |
| # | ||
| # SPDX-License-Identifier: LicenseRef-Nordic-5-Clause | ||
| # | ||
| menuconfig BLE_RADIO_NOTIFICATION |
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.
I just realized that this is not expressing the dependency on SoftDevice.
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.
Added. Note that this applies for most of the bluetooth libraries. So we should clean it up.
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.
@eivindj-nordic What do you mean?
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 do not have this dependency for many of the other ble libraries. See connection parameters etc.
137e477 to
a34885f
Compare
fe2b67f to
ba0b865
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.
Approving, but please address comments.
ba0b865 to
bad6849
Compare
|
@nordicjm Please have another look. |
bad6849 to
aa43dd2
Compare
include/ble_radio_notification.h
Outdated
|
|
||
| #include <stdint.h> | ||
| #include <stdbool.h> | ||
| #include <nrf_soc.h> |
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.
It seems this is not needed here
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.
Moved to c-file.
| CONFIG_LOG_BACKEND_BM_UARTE=y | ||
|
|
||
| CONFIG_SOFTDEVICE=y | ||
| CONFIG_NRF_SDH=y |
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.
just CONFIG_SOFTDEVICE should work now
| CONFIG_BLE_ADV_FAST_ADVERTISING_INTERVAL=400 | ||
| CONFIG_BLE_ADV_SLOW_ADVERTISING_INTERVAL=1600 |
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.
Do we need to override defaults here?
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.
Strictly no, though it makes it more observable by looking at the LED. With default intervals it blinks quite fast regardless of the advertising mode.
| NRF54L15_XXAA | ||
| CONFIG_BLE_RADIO_NOTIFICATION_ON_BOTH=1 | ||
| CONFIG_BLE_RADIO_NOTIFICATION_IRQ_PRIO=5 | ||
| SWI02_IRQn=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.
uhm why SWI02_IRQn=0 (equal zero?)
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.
Just needed to be defined to something, updated to 30 as is what it is defined to for nRF54l15.
8cc6735 to
e6dd6fd
Compare
|
@nordicjm @nrfconnect/ncs-bm-doc Please review! |
Add ble_radio_notification library and sample. Signed-off-by: Eivind Jølsgard <[email protected]>
4a5b190 to
570b607
Compare
Add ble_radio_notification library and sample.