BLE central Role implementation and peripheral refactor #255

sgbihu · 2016-07-29T15:10:28Z

I'm sorry for I make the old pull request lost when try to reset my local branch. The logs are lost too.

eriknyquist · 2016-07-29T23:06:23Z

@sgbihu please read our commit message guidelines and follow them. You have 3 commits here; one of them adds a single line to a file, and the very next one removes that same line..

Can you please remove these two first commits?

Additionally, github is unable to show me the whole diff because there are so many files. This will make it difficult for us to review and add line comments. Could you consider breaking this one giant commit into smaller, logical commits? for example, 1 for Serial1 debug interface, 1 for BLE peripheral refactor, 1 for BLE central implementation & example sketches.

eriknyquist · 2016-07-29T23:07:32Z

README.md

@@ -53,3 +53,21 @@ them to the [support forum](https://forum.arduino.cc/index.php?board=103).
 > "How do I use this library?"

 > "I can't get this example sketch to work. What am I doing wrong?"
+
+# Enable debug interface on Serail1


Should be Serial1

Change the typo

eriknyquist · 2016-07-29T23:27:33Z

I'm sorry @sgbihu, I misspoke; the commit message on your large BLE commit is actually fine. So the only things that need to be done here are 1) remove the first two commits, and 2) split each task in the big BLE commit into separate commits. We can still review it, in the meantime, but if you can fix these things before it gets merged we would appreciate it.

eriknyquist · 2016-07-29T23:35:48Z

cores/arduino/printk.cpp

+#include <stdarg.h>
+#include "UARTClass.h"
+
+extern "C" void printk(const char *fmt, va_list args);


Is printk/vsnprintf really needed for BLE central?

Not for central. Only for debug.

I guess I'm just not sure why we need a new function to print formatted strings, when there are already some options for that available. Which specific features of vsnprintf are required to debug BLE Central?

Remove this function and use vsnprintf in cstdio.

Just want debug corelib easier.

eriknyquist · 2016-08-01T06:15:12Z

libraries/CurieBLE/src/BLECentralRole.cpp

+{
+    if (NULL == _ble_central_ins)
+    {
+        _ble_central_ins = new BLECentralRole();


I think _ble_central_ins should also be deleted in BLECentralRole::~BLECentralRole, otherwise that memory will be lost after using BLE

The BLECentralRole and BLEPeripheralRole is a hardware abstract layer for application. So the instance will always exist when start BLE. And this variable is static. No memory leak.

static BLECentralRole* _ble_central_ins;
Well, the pointer is static, but not what it points to. At the end of the day, malloc gets called when you use the new keyword (see the override in cores/arduino/new.cpp).

Yes, it call the malloc and get the memory resource. But the stack and Nordic’s code didn’t have API to deregister the BLE profile. And the stack also use the pointer that point to the CurieBLE LIB. If we release those resource, the stack will have wild pointer and we didn't have a API to clean it. So we can't release those resource. Our current design use singleton pattern. Those resource can be managed and never leak.

We also can release those resource. But the BLE can't call begin after call end. I can change the code if you think this is acceptable.

eriknyquist · 2016-08-01T17:41:00Z

libraries/CurieBLE/examples/Scanning/Scanning.ino

@@ -0,0 +1,129 @@
+/*


Can you please put some explanatory comments at the top of each example sketch? i.e. what the sketch does and maybe a quick summary of how it works. Look at the existing built-in example sketches for reference.

Add some comment to describe the function.

noelpaz · 2016-08-02T17:48:30Z

There is a runtime issue where a peripheral cannot be discovered or not advertising . Please see #258

eriknyquist · 2016-08-03T17:05:51Z

libraries/CurieBLE/examples/IMUBleNotification/IMUBleNotification.ino

+#include <CurieIMU.h>
+
+/*
+   This sketch example partially implements the standard Bluetooth Low-Energy Battery service.


Is this comment accurate?

Change the comments.

It looks the same... I'm talking about this line
This sketch example partially implements the standard Bluetooth Low-Energy Battery service

It's the same line from the BatteryMonitor sketch. Can you replace it with something that explains to the user what the sketch is doing? and likewise for the other examples?

calvinatintel · 2016-08-18T08:29:23Z

LED and LED Central sketches need some work. While LED Central is more robust, LED often fails to run, or even put the board in a state which requires a manual reset. I've had to upload LED multiple times and reboot the board multiple times in order to get it to pair with a device running LED Central. Once paired, I can reset the LED Central board repeatedly and the connection will resume as soon as the board boots back up. However, resetting the device running LED will break the pairing, and the LED running device doesn't recover until it's reflashed and reset many times.

noelpaz · 2016-08-19T15:31:11Z

@calvinatintel . Have you tried using another serial terminal for the LED sketch and then push the LEDCentral after it is running. I find running 2 instances of the IDE for Serial Monitor is problematic. Anyway I will test this again.

noelpaz · 2016-08-19T15:36:18Z

I have not counted but I have also noticed having to hit the reset button more than once and it is not exclusive to the BLE sketches. I am using 1.6.11

sgbihu · 2016-08-22T09:37:04Z

Hi @calvinatintel and @noelpaz ,

Can we move to Jira? I create a Jira tickets. https://jira.ndg.intel.com/browse/ATLEDGE-680

Now this issue is fixed.

noelpaz · 2016-08-22T23:48:27Z

That's okay -- I saw the fix for JIRA-680

noelpaz · 2016-08-22T23:55:31Z

libraries/CurieBLE/src/BLEPeripheralRole.cpp

@@ -295,7 +295,7 @@ void BLEPeripheralRole::handleParamUpdated(struct bt_conn *conn,
                        conn, interval, latency, timeout);
    if (_event_handlers[BLEUpdateParam])
    {
-        _central.setConnParames(interval, interval, latency, timeout);
+        _central.setConnectionParameters(interval, interval, latency, timeout);


@sgbihu -- do we have a test sketch that will test these changes

-Add debug interface on Serial1 -Update BLE stack and need update BLE's FW -Reconstruct the BLE peripheral base on V3 -Implement the BLE Central Role base on V3 -Implement some sketches for new BLE library -Add central read/write example -Add set advertising parameter interface -Add API to allow set up advertising after setup -Add interface to set the device name File description Porting from V3 system/libarc32_arduino101/common/atomic.h system/libarc32_arduino101/common/misc/byteorder.h system/libarc32_arduino101/drivers/atomic_native.c system/libarc32_arduino101/drivers/bluetooth/att.h system/libarc32_arduino101/drivers/bluetooth/bluetooth.h system/libarc32_arduino101/drivers/bluetooth/conn.h system/libarc32_arduino101/drivers/bluetooth/conn_internal.h system/libarc32_arduino101/drivers/bluetooth/gatt.h system/libarc32_arduino101/drivers/bluetooth/hci.h system/libarc32_arduino101/drivers/bluetooth/uuid.h system/libarc32_arduino101/drivers/rpc/rpc.h system/libarc32_arduino101/drivers/rpc/rpc_deserialize.c system/libarc32_arduino101/drivers/rpc/rpc_functions_to_ble_core.h system/libarc32_arduino101/drivers/rpc/rpc_functions_to_quark.h system/libarc32_arduino101/drivers/rpc/rpc_serialize.c system/libarc32_arduino101/framework/include/util/misc.h system/libarc32_arduino101/framework/src/os/panic.c system/libarc32_arduino101/framework/src/services/ble/conn.c system/libarc32_arduino101/framework/src/services/ble/conn_internal.h system/libarc32_arduino101/framework/src/services/ble/dtm_tcmd.c system/libarc32_arduino101/framework/src/services/ble/gap.c system/libarc32_arduino101/framework/src/services/ble/gatt.c system/libarc32_arduino101/framework/src/services/ble/hci_core.h system/libarc32_arduino101/framework/src/services/ble/l2cap.c system/libarc32_arduino101/framework/src/services/ble/l2cap_internal.h system/libarc32_arduino101/framework/src/services/ble/smp.h system/libarc32_arduino101/framework/src/services/ble/smp_null.c system/libarc32_arduino101/framework/src/services/ble/uuid.c system/libarc32_arduino101/framework/src/services/ble_service/ble_service.c system/libarc32_arduino101/framework/src/services/ble_service/ble_service_api.c system/libarc32_arduino101/framework/src/services/ble_service/ble_service_int.h system/libarc32_arduino101/framework/src/services/ble_service/ble_service_internal.h system/libarc32_arduino101/framework/src/services/ble_service/ble_service_utils.c system/libarc32_arduino101/framework/src/services/ble_service/gap_internal.h system/libarc32_arduino101/framework/src/services/ble_service/gatt_internal.h system/libarc32_arduino101/framework/src/services/ble_service/nble_driver.c

noelpaz · 2016-09-08T04:59:15Z

@sgbihu -- For JIRA 685 Janet wanted a comment on BatteryAdvChange sketch example. On the top of the comment. Please add:

"This sketch is not paired with a specific central example sketch, but to see how it works you need to use a BLE app on your phone or central device and try connecting when it is either a connectable or not connectable state as displayed in the serial monitor."

1. Fix Jira 664 demonstrates changing the ADV data 2. Fix Jira 671 Support update connection interval in central/peripheral

calvinatintel · 2016-09-08T21:19:15Z

Merging in!

eriknyquist reviewed Jul 29, 2016
View reviewed changes

sgbihu force-pushed the master branch 2 times, most recently from a91080e to 91018b5 Compare August 1, 2016 03:02

eriknyquist reviewed Aug 1, 2016
View reviewed changes

sgbihu force-pushed the master branch from 6c9221a to 286f98c Compare August 1, 2016 06:59

eriknyquist reviewed Aug 1, 2016
View reviewed changes

eriknyquist mentioned this pull request Aug 1, 2016

GATT Client API needed #254

Closed

kitsunami added the Under Review label Aug 1, 2016

kitsunami added this to the Castor milestone Aug 1, 2016

kitsunami assigned eriknyquist Aug 1, 2016

sgbihu force-pushed the master branch from 99af2e0 to 74ea011 Compare August 3, 2016 01:18

sgbihu force-pushed the master branch 2 times, most recently from 5495297 to 02ffdf0 Compare August 3, 2016 03:20

eriknyquist reviewed Aug 3, 2016
View reviewed changes

sgbihu force-pushed the master branch from 0e5b211 to 53a757a Compare August 22, 2016 03:18

noelpaz reviewed Aug 22, 2016
View reviewed changes

sgbihu force-pushed the master branch from 75fb944 to bf4739e Compare August 24, 2016 02:12

sgbihu force-pushed the master branch 2 times, most recently from 293a9cd to 2171953 Compare August 31, 2016 01:54

Create BLE Example Sketches

1537c0a

1. Fix Jira 664 demonstrates changing the ADV data 2. Fix Jira 671 Support update connection interval in central/peripheral

sgbihu force-pushed the master branch 2 times, most recently from ac7eda5 to 1537c0a Compare September 8, 2016 08:53

calvinatintel merged commit 1537c0a into arduino:master Sep 8, 2016

rdower unassigned eriknyquist Nov 29, 2017

Uh oh!

BLE central Role implementation and peripheral refactor #255

BLE central Role implementation and peripheral refactor #255

Uh oh!

Conversation

sgbihu commented Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eriknyquist commented Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eriknyquist commented Jul 29, 2016

Uh oh!

eriknyquist Jul 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noelpaz commented Aug 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

calvinatintel commented Aug 18, 2016

Uh oh!

noelpaz commented Aug 19, 2016

Uh oh!

noelpaz commented Aug 19, 2016

Uh oh!

sgbihu commented Aug 22, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

noelpaz commented Aug 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

noelpaz commented Sep 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

calvinatintel commented Sep 8, 2016

Uh oh!

Uh oh!

sgbihu commented Jul 29, 2016 •

edited

Loading

eriknyquist commented Jul 29, 2016 •

edited

Loading

eriknyquist Jul 29, 2016 •

edited

Loading

sgbihu commented Aug 22, 2016 •

edited

Loading

noelpaz commented Sep 8, 2016 •

edited

Loading