sys/nrf_sf_radio: Add new module - #22447
Conversation
|
Hey @merlins123, thank you for your first contribution! We really appreciate it! If you haven't already, please take a look at our contributing guidelines before the review process starts. Also, due to how the GitHub review system works, please avoid force-pushing or squashing your commits unless asked to by a maintainer (or unless your commit is still in "draft commit" stage). Lastly, make sure to comply with our AI Policy when using AI. Your pull request will be reviewed as soon as possible. |
92c089f to
523e151
Compare
sys/nrf_sf_radio: add synchronous flooding physical layer and link layer module tests/nrf_sf_radio/: add test for sys/nrf_sf_radio tests/sys/nrf_sf_radio: move test module refresh
523e151 to
c77657e
Compare
crasbe
left a comment
There was a problem hiding this comment.
To be honest I'm not quite sure I see the point of this module. It is highly specific to the nRF52 (which means it should probably not be placed in sys) and pretty specific in the configuration options that are preset.
The model lacks general documentation about how it should be used and what it can be used for, what configuration options exist and the fact that it allocates two of the hardware timers.
Especially considering this is about BLE, I wonder if there aren't some initialization functions from NimBLE that could be reused instead.
| /* | ||
| * Copyright (C) 2026 Xin He | ||
| * | ||
| * This file is subject to the terms and conditions of the GNU Lesser | ||
| * General Public License v2.1. See the file LICENSE in the top level | ||
| * directory for more details. | ||
| */ |
There was a problem hiding this comment.
| /* | |
| * Copyright (C) 2026 Xin He | |
| * | |
| * This file is subject to the terms and conditions of the GNU Lesser | |
| * General Public License v2.1. See the file LICENSE in the top level | |
| * directory for more details. | |
| */ | |
| /* | |
| * SPDX-FileCopyrightText: 2026 Xin He | |
| * SPDX-License-Identifier: LGPL-2.1-only | |
| */ |
The copyright headers should be in SPDX format, see #21515 and https://github.com/RIOT-OS/RIOT/blob/master/CODING_CONVENTIONS.md#documentation for more information.
There was a problem hiding this comment.
Please adapt the other files accordingly.
| #ifndef NRF_SF_RADIO_LINK_RADIO_H | ||
| #define NRF_SF_RADIO_LINK_RADIO_H |
There was a problem hiding this comment.
The old style header guards should not be used anymore, use #pragma once instead.
See also: https://github.com/RIOT-OS/RIOT/blob/master/CODING_CONVENTIONS.md#header-guards
There was a problem hiding this comment.
Please adapt the other files accordingly.
| * @ingroup sys_nrf_sf_radio | ||
| * @{ | ||
| * | ||
| * @file link_radio.h |
There was a problem hiding this comment.
| * @file link_radio.h | |
| * @file |
It's not necessary to specify the actual file name. This will just cause issues when/if the file is renamed.
There was a problem hiding this comment.
Please adapt the other files accordingly.
| * @return true if the transmission completed before the end time | ||
| * @return false if the transmission failed or timed out |
There was a problem hiding this comment.
| * @return true if the transmission completed before the end time | |
| * @return false if the transmission failed or timed out | |
| * @retval true if the transmission completed before the end time | |
| * @retval false if the transmission failed or timed out |
For discrete return values, the @retval command can and should be used.
There was a problem hiding this comment.
Please adapt the other function documentation headers accordingly.
| * @return Corrected ADDRESS timestamp in timer ticks on success | ||
| * @return 0 on timeout or CRC failure |
There was a problem hiding this comment.
| * @return Corrected ADDRESS timestamp in timer ticks on success | |
| * @return 0 on timeout or CRC failure | |
| * @return Corrected ADDRESS timestamp in timer ticks on success | |
| * @retval 0 on timeout or CRC failure |
It is also possible to use both @return and @retval.
| * @return true if a valid packet was received before the end time | ||
| * @return false if no valid packet was received or reception timed out |
There was a problem hiding this comment.
This is not what the function actually returns. It returns numeric values.
| uint32_t rx_window_end_ticks, | ||
| uint32_t rx_end_ticks); |
There was a problem hiding this comment.
| uint32_t rx_window_end_ticks, | |
| uint32_t rx_end_ticks); | |
| uint32_t rx_window_end_ticks, | |
| uint32_t rx_end_ticks); |
| #ifndef NRF_SF_RADIO_ACCESS_ADDRESS | ||
| # define NRF_SF_RADIO_ACCESS_ADDRESS (0x8367BED6u) | ||
| #endif |
There was a problem hiding this comment.
Is that address something that would be expected to change or is that the address of the internal peripheral?
There was a problem hiding this comment.
It should be the standard ble advertising access address. I changed it for test to to avoid environmental interference. I will change it back.
| NRF_RADIO->MODE = RADIO_MODE_MODE_Ble_1Mbit; | ||
| uint32_t modecnf0 = (RADIO_MODECNF0_DTX_Center << RADIO_MODECNF0_DTX_Pos); | ||
| #if NRF_SF_RADIO_FAST_RAMPUP | ||
| modecnf0 |= (RADIO_MODECNF0_RU_Fast << RADIO_MODECNF0_RU_Pos); | ||
| #else | ||
| modecnf0 |= (RADIO_MODECNF0_RU_Default << RADIO_MODECNF0_RU_Pos); | ||
| #endif | ||
| NRF_RADIO->MODECNF0 = modecnf0; | ||
|
|
||
| NRF_RADIO->TXPOWER = (RADIO_TXPOWER_TXPOWER_0dBm << RADIO_TXPOWER_TXPOWER_Pos); | ||
| NRF_RADIO->FREQUENCY = ble_hw_frequency_channels[37]; | ||
| NRF_RADIO->DATAWHITEIV = 37; |
There was a problem hiding this comment.
The mode, TX power and frequency would be something worth having adjustable, wouldn't it?
There was a problem hiding this comment.
That's true. I'll add this part.
There was a problem hiding this comment.
That test has rather... little coverage of the module 🤔
Also it is missing the copyright and doxgen header.
mguetschow
left a comment
There was a problem hiding this comment.
Thanks for your PR!
I agree with @crasbe here, your PR is lacking documentation and a rationale why this is a fitting addition for RIOT.
Wouldn't it make sense to design a hardware-independant API (or reuse / extend one of the available BLE-ones) for your use-case, and then just implement it for the different hardware? (Why) is it not possible to build on top of existing BLE APIs like nimble?
|
Hi everyone,
Thanks for the feedback. I agree that the current module is nRF-specific,
and `sys/` may not be the best location if RIOT prefers to keep `sys/`
mostly platform-independent.
The motivation for this module is to factor out the low-level
RADIO,TIMER,PPI control that is commonly needed by synchronous flooding
protocols on nRF-based platforms. These protocols require deterministic
TX/RX scheduling at slot boundaries and direct access to nRF RADIO
events/tasks, timers, and PPI/shortcuts. This timing-critical control is
not exposed by the existing BLE stack APIs.
I considered using NimBLE, but NimBLE abstracts packet transmission and
reception through the BLE stack and controller. This means the application
cannot precisely control radio state transitions and transmission timing at
the level required for synchronized flooding. For this reason, the
timing-critical part of the module is implemented below the BLE stack and
directly controls the nRF radio peripherals.
That said, I agree that the current version is still too specific in its
current form:
1. The module is tied to nRF hardware.
2. Some configuration values are preset for my experimental setup.
3. The documentation should better describe the intended use, configuration
options, and hardware resources used by the module.
I can address this by moving the module to a more appropriate nRF-specific
location, if that is preferred, and by reducing or generalizing the preset
configuration values. I can also add documentation that explicitly
describes the configuration options and the required hardware resources.
Best Regards,
Xin He
mguetschow ***@***.***> 于2026年7月7日周二 17:33写道:
… ***@***.**** requested changes on this pull request.
Thanks for your PR!
I agree with @crasbe <https://github.com/crasbe> here, your PR is lacking
documentation and a rationale why this is a fitting addition for RIOT.
Wouldn't it make sense to design a hardware-independant API (or reuse /
extend one of the available BLE-ones) for your use-case, and then just
implement it for the different hardware? (Why) is it not possible to build
on top of existing BLE APIs like nimble?
—
Reply to this email directly, view it on GitHub
<#22447?email_source=notifications&email_token=BFZKPUGFENXDW5ZYYASE4UL5DUJ2XA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRUGY3DGNZZGI42M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#pullrequestreview-4646637929>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BFZKPUHT5HOLG3BDCXUXW6T5DUJ2XAVCNFSNUABDKJSXA33TNF2G64TZHM3TGNZQGI2DCO2JONZXKZJ3GQ4DCOJQGM2DMNRQUF3AE>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/BFZKPUFHG3PBPX7IDWALK335DUJ2XA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRUGY3DGNZZGI42M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/BFZKPUHS2SFL7VK2C5IKK4D5DUJ2XA5CNFSNUABKM5UWIORPF5TWS5BNNB2WEL2QOVWGYUTFOF2WK43UKJSXM2LFO4XTINRUGY3DGNZZGI42M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Have you had a look at https://github.com/RIOT-OS/RIOT/tree/master/cpu/nrf5x_common/radio/nrfble? |
|
I looked into `cpu/nrf5x_common/radio/nrfble`. I think some parts can be
reused or mirrored, especially the BLE channel mapping, access-address
setup, TX power mapping, and the use of `clock_hfxo_request()` for enabling
the HFXO.
However, the `nrfble` driver itself is built around the netdev BLE model,
interrupt-driven completion callbacks, and the BLE alternating RX/TX
sequence with fixed TIFS. This does not fit the synchronous flooding use
case, where TXEN/RXEN and START need to be scheduled at absolute timer
ticks through TIMER/PPI and where READY/ADDRESS/END timestamps are part of
the protocol timing.
So I can adapt the configuration style and use `clock_hfxo_request()`, but
I do not think the `nrfble` send/receive path can be reused directly for
this module.
mguetschow ***@***.***> 于2026年7月8日周三 15:33写道:
… *mguetschow* left a comment (RIOT-OS/RIOT#22447)
<#22447 (comment)>
I considered using NimBLE, but NimBLE abstracts packet transmission and
reception through the BLE stack and controller. This means the application
cannot precisely control radio state transitions and transmission timing at
the level required for synchronized flooding. For this reason, the
timing-critical part of the module is implemented below the BLE stack and
directly controls the nRF radio peripherals.
Have you had a look at
https://github.com/RIOT-OS/RIOT/tree/master/cpu/nrf5x_common/radio/nrfble?
—
Reply to this email directly, view it on GitHub
<#22447?email_source=notifications&email_token=BFZKPUGEMYRKFPIVOL5J5635DZERBA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJRGUZTEMRSGM32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-4915322237>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/BFZKPUDF4J4LICOJSMUG4FT5DZERBAVCNFSNUABDKJSXA33TNF2G64TZHM3TGNZQGI2DCO2JONZXKZJ3GQ4DCOJQGM2DMNRQUF3AE>
.
Triage notifications, keep track of coding agent tasks and review pull
requests on the go with GitHub Mobile for iOS
<https://github.com/notifications/mobile/ios/BFZKPUGAYFYX6BSVKUQTU2D5DZERBA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJRGUZTEMRSGM32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJKTGN5XXIZLSL5UW64Y>
and Android
<https://github.com/notifications/mobile/android/BFZKPUDHPN5SIOIAI4B4GKT5DZERBA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTIOJRGUZTEMRSGM32M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLTGN5XXIZLSL5QW4ZDSN5UWI>.
Download it today!
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
aabe6bf to
aa0fc46
Compare
aa0fc46 to
de386e5
Compare
|
After checking the radio support currently available in RIOT, I could not CC26x2 does not currently have a BLE radio backend in RIOT. The existing Therefore, with the interfaces currently available in RIOT, this Given your feedback, I agree that this hardware-specific implementation |
Contribution description
Added a new module, sys/nrf_sf_radio. This module encapsulates the common physical-layer and link-layer radio control functions used for synchronous flooding.
This module is designed for BLE flooding and for development boards such as the nRF51 and nRF52 that support the
NRF_PPIperipheral.By introducing this module, developers can focus on implementing higher-layer flooding protocols without having to rebuild the low-level precise radio control mechanisms from scratch.
Testing procedure
run
make -C tests/sys/nrf_sf_radio BOARD=adafruit-feather-nrf52840-sense UF2_SOFTDEV=DROP clean all flash testto test the module.
UF2_SOFTDEV=DROPwas used because the tested board does not have a SoftDevice installed. The test does not require SoftDevice functionality.The expected output is
Issues/PRs references
None.
Declaration of AI-Tools / LLMs usage:
AI-Tools / LLMs that were used are: