-
Notifications
You must be signed in to change notification settings - Fork 429
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 dualstack softdevice support #151
base: master
Are you sure you want to change the base?
Conversation
- Allows dualstack softdevices to use the bootloader properly (OTA updates, serial updates) - Users must obtain softdevice headers from the appropriate package at thisisant.com and must build against those headers
Select the correct softdevice name for each protocol for each SoC
Build is failing because of #149 (the build no longer attempts to create the _build directory). Either that commit needs to be reverted or updated. |
as said in #146 , Softdevice is not part of bootloader anymore. There is no need to build and merged with SD, you just flashed it with jlink or uf2. Unless there is anything else I am not aware of, this PR is not likely to got merged. |
@hathach sd_softdevice_enable will fault on S3xx builds if you do not include the ANT_LICENSE_KEY argument. It does not matter if it is or is not a part of the bootloader, you need to build against the correct softdevice headers to get proper bootloader functionality on S3xx softdevices. Without this change OTA DFU will not be functional with dualstack softdevices. That call jumps into softdevice, so technically, no you can't just upload any softdevice you want. The APIs are different for dualstack softdevices. It might work ok for serial, but as soon as you try to do OTA DFU, you will have major issues. Its the same API name, but the implementations are different... |
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 see, there is a legit reason to add make option, though I preferred to intput the actual SD=
for actual softdevice stack than the PROTOCOL
. That way when Nordic introduce new stack name for new chip (e.g optimized version for smaller chip like 810 we don't have to maintain the makefile). Here is what i think the PR should implement
- rename current
SD_NAME
to simplySD
- if
SD
is not input, default as it is i.e s132 for 832, s140 for 840 SD
is passed to make will change the SD flag, name, linker etc as the same convention as current script
Agreed. I will update the PR to allow users to enter an SD name. |
With this change it should be pretty easy to add new softdevices. We just need to add a new "else ifeq" for the new softdevice then add another "else ifeq" for the SoCs that support the new softdevice. |
@@ -109,25 +109,25 @@ Prerequisites | |||
To build: | |||
|
|||
``` | |||
make BOARD=feather_nrf52840_express all | |||
make BOARD=feather_nrf52840_express SD=s140 all |
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 remove those SD=s140
, you can add an additional build command to demonstrate using your ANT stack e.g
By default, the BLE stack S132/S140 is used, to build with a different softdevice stack,
make SD=....
@@ -328,7 +328,13 @@ static uint32_t softdev_init(bool init_softdevice) | |||
.accuracy = NRF_CLOCK_LF_ACCURACY_250_PPM | |||
}; | |||
|
|||
#ifdef BLE |
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 like to have an extra macro for this, please use the ANT_LICENSE_KEY for this as well. These macro should be removed from the makefile as well.
ifndef ANT_LICENSE_KEY
else | ||
$(error Sub Variant $(MCU_SUB_VARIANT) is invalid for softdevice $(SD)) | ||
endif | ||
else ifeq ($SD,) |
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.
Too many duplication here, this should be checked before the whole if, and SD should get the default only to S132/S140 only. Then the ifeq() sequence take care of all the CFLAGS.
freely available for evaluation use only. Garmin Canada must be contacted to obtain | ||
ANT licenses for commercial use. | ||
2. Place the contents of the softdevice package in the appropriate lib/softdevice folder. | ||
3. Rename the API folder to <SD name>_nrf52_6.1.1_API. |
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.
can you tell me what is the default name of the s332 and s340 name, I would prefer to have the makefile to use their default name from the extracted instead.
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 is looking good, though we still need a bit of changes for handling the default SD name and the removal of BLE macro.
Due to licensing, add a compile switch for creating dualstack softdevice builds. Anyone who wishes to build the dualstack-supported bootloader MUST obtain their own copy of the softdevice from thisisant.com. For evaluation purposes this is free, but anyone who wishes to use the dualstack softdevice commerically must purchase an ANT license.
These builds should be cross-compatible within dualstack softdevices, as long as they are compiled against the dualstack headers (i.e. if the bootloader is built against the s340 headers, the user SHOULD be able to load any s3xx series softdevice and maintain all bootloader functionality).
Single ANT stack support is not added. I was able to disable OTA DFU, but was not able to properly re-enable seiral DFU. This is something I can look at in the future if anyone files an issue requesting it.
Verifed with S140 and S340 v6.1.1 that I am able to:
One note on this: If it wasn't for the licensing issue we could have made this a runtime check, but defining ANT_LICENSE_KEY is not allowed by us (the user must download the softdevice and define that themselves in the correct API file).