-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
manifest: sdk-zephyr: modules: hal_nordic: Add multi-instance DPPI and PPIB drivers #79857 #18073
base: main
Are you sure you want to change the base?
manifest: sdk-zephyr: modules: hal_nordic: Add multi-instance DPPI and PPIB drivers #79857 #18073
Conversation
The following west manifest projects have been modified in this Pull Request:
Note: This message is automatically posted and updated by the Manifest GitHub Action. |
CI InformationTo view the history of this post, clich the 'edited' button above Inputs:Sources:sdk-nrf: PR head: 2b97e068b1cb25fbc0da2f8276e30c9895fab4f5 more detailssdk-nrf:
nrfxlib:
zephyr:
Github labels
List of changed files detected by CI (45)
Outputs:ToolchainVersion: b81a7cd864 Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped;
|
98efc5f
to
70653fa
Compare
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 don't think we need to enable all of the DPPIC's and PPIB's 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.
Without having all PPIB and DPPICs enabled, the nrfx_gppi_dppi_ppib_lumos.c
won't build unfortunately. Maybe there could be a separate CONFIG_NRFX_GPPI
config that automatically enables those instances on Lumos.
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.
Yeah, then we should either use a separate CONFIG_NRFX_GPPI
, or add a followup task to remove the ones that are not needed. The commit message should explain the reasoning behind adding all these
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 the CONFIG_NRFX_GPPI
config and used it instead.
You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds. Note: This comment is automatically posted by the Documentation Publishing GitHub Action. |
#ifdef DPPI_PRESENT | ||
static nrfx_dppi_t dppi = NRFX_DPPI_INSTANCE(0); | ||
#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.
@e-rk The dppi
variable is used only as a kind of a "handle" to given resources represented by nrfx_dppi_t
.
See https://github.com/e-rk/hal_nordic/blob/d7317a9cd28a845dfd174e14c16ba3def1c02a78/nrfx/drivers/src/nrfx_dppi.c#L527
The nrfx_dppi_channel_alloc
has const
qualifier for the p_instance
, so the dppi
variable itself (not referring to resource it "points to") will not change.
nrfx_err_t nrfx_dppi_channel_alloc(nrfx_dppi_t const * p_instance, uint8_t * p_channel)
This is true for all other "nrfx_dppi_xxxxxx" calls.
Could the variable be moved to be inside appropriate function as a local variable?
In this case into mpsl_fem_utils_ppi_channel_alloc
so that it is a local non-static variable (on stack).
This could be done for all places you introduce the dppi
variable .
Justification:
- The
static nrfx_dppi_t dppi
will exist for application lifetime, but thedppi
object itself does not change. - The initializer
NRFX_DPPI_INSTANCE
is prepared by preprocessor. - There are multiple such variables, each takes space.
- There is only one underlying allocator per given DPPIC regardless of how many
nrfx_dppi_t dppi
we have used. - They are used rather in non time-critical code, there is no need to have them precalculated.
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 instance to local scope, with one exception.
70653fa
to
2889c7e
Compare
d3b3e83
to
b7a79c7
Compare
b7a79c7
to
9847e56
Compare
9847e56
to
e0650b2
Compare
9847e56
to
70b02a5
Compare
70b02a5
to
bdf0491
Compare
bdf0491
to
d38e0e3
Compare
d38e0e3
to
bda3a2a
Compare
bda3a2a
to
9b03ad7
Compare
9b03ad7
to
b969ad7
Compare
b969ad7
to
c3f8ffd
Compare
c3f8ffd
to
93cbe13
Compare
93cbe13
to
abc1e4d
Compare
abc1e4d
to
93cbe13
Compare
d3363f2
to
352adcb
Compare
352adcb
to
05d3dfb
Compare
05d3dfb
to
03aae38
Compare
03aae38
to
002d9b9
Compare
Updated the hal_nordic revision and adjusted the code base to use multi-instance DPPI API. Certain applications have to explicitly enable nrfx_dppi and nrfx_ppib support with Kconfigs. Updated the nrfxlib revision to add the softdevice_controller include paths globally. Signed-off-by: Rafał Kuźnia <[email protected]>
002d9b9
to
2b97e06
Compare
Automatically created by action-manifest-pr GH action from PR: nrfconnect/sdk-zephyr#2143