New BLE API preview #337

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.

Sign up for GitHub

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

Jump to bottom

Merged

calvinatintel merged 2 commits into arduino:master from sgbihu:master

Dec 19, 2016

Contributor

sgbihu commented Oct 27, 2016

A preview version for Arduino BLE API.
Just for test, please don't merge now.

Contributor

noelpaz commented Oct 27, 2016

On 1.0.7 with older BLE FW , the advertised address is the same as the one on the sticker
ex: 98:4F:EE:0C:FC:D7
On 2.0.0 with V3 FW, the advertised address is not the same and completely different
ex: D2:18:86:B3:59:25
but if it does a self inquiry using the API call BLE.address
it gets the same address as in the sticker.

Contributor

sandeepmistry commented Oct 27, 2016

I have made the same observation as @noelpaz, on the sticker 98:4F:EE:0C:07:A5 is displayed and BLE.address() returns the same value. The OS X Packet Logger app (from Xcode Hardware IO tools) is displaying an address of CB:0A:84:1D:F7:A7 and random device address. Previously a public device address was used that matched the sticker.

Contributor

SidLeung commented Oct 27, 2016 •

edited

Loading

Jira ticket, 738, created for this issue. The issue did not indicate a manufacturing problem as initially suspected. Thus, OTP area memory content were initialized correctly and was intag.

sandeepmistry reviewed

View reviewed changes

Contributor

sandeepmistry left a comment

I've done a first pass review of the public facing API and made my comments.

We need to discuss the details of how to filter scanning by local name and service UUID, it was not in the original spec. Also, when scanning I cannot seem to access the peripheral local name, an empty string is always returned.

libraries/BLE/examples/central/central.ino

+              #include "BLEAttribute.h"
+              #include "BLECharacteristicImp.h"
+              #include "BLEProfileManager.h"

Contributor

sandeepmistry Oct 27, 2016

Can we replace all of these includes with just #include <CurieBLE.h>?

Contributor Author

sgbihu Oct 28, 2016

Fixed

libraries/BLE/examples/central/central.ino

+              #define LED_PIN   13
+              void setup() {
+                  Serial.begin(115200);

Contributor

sandeepmistry Oct 27, 2016

All examples should use a baud rate of 9600 for Serial, as it's the default baud rate of the IDE's Serial Monitor.

Contributor Author

sgbihu Oct 28, 2016

Fixed.

libraries/BLE/examples/central/central.ino

+                  Serial.println("test---");
+                  // set LED pin to output mode
+                  pinMode(LED_PIN, OUTPUT);

Contributor

sandeepmistry Oct 27, 2016

This example does not need to control the on board LED, we can remove this.

libraries/BLE/examples/central/central.ino

+                  BLE.begin();
+                  Serial.println(BLE.address());
+                  BLE.startScanning("LED");

Contributor

sandeepmistry Oct 27, 2016

Filtering scanning by name (or service UUID) is not something we discussed, we need to have a broader discussion on this.

Also, you should be allowed to scan without any filters.

Contributor Author

sgbihu Oct 28, 2016

If the environment is noisy, the available api is slow, need more buffer in the stack. I think we need consider those questions

The RAM
Does device need buffer the ADV's data?
How to let user process the RAW ADV data?
Filter ADV

Contributor

sandeepmistry Oct 28, 2016

I've started another discussion on this outside this PR.

We can drop discovered peripherals if the user does not call available() fast enough.

We should not expose the raw advertisement and scan response to the user. There are some existing API's in BLE devices for them to access the parsed data.

Contributor Author

sgbihu Nov 8, 2016

Done

libraries/BLE/examples/central/central.ino

+                  BLE.startScanning("LED");
+              }
+              void controlLed(BLEDevice &peripheral)

Contributor

sandeepmistry Oct 27, 2016

We should aim to remove all passing by reference from the examples.

Contributor Author

sgbihu Oct 28, 2016

Ok

libraries/BLE/src/BLEDevice.h

+                  void setAcceptAdvertiseLocalName(String name);
+                  void setAcceptAdvertiseLocalName(BLEService& service);
+                  void setAcceptAdvertiseCallback(String name);

Contributor

sandeepmistry Oct 27, 2016

The setAcceptAdvertise* API's should not be public.

libraries/BLE/src/BLEDevice.h

+                  bool operator!=(const BLEDevice& device) const;
+                  //BLEDevice& operator=(const BLEDevice& device);
+                  // central mode
+                  void startScanning(String name); // start scanning for peripherals

Contributor

sandeepmistry Oct 27, 2016

We need to support scanning without filters and have a broader discussion for how to do filtering for both local name and service uuid's.

Contributor

noelpaz Oct 28, 2016

I had the same question -- or maybe support "*" wildcard

libraries/BLE/src/BLEDeviceManager.h

+              /*
+                BLE Device API
+                Copyright (c) 2016 Arduino LLC. All right reserved.

Contributor

sandeepmistry Oct 27, 2016

Is this an internal class? If so, can we move it to src/internal/BLEDeviceManager.h?

Contributor

SidLeung Nov 1, 2016

Will move implementation related class definitions/files to the internal subdirectory.

Contributor Author

sgbihu Nov 8, 2016

Done

libraries/BLE/src/BLEProfileManager.cpp

@@ @@ -0,0 +1,894 @@ @@
+              /*
+               * Copyright (c) 2016 Intel Corporation.  All rights reserved.
+               *

Contributor

sandeepmistry Oct 27, 2016

Similar to above, is this an internal class?

Contributor Author

sgbihu Nov 8, 2016

Done

libraries/BLE/src/BLETypedCharacteristic.h

+                   *
+                   * @note  none
+                   */
+                  bool setValue(T value);

Contributor

sandeepmistry Oct 27, 2016

We discussed a new writeValue operations, I don't see them here. setValue will be deprecated.

Contributor Author

sgbihu Oct 28, 2016

Yes. This PRU already include deprecated code.

Contributor

sandeepmistry Oct 28, 2016

Well you have setValue but I doubt see the new writeValue in here though.

Contributor

SidLeung Nov 1, 2016

This is needed for baseline.

Contributor Author

sgbihu Nov 8, 2016

Done

eriknyquist suggested changes

View reviewed changes

Contributor

eriknyquist left a comment •

edited

Loading

Need to be more careful when using the heap. Remember that using new also allocates on the heap, and pointers to objects allocated with new also need to be checked for NULL-ness.

Additionally, I noticed that you are using balloc() instead of malloc(). This function calls cfw_alloc() which simply calls malloc() but disables interrupts first.
(See here https://github.com/01org/corelibs-arduino101/blob/master/system/libarc32_arduino101/framework/src/os/os.c#L43)
Is this really necessary? Is there some reason you cannot use malloc()?

libraries/BLE/src/BLEProfileManager.cpp

+                  if (NULL == _attr_base)
+                  {
+                      _attr_base = (bt_gatt_attr_t *)balloc(attr_counter * sizeof(bt_gatt_attr_t), NULL);
+                      memset(_attr_base, 0x00, (attr_counter * sizeof(bt_gatt_attr_t)));

Contributor

eriknyquist Oct 28, 2016

Check for NULL-ness before memset

libraries/BLE/src/BLECallbacks.cpp

+                  BLEDevice bleDevice(bt_conn_get_dst(conn));
+                  // Get characteristic by handle params->single.handle
+                  chrc = BLEProfileManager::instance()->characteristic(bleDevice, params->single.handle);

Contributor

eriknyquist Oct 28, 2016

Remember, every time you call BLEProfileManager::instance(), it creates a new BLEProfileManager on the (very small) heap. Running out of heap space is very possible with < 8k available. Always check for NULL-ness.

Contributor Author

sgbihu Nov 8, 2016

Used Singleton pattern, it only be created 1 time. Will add assert to avoid NULL check in instance method. If the device can't allocate the memory, the APP can't execute correct.

libraries/BLE/src/BLECharacteristicImp.cpp

+                  _value = (unsigned char*)balloc(_value_size, NULL);
+                  if (_value_size > BLE_MAX_ATTR_DATA_LEN)
+                  {
+                      _value_buffer = (unsigned char*)balloc(_value_size, NULL);

Contributor

eriknyquist Oct 28, 2016

Check _value and _value_buffer for NULL-ness. Heap is very small, it might well be NULL.

kitsunami added this to the Deneb milestone

kitsunami added the enhancement label

kitsunami assigned eriknyquist

eriknyquist suggested changes

View reviewed changes

Contributor

eriknyquist left a comment

Still a lot of unchecked heap allocations happening.

libraries/BLE/src/BLECharacteristicImp.cpp

+                  if (NULL != characteristic._value)
+                  {
+                      memcpy(_value, characteristic._value, _value_size);

Contributor

eriknyquist Oct 31, 2016

_value is a pointer returned from malloc. You need to check for NULL-ness before using it.

libraries/BLE/src/BLECharacteristicImp.cpp

+                      return;
+                  }
+                  memcpy(_value_buffer + offset, value, length);

Contributor

eriknyquist Oct 31, 2016

Check _value_buffer for NULL-ness before using.

libraries/BLE/src/BLEDescriptor.cpp

+                  _value_size = descriptorImp->valueSize();
+                  _value = (unsigned char*)balloc(_value_size, NULL);
+                  memcpy(_value, descriptorImp->value(), _value_size);

Contributor

eriknyquist Oct 31, 2016

You can't assume balloc/malloc will return a valid pointer. ALways check it for NULL-ness.

libraries/BLE/src/BLEDescriptor.cpp

+                  _bledev.setAddress(*BLEUtils::bleGetLoalAddress());
+                  _value_size = valueLength > BLE_MAX_ATTR_LONGDATA_LEN ? BLE_MAX_ATTR_LONGDATA_LEN : valueLength;
+                  _value = (unsigned char*)balloc(_value_size, NULL);

Contributor

eriknyquist Oct 31, 2016

Same thing as line 44. Have to check _value for NULL-ness

libraries/BLE/src/BLEDescriptorImp.cpp

+                  _value_length = descriptor.valueLength();
+                  _value = (unsigned char*)balloc(_value_length, NULL);
+                  memcpy(_value, descriptor.value(), _value_length);

Contributor

eriknyquist Oct 31, 2016

You must check _value for NULL-ness before you try to copy something to it.

libraries/BLE/src/BLEDescriptorImp.cpp

+                  _value_length = BLE_MAX_ATTR_DATA_LEN;
+                  _value = (unsigned char*)balloc(_value_length, NULL);
+                  memset(_value, 0, _value_length);

Contributor

eriknyquist Oct 31, 2016

Pointer _value was returned by balloc. Check for NULL-ness first.

kitsunami mentioned this pull request

BLE Central review (Arduino style) #330

Closed

SidLeung reviewed

View reviewed changes

platform.txt

@@ @@ -14,12 +14,12 @@ compiler.prefix=arc-elf32 @@
               compiler.path={runtime.tools.arc-elf32.path}/bin/
               compiler.c.cmd=arc-elf32-gcc
-              compiler.c.flags=-c -std=gnu11 -mcpu=quarkse_em -mlittle-endian -g -Os -Wall -fno-reorder-functions -fno-asynchronous-unwind-tables -fno-omit-frame-pointer -fno-defer-pop -Wno-unused-but-set-variable -Wno-main -ffreestanding -fno-stack-protector -mno-sdata -ffunction-sections -fdata-sections -fsigned-char -MMD -D__ARDUINO_ARC__ -DCONFIG_BLUETOOTH_PERIPHERAL -DCONFIG_BLUETOOTH_CENTRAL -DCONFIG_BLUETOOTH_GATT_CLIENT
+              compiler.c.flags=-c -std=gnu11 -mcpu=quarkse_em -mlittle-endian -g -O0 -Wall -fno-reorder-functions -fno-asynchronous-unwind-tables -fno-omit-frame-pointer -fno-defer-pop -Wno-unused-but-set-variable -Wno-main -ffreestanding -fno-stack-protector -mno-sdata -ffunction-sections -fdata-sections -fsigned-char -MMD -D__ARDUINO_ARC__ -DCONFIG_BLUETOOTH_PERIPHERAL -DCONFIG_BLUETOOTH_CENTRAL -DCONFIG_BLUETOOTH_GATT_CLIENT

Contributor

SidLeung Nov 8, 2016

This is not a fix, a temporary patch.

sgbihu force-pushed the master branch 2 times, most recently from 443602f to c46f904 Compare

November 16, 2016 05:47

SidLeung force-pushed the master branch from 599230b to 19baf30 Compare

November 16, 2016 22:07

sgbihu force-pushed the master branch from 9402041 to 63674c3 Compare

November 18, 2016 04:43

sgbihu force-pushed the master branch 3 times, most recently from d6a4d36 to 7ce5e79 Compare

December 1, 2016 01:02

sandeepmistry mentioned this pull request

Add cast for compatibility with the next version of CurieBLE firmata/arduino#335

Merged

sgbihu force-pushed the master branch 5 times, most recently from 1e1ed49 to 4c89a18 Compare

December 19, 2016 06:59

sgbihu added 2 commits

December 19, 2016 15:00


          Arduino BLE new library

ade23f0

1. Add new library

2. Add BLEPeripheral library back compatible features

3. Fix IMU build issue

4. Fix the memset crash issue
  i.  The FIRQ doesn't save the LP_COUNTER register.
  ii. Update the code to save LP related register

5. Fix the central crash issue when peripheral disconnect

6. Revert the UART changes
   -This change will break the BLE's communication

7. Implement the balloc to replace old one
   -Create memory pool
   -Port V3's balloc
   -Fix the local name display unexpected chars

8. Fix ScanCallback, LED related issue
   -The Serial.print called in interrupt and interrupt block the Serial interrupt.
   -The resource not released when call disconnect API.

9. Fix the discover blocked when connect/disconnect the link successively

Block the disconnect if not receive the disconnect event

10. Return error when discover attributes if error happened

11. Update the back compatible comments

12. Fix the discover attribute failed issue
    -Add the -fcheck-new build option
    -Increase the IRQ stack size

13. Add discoverAttributesByService API
    - The sensorTag has many services and RAM is not enough for discover all services
    -This crash caused by bt_uuid_16 and bt_uuid_128 has different space and makes an address exception

14. Add keywords and fix read issue

15. Update the CurieBLE and delete BLE


          Add test sketches

4c89a18

eriknyquist approved these changes

View reviewed changes

calvinatintel merged commit 4c89a18 into arduino:master

rdower unassigned eriknyquist

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

Labels