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

[NRF5 + NRF52840]: Merge nrf52840 to [NRF5] sources #4245

Merged
merged 8 commits into from
May 15, 2017

Conversation

nvlsianpu
Copy link
Contributor

Description

Former nRF52840 was based on its private api hal drivers and ble implementation. This PR merge those nRF52840 changes to NRF5.

nRF5 SDK base codes and softdevice versions are not changed for mcu targets.

changes:
renamed dir. NRF5/sdk -> NRF5/TARGET_SDK11
moved dir. NRF5_SDK13/sdk -> NR5/TARGET_SDK13
moved dir. NRF5_SDK13/TARGET_MCU_NRF52840 -> NRF5/TARGET_MCU_NRF52840

merged changes introduce to BLE port for compatibility with newest SD API
changes FEATURE_BLE(...)/NRF5_SDK13/.(c,h,cpp) -> FEATURE_BLE(...)/NRF5/.(c,h,cpp)

merged changes introduce to device hal driver port for compatibility with nRF5 SDK13
NRF5_SDK13/.(c,h) -> NRF5/.(c,h)

Added backward compatibility code to API implementation of gpio, pwm, serial, port, spi
Added backward compatibility code to BLE implementation of GAP.

Modified targets description in order to use moved files.

Purpose of changes

This PR purpose is to reduce Nordic maintenance effort. It introduce coherence between NRF51, NRF52, NRF52840 ports.

Status

READY

TARGET_NRF5_SDK13/sdk -> TARGET_NRF5/TARGET_SDK13
TARGET_NRF5/sdk -> TARGET_NRF5/TARGET_SDK11
TARGET_NRF5_SDK13/TARGET_MCU_NRF52840 -> TARGET_NRF5/TARGET_MCU_NRF52840
HAL driver: Add changes from needad for nrf52840 support
us_ticker, spi, sleep, serial, pwmout, pinmap, object, i2c, gpio, analogin

Add compatibility patches for:
- SoftDevice headers renamed (redirec by a few h files)
- sdk configuration (redirect by sdk_config.h files)
- renaming of func in softdevice handler module
….x,5.0.0-1.alpha by

Copy of changes from
features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NRF5_SDK13/source
to
features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NRF5/source
@nvlsianpu
Copy link
Contributor Author

I tested BLE applications for all Nordic Soc witch success.

mbed test are blocked by #4237. Although this tests pass if unwanted intel-hex record was removed from hex file "by hand".

@tommikas
Copy link
Contributor

A couple of NRF52 DK builds are failing. Neither build has the BLE feature included.

Compile [  8.2%]: btle.cpp
[Fatal Error] ble.h@1,23: .\nrf_ble.h: No such file or directory
[ERROR] In file included from ./mbed-os/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NRF5/source/btle/btle.h:27:0,
                 from ./mbed-os/features/FEATURE_BLE/targets/TARGET_NORDIC/TARGET_NRF5/source/btle/btle.cpp:20:
./mbed-os/targets/TARGET_NORDIC/TARGET_NRF5/TARGET_MCU_NRF52832/sdk/softdevice/s132/headers/ble.h:1:23: fatal error: .\nrf_ble.h: No such file or directory
 #include ".\nrf_ble.h"

Btw @theotherjimmy, getting [Warning] mbed_config.h@64,0: #1-D: last line of file ends without a newline for every file with armcc builds.

@0xc0170
Copy link
Contributor

0xc0170 commented Apr 28, 2017

Btw @theotherjimmy, getting [Warning] mbed_config.h@64,0: #1-D: last line of file ends without a newline for every file with armcc builds.

The patch is already for review, will be fixed soon

@tommikas
Copy link
Contributor

Cool, I figured you'd probably be aware of it already.

@theotherjimmy
Copy link
Contributor

@pan- could you review?

@nvlsianpu nvlsianpu changed the title [NRF5 + NRF52840]: Merge nrf52840 to [NRF5] suorces [NRF5 + NRF52840]: Merge nrf52840 to [NRF5] sources May 4, 2017
Copy link
Member

@pan- pan- left a comment

Choose a reason for hiding this comment

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

Thanks, it looks good. I leaved few comments you can address.
I still need to run the test suite on these changes.

@@ -46,15 +56,15 @@ gpio_cfg_t m_gpio_cfg[GPIO_PIN_COUNT];

static gpio_irq_handler m_irq_handler;
static uint32_t m_channel_ids[GPIO_PIN_COUNT] = {0};
uint32_t m_gpio_irq_enabled;
static uint32_t m_gpio_irq_enabled;
Copy link
Member

Choose a reason for hiding this comment

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

The type should be gpio_mask_t.

@@ -79,20 +89,26 @@ void gpio_init(gpio_t *obj, PinName pin)
m_gpio_cfg[obj->pin].used_as_gpio = true;
}

#ifdef TARGET_SDK11
Copy link
Member

Choose a reason for hiding this comment

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

Is it a polyfill for SDK V11 ? If so, can it be explicitly indicated.

#error not recognized gpio count for mcu
#endif

#define GPIO_PORT_COUNT (sizeof(m_gpio_pin_count)/sizeof(uint32_t))
Copy link
Member

Choose a reason for hiding this comment

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

little note, (sizeof(m_gpio_pin_count)/sizeof(m_gpio_pin_count[0])) is more correct because it doesn't assume the type of m_gpio_pin_count as uint32_t.

US_TICKER_INT_MASK |
NRF_RTC_INT_OVERFLOW_MASK);
#if DEVICE_LOWPOWERTIMER
LP_TICKER_INT_MASK |
Copy link
Member

Choose a reason for hiding this comment

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

Could you fix the indentation ?

#if DEVICE_LOWPOWERTIMER
LP_TICKER_INT_MASK |
#endif
US_TICKER_INT_MASK |
Copy link
Member

Choose a reason for hiding this comment

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

Indentation

@@ -0,0 +1 @@
#include "nrf_ble_hci.h"
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this header.

@@ -0,0 +1 @@
#include "nrf_ble_l2cap.h"
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this header.

@@ -0,0 +1 @@
#include "nrf_ble_ranges.h"
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this header.

@@ -0,0 +1 @@
#include "nrf_ble_types.h"
Copy link
Member

Choose a reason for hiding this comment

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

Please remove this header.

@@ -167,7 +173,11 @@ ble_error_t nRF5xGap::startAdvertising(const GapAdvertisingParams &params)
(params.getTimeout() > GapAdvertisingParams::GAP_ADV_PARAMS_TIMEOUT_MAX)) {
return BLE_ERROR_PARAM_OUT_OF_RANGE;
}

uint32_t err;
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to move back err and adv_para variables ?

s140 headers renamed form ble_* to nrf_ble_*,
Removed s130 and s132 headers named form ble_*
(Them had been added by #2ff572682798562e812015dc775b5896e0fda5a4)
Headers inclusinons were changed in order to meet above changes.

Revrted bad change in us_ticker.c:
use __disable_irq lock instead of core_util_critical_section_enter lock
for setting rtc1 tick for systick emulation as was good before.
@0xc0170
Copy link
Contributor

0xc0170 commented May 11, 2017

/morph test

@pan-
Copy link
Member

pan- commented May 11, 2017

test ble

{ 
    "targets": ["NRF51_DK", "NRF52_DK"],
    "toolchains": ["GCC_ARM", "ARM"]
}

@mbed-bot
Copy link

Result: SUCCESS

Your command has finished executing! Here's what you wrote!

/morph test

Output

mbed Build Number: 188

All builds and test passed!

@theotherjimmy
Copy link
Contributor

@pan- Did IAR get tested?

@adbridge
Copy link
Contributor

This PR didn't have examples or exporters tested

@nvlsianpu
Copy link
Contributor Author

https://github.com/ARMmbed/mbed-os-example-ble work fine on nRF52840_DK (although BLE_Button and BLE_GAPButton require new definition of buton pin in mbed_app.json):

"NRF52840_DK": {
            "ble_button_pin_name": "BUTTON1"
        }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants