-
Notifications
You must be signed in to change notification settings - Fork 17
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
Update files to Nordic sdk 9.0.0 #8
Conversation
@@ -39,7 +39,7 @@ | |||
#include <string.h> | |||
#include "nordic_common.h" | |||
#include "app_error.h" | |||
|
|||
#include "ble.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.
is this the ble.h
from Nordic-SDK as opposed to ble/BLE.h
? is there a potential for misinterpretation?
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.
@rgrover In theory, all files including ble.h should only find ble.h as BLE.h is not exposed with extraIncludes. All files intending to use BLE.h would use ble/BLE.h. The problem only comes if a file, intending to use BLE.h, uses #include BLE.h
, on mac os, this can find ble.h 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.
and on windows too right?
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.
@jrobeson Yes it can be a problem on windows as well. So you should always use #include ble/BLE.h
to use the BLE API.
@LiyouZhou please confirm that the bootloader is still able to build and work correctly with this new SDK. I have a feeling you'll find that the bootloader doesn't work any more unless you fix the constants in Please also ensure that the memory consumption doesn't go up with this new SDK. Building any of our examples should show the heap sizes at the end. This is important. |
I would like you to discuss your approach with others members of the team. Please document the affects of switching to the new SDK; we need to ensure that we don't affect existing functionality--such as use of |
@LiyouZhou please also be mindful that we need to hand-apply the changes from SDKv9 to the ble-nrf51822 repository on mbed.org. That repository will no longer automatically sync with the changes to the SDK. |
@@ -68,8 +69,6 @@ | |||
#define BLE_UUID_RUNNING_SPEED_AND_CADENCE 0x1814 /**< Running Speed and Cadence service UUID. */ | |||
#define BLE_UUID_SCAN_PARAMETERS_SERVICE 0x1813 /**< Scan Parameters service UUID. */ | |||
#define BLE_UUID_TX_POWER_SERVICE 0x1804 /**< TX Power service UUID. */ | |||
#define BLE_UUID_IPSP_SERVICE 0x1820 /**< Internet Protocol Support service UUID. */ |
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.
Interesting this UUID has been removed from the Nordic SDK. I wonder if that has something to do with their new IP strategy.
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.
@LiyouZhou could you check if this UUID is still removed in v10?
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.
@rgrover @marcuschangarm It is reintroduced in v10
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.
must be an oversight then.
@rgrover |
@LiyouZhou you claim: |
@rgrover by looking at the obj-dump the major refactorying of pstorage.c caused this increase. There is a lot of change, hence it is difficult to tell exactly what cause memory increase. |
@LiyouZhou ok. Thanks for this dive. 24bytes isn't too much to worry about. |
@rgrover hold off merging, need to talk to jonny and make a small change. |
sdk10 has a fix for "App_pwm occasionally gives inverted signal" |
@jrobeson I'm currently in the process of upgrading to v10. Should be up shortly. |
Update files to Nordic sdk 9.0.0 - Merging to prepare for v10 PR
Tests:
clitest - all expected passes pass, all expected fails fail.
build - all ble-examples build successfully
pstorage - tested against https://github.com/andresag01/ble-examples-1/tree/eddystone_pstorage and works
dfu - tested to work
Highlight from sdk 9 release notes:
Changes
Fixed issues:
freed while sending ACK or NACK.
Known issues:
S130. This must be decided at compile time.
connection interval of 11.25 ms. The application cannot handle
connection intervals lower than 11.25 ms and may undergo a system
reset in the middle of a firmware update.
Workaround: If you face unexpected disconnects during the firmware
update process, consider increasing the connection interval used
by the master.
the documentation, can lead to the DFU process hanging or returning
an error when used with Master Control Panel 3.8 and newer.
@ARMmbed/ble-owners Please review.