-
Notifications
You must be signed in to change notification settings - Fork 51
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 to sdk 8.1 #77
Conversation
thakns for this pull request. I'll review this shortly. |
@@ -49,7 +49,7 @@ | |||
{ \ | |||
__asm( \ | |||
"svc %0\n" \ | |||
"bx r14" : : "I" ((uint32_t)number) : "r0" \ |
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 seems that this file is generating way too many warning of type -Wunused-function
and -Wunused-parameter
. Any ideas on how to modify this code to fix the problem?
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.
This is a very old issue from SDK 5.2. See this post on the devzone. The cast to uint16_t
works for me. https://devzone.nordicsemi.com/question/6950/upgrading-to-sdk-520-breaks-build/
Btw, this is still a known issue in SDK 10, see my answer here: https://devzone.nordicsemi.com/question/57411/what-are-nrf51-sdk-100-known-issues/?answer=58295#post-id-58295
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.
@andresag01 are these warnings new? I don't see any change in this PR to generate new warnings.
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 : they are not new. These are the same warnings generated using the previous version of the sdk. I suppose what I am asking is how feasible is to modify these files to fix this if they are meant to be easily replaceable...
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 there an easy fix to these warnings?
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.
@metc I am not sure the uint16_t
fix works anymore. I replaced the uint32_t
by uint16_t
in the file nrf_svc.h
and I got the same warnings.
all this looks good to me. Since we've incorporated some changes to pstorage, it would be nice if we could re-test with the URI_Beacon (or any other demo which tries to use persistent storage). |
could you also confirm that we don't need to update our softdevice hex files to match this new SDK? I believe we're already taking our softdevice from SDK8.1, but I could be wrong about that. |
@rgrover @LiyouZhou : The softdevice files we are using are these ones. I am not sure how the versioning works but the file names say:
|
@LiyouZhou please check if we could benefit from a different softdevice from SDKv8.1. |
@LiyouZhou could you please also review the release notes for v8.1 of the Nordic SDK to help clarify what benefits exist in upgrading to it? We will be upgrading the Nordic SDK over time, but we should try to document any major benefits at each upgrade step--if such benefits exist. |
@rgrover I have just tried the URIBeacon demo in ble-examples and it seems to behave as expected i.e. I am able to connect, configure something. Then when I reset the device my configuration is retained, I assume because it was saved in permanent storage. |
ok. I'm satisfied with our upgradability to v8.1 of the SDK. I'd just like confirmation about the softdevice hex file. |
@rgrover I can confirm current headers are from s130_nrf51_1.0.0 softdevice. Softdevice have some definition changes, be I see no obvious performance benefit. I would like to keep the softdevice upgrade a separate effort as it needs more thought and investigation. S130v2 is at alpha release stage and may not be fully compatible with sdk 8.1. |
@rgrover Here are some benefits from the sdk v8.1 release notes: Highlights:
Changes:ANT + BLE (dual stack):
Fixed issues:
Known issues:
|
it all seems good to me. I'll go ahead with v8.1 |
@rgrover @andresag01 Please review.