Skip to content
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

Remove Maxim and SoftDevice BLE stacks which do not support the latest APIs #12674

Merged
merged 5 commits into from Apr 15, 2020

Conversation

LDong-Arm
Copy link
Contributor

@LDong-Arm LDong-Arm commented Mar 23, 2020

Summary of changes

  • Remove Nordic SoftDevice stack (used by nRF51* targets). Notes: nRF52* targets are unaffected as they use Cordio stack.
  • Remove the current Maxim BLE stack (until a newer version based on up-to-date Cordio stack becomes available).
  • Disable BLE features of affected targets from targets.json.

Note: Some references/macros for SoftDevice are still present in the Nordic SDK (targets/TARGET_NORDIC/*) and not cleanly separated with other components.

Impact of changes

  • BLE feature will be unavailable on nRF51* targets and temporarily unavailable on Maxim targets until the driver gets updated in the future.

Migration actions required

  • For nRF51* targets, use Mbed OS release 5.15 - future support for those targets are under review.
  • For nRF52* targets, ensure the default BLE configurations that enable Cordio stack is used.
  • For Maxim targets, use Mbed OS release 5.15 until the BLE driver gets updated.

Documentation

None


Pull request type

[] Patch update (Bug fix / Target update / Docs update / Test update / Refactor)
[] Feature update (New feature / Functionality change / New API)
[X] Major update (Breaking change E.g. Return code change / API behaviour change)

Test results

[] No Tests required for this change (E.g docs only update)
[x] Covered by existing mbed-os tests (Greentea or Unittest)
[] Tests / results supplied as part of this PR

Note: This only disables BLE feature from targets.


Reviewers

@pan- @evedon


@ciarmcom ciarmcom requested review from a team March 23, 2020 16:00
@ciarmcom
Copy link
Member

@LDong-Arm, thank you for your changes.
@ARMmbed/mbed-os-maintainers @ARMmbed/mbed-os-pan please review.

@LDong-Arm
Copy link
Contributor Author

LDong-Arm commented Mar 23, 2020

Hi @pan- @evedon, this is the PR to remove deprecated target-specific BLE stacks.
For nRF51* targets this only disables BLE - their complete removal (e.g. of components other than BLE) would be future work.

As in the description, this gets rid of the API implementation by SoftDevice but is not a complete clean-up of the lower level SDK code - do we care about this at the moment?

evedon
evedon previously approved these changes Mar 24, 2020
Copy link
Contributor

@evedon evedon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mergify mergify bot added needs: CI and removed needs: review labels Mar 24, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 24, 2020

FYI @ARMmbed/team-maximintegrated commit 7ef08df

0xc0170
0xc0170 previously approved these changes Mar 24, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Mar 24, 2020

CI started

@mbed-ci
Copy link

mbed-ci commented Mar 24, 2020

Test run: FAILED

Summary: 1 of 6 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_greentea-test

@mergify mergify bot added needs: work and removed needs: CI labels Mar 24, 2020
@0xc0170 0xc0170 added this to needs: work in Pull request triage Mar 25, 2020
@mergify mergify bot added needs: CI and removed needs: work labels Mar 30, 2020
@LDong-Arm
Copy link
Contributor Author

Hi @0xc0170, it looks like the CI failed at some HTTP request which is probably unrelated?

@0xc0170
Copy link
Contributor

0xc0170 commented Mar 31, 2020

test restarted

Copy link
Contributor

@donatieng donatieng left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @LDong-Arm, there are a few more things that need to be cleaned up in targets.json:

mbed-os/targets/targets.json

Lines 10947 to 10949 in ba6f89e

"BLE_STACK_SUPPORT_REQD",
"SOFTDEVICE_PRESENT",
"S130",

mbed-os/targets/targets.json

Lines 10972 to 10979 in ba6f89e

"MERGE_SOFT_DEVICE": true,
"EXPECTED_SOFTDEVICES_WITH_OFFSETS": [
{
"boot": "",
"name": "s130_nrf51_2.0.0_softdevice.hex",
"offset": 110592
}
],

Otherwise, LGTM

@mergify mergify bot added needs: work and removed needs: CI labels Mar 31, 2020
@LDong-Arm LDong-Arm requested a review from donatieng April 1, 2020 08:57
@mergify mergify bot dismissed stale reviews from evedon, 0xc0170, and donatieng April 1, 2020 08:57

Pull request has been modified.

@mergify mergify bot removed the needs: review label Apr 15, 2020
@0xc0170
Copy link
Contributor

0xc0170 commented Apr 15, 2020

@LDong-Arm Please rebase (recent merges made unfortunately a conflict here), will restart testing and this should go in today.

The current Maxim BLE driver only implements the old BLE API
which is deprecated and will be removed soon. Once an updated
BLE stack for Maxim becomes available, BLE feature can be
re-enabled.
…ets)

The Nordic SoftDevice BLE stack used by NRF51* targets only
supports legacy BLE APIs which we will removed completely.

Note: NRF52* targets which use Cordio BLE stack are unaffected.
@LDong-Arm LDong-Arm force-pushed the ble_remove_deprecated_targets branch from b475caf to 6954dbe Compare April 15, 2020 09:32
@LDong-Arm
Copy link
Contributor Author

@0xc0170 Rebased

@mergify mergify bot dismissed stale reviews from 0xc0170, donatieng, and bulislaw April 15, 2020 09:33

Pull request has been modified.

@LDong-Arm
Copy link
Contributor Author

@0xc0170 @donatieng @evedon @bulislaw I resolved the merged conflicts in targets.json. The diff is identical to what it was before, could you re-approved? Cheers.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 15, 2020

CI started

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 15, 2020

@0xc0170 @donatieng @evedon @bulislaw I resolved the merged conflicts in targets.json. The diff is identical to what it was before, could you re-approved? Cheers.

As it was just a rebase to resolve a conflict, all fine here.

@mbed-ci
Copy link

mbed-ci commented Apr 15, 2020

Test run: SUCCESS

Summary: 6 of 6 test jobs passed
Build number : 4
Build artifacts

@0xc0170 0xc0170 merged commit 58de040 into ARMmbed:master Apr 15, 2020
@mergify mergify bot removed the ready for merge label Apr 15, 2020
LDong-Arm added a commit to LDong-Arm/mbed-os that referenced this pull request Apr 27, 2020
…er used

Previously we overrode NRF targets to have a larger stack
due to memory required by SoftDevice. Having deprecated SoftDevice
in favour of Cordio for BLE (ARMmbed#12674), such requirement does not
apply anymore.
LDong-Arm added a commit to LDong-Arm/mbed-os that referenced this pull request Apr 27, 2020
…used

Previously we overrode nRF targets to have a larger stack
due to memory required by SoftDevice. Having deprecated SoftDevice
in favour of Cordio for BLE (ARMmbed#12674), such requirement does not
apply anymore.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Pull request triage
  
needs: work
Development

Successfully merging this pull request may close these issues.

None yet

7 participants