dcd_nrf5x: make it compile also with old Nordic SDK #2263
Conversation
| #if defined(SOFTDEVICE_PRESENT) && defined(OLD_NORDIC_SDK) | ||
| if (sd_nvic_EnableIRQ(USBD_IRQn) != NRF_SUCCESS) | ||
| { | ||
| NVIC_EnableIRQ(USBD_IRQn); | ||
| } | ||
| #else |
There was a problem hiding this comment.
do we really need it this, USB isn't restricted peripheral by SoftDevice ? If it is required to use sd_nvic_EnableIRQ() when sd is enabled, we should use this instead
#ifdef SOFTDEVICE_PRESENT
if ( is_sd_enabled() ) sd_nvic_EnableIRQ(USBD_IRQn)
There was a problem hiding this comment.
I'm actually not absolutely sure if this is required. This is more a defensive approach. If one guarantees, that "NVIC_EnableIRQ(USBD_IRQn)" works for all Nordic SDK, I'll remove those #ifdefs immedately. Same is true, if you accept the PR only if this construct is removed of course ;-)
is_sd_enabled() is not available in all Nordic SDKs. Before using another #ifdef I prefer a "generic" approach which works for all Nordic SDKs.
| @@ -57,6 +56,13 @@ | |||
| #include "mcu/mcu.h" | |||
| #endif | |||
|
|
|||
| #if 1000*MDK_MAJOR_VERSION + MDK_MINOR_VERSION <= 8040 | |||
| // Nordic actually has generated a mess here: nrfx==1.9.0 has MDK 8.40.3 while nrfx==2.0.0 has MDK 8.29.0 | |||
There was a problem hiding this comment.
They concurrently developed both 1.x and 2.x together since it is breaking changes. 1.9 is actually released after 2.0. It is supprising that nordicsemi didn't include NRFX_VERSION in their codebase.
This check make version v2.0 to v2.4 not compilable with tinyusb which is not correct. The correct check must ensure:
- all v2.x compile as it is
- additionally some of 1.x can be compiled, not all since there is a slash of MDK version.
From what I found MDK version for releases are
- v2.6.0: 8.44.1
- v2.5.0: 8.40.2
- v2.4.0: 8.37.0
- v2.3.0: 8.35.0
- v2.2.0: 8.32.1
- v2.1.0: 8.30.2
- v2.0.0: 8.29.0
- v1.9.0: 8.40.3
- v1.8.6: 8.35.0 <- conflict with 2.3.0
- v1.8.5: 8.32.3
- v1.8.4: 8.32.1 <- conflict with 2.2.0
- v1.8.2: 8.32.1 <- conflict with 2.2.0
- v1.8.1: 8.27.1
Therefore the correct check would be:
- MDK < 8.29.0 (v2.0), MDK != 8.40.3, 32.3, 27.1
v1.x conflict with v1.x is not supported since there is no way to tell the difference. User of those version must upgrade to other 1.x version.
PS: Do we need to support all 1.x version, if not, we can just only inlcude the 1.9.0 using its MDK version. It is realative easy to upgrade to 1.9
There was a problem hiding this comment.
I've changed the code so that the old API is correctly detected. But, I put priority on detection of the old API in case of conflicts, because TMO most people now keep the versions current because handling of the Connect SDK is different.
Upgrading to nrfx 1.9.0 is not that easy, because the nrfx is part of the Nordic SDK.
| @@ -253,13 +258,27 @@ void dcd_init (uint8_t rhport) | |||
| void dcd_int_enable(uint8_t rhport) | |||
| { | |||
| (void) rhport; | |||
| #if defined(SOFTDEVICE_PRESENT) && defined(NORDIC_SDK_OLD_API) | |||
There was a problem hiding this comment.
please remove these since they are not tested.
| // Unfortunately there are API differences between nrfx<2.0.0 and nrfx>=2.0.0 | ||
| // Nordic actually has generated a mess here: nrfx==1.9.0 has MDK 8.40.3 while nrfx==2.0.0 has MDK 8.29.0. | ||
| // See the below statement to catch all nrfx versions with an old API. | ||
| #define _MDK_VERSION 10000*MDK_MAJOR_VERSION + 100*MDK_MINOR_VERSION + MDK_MICRO_VERSION |
There was a problem hiding this comment.
unless there is a good reason (with explanation), for case where mdk conflicts, nrfx v2 should be used. If it is due to sdk tighted version, please list the sdk + mdk here.
- allow to force nrfx version with CFG_TUD_NRF_NRFX_VERSION=1
hathach
force-pushed
the
compile-with-old-nordic-sdk
branch
from
fe00e81
to
d82ee79
Compare
Describe the PR
With this PR it is also possible to compile the dcd_nrf5x with old Nordic SDKs. The current version of the driver uses API calls to nrf_clock with two parameters while the old SDK only uses one parameter for the call.
For correct behavior the NVIC calls also has to be directed thru the softdevice.
Additional context