Jira 916 BLE.connected() always return false.

[jimaobian]

Here is the code. It always return false, even BLE is connected.

corelib Version: 2.0.1-RC2

/*
 * Copyright (c) 2016 Intel Corporation.  All rights reserved.
 * See the bottom of this file for the license terms.
 */

#include <CurieBLE.h>

const int ledPin = 13; // set ledPin to on-board LED
const int buttonPin = 4; // set buttonPin to digital pin 4

BLEService ledService("19B10010-E8F2-537E-4F6C-D104768A1214"); // create service

// create switch characteristic and allow remote device to read and write
BLECharCharacteristic ledCharacteristic("19B10011-E8F2-537E-4F6C-D104768A1214", BLERead | BLEWrite);
// create button characteristic and allow remote device to get notifications
BLECharCharacteristic buttonCharacteristic("19B10012-E8F2-537E-4F6C-D104768A1214", BLERead | BLENotify); // allows remote device to get notifications

void setup() {
  Serial.begin(9600);
  pinMode(ledPin, OUTPUT); // use the LED on pin 13 as an output
  pinMode(buttonPin, INPUT); // use button pin 4 as an input

  // begin initialization
  BLE.begin();

  // set the local name peripheral advertises
  BLE.setLocalName("BtnLED");
  // set the UUID for the service this peripheral advertises:
  BLE.setAdvertisedService(ledService);

  // add the characteristics to the service
  ledService.addCharacteristic(ledCharacteristic);
  ledService.addCharacteristic(buttonCharacteristic);

  // add the service
  BLE.addService(ledService);

  ledCharacteristic.setValue(0);
  buttonCharacteristic.setValue(0);

  // start advertising
  BLE.advertise();

  Serial.println("Bluetooth device active, waiting for connections...");
}

void loop() {

//////////////////////////
  static unsigned long BLEWriteTimer = millis();
  if (millis() - BLEWriteTimer >= 1000) {
    BLEWriteTimer = millis();
    Serial.println(BLE.connected());
  }
//////////////////////////

  // poll for BLE events
  BLE.poll();

  // read the current button pin state
  char buttonValue = digitalRead(buttonPin);

  // has the value changed since the last read
  boolean buttonChanged = (buttonCharacteristic.value() != buttonValue);

  if (buttonChanged) {
    // button state changed, update characteristics
    ledCharacteristic.setValue(buttonValue);
    buttonCharacteristic.setValue(buttonValue);
  }

  if (ledCharacteristic.written() || buttonChanged) {
    // update LED, either central has written to characteristic or button state has changed
    if (ledCharacteristic.value()) {
      Serial.println("LED on");
      digitalWrite(ledPin, HIGH);
    } else {
      Serial.println("LED off");
      digitalWrite(ledPin, LOW);
    }
  }
}

/*
  Copyright (c) 2016 Intel Corporation. All rights reserved.

  This library is free software; you can redistribute it and/or
  modify it under the terms of the GNU Lesser General Public
  License as published by the Free Software Foundation; either
  version 2.1 of the License, or (at your option) any later version.

  This library is distributed in the hope that it will be useful,
  but WITHOUT ANY WARRANTY; without even the implied warranty of
  MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
  Lesser General Public License for more details.

  You should have received a copy of the GNU Lesser General Public
  License along with this library; if not, write to the Free Software
  Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-
  1301 USA
*/

[sandeepmistry]

@SidLeung has anyone from the Intel team looked into this?

[sgbihu]

That's expected. Because your code use BLE object. The BLE object is used to manage the local device. So the code always return false.

In your code, you need get the connected device and use this BLEDevice object to call connected method.

  BLEDevice central = BLE.central();

  if (central && central.connected()) {
    // Do what you want
  }

[sandeepmistry]

That's expected. Because your code use BLE object. The BLE object is used to manage the local device. So the code always return false.

@sgbihu I disagree with this expected behaviour.

The original spec we discussed had:

bool connected(); // is the device connected

So I expect BLE.connected() to return true is a central is connected, when the 101 is in peripheral mode.

[sgbihu]

Hi @sandeepmistry ,

I disagree with your opinion too.

1. The BLE can work in central mode and peripheral mode at same time.
2. The connected means the device connected to the BLE (Local device). So the BLE can't connect to itself. Only peer device allow to call this.

So I think your opinion want return true if the BLE has any connected device. Do you think this is necessary?

[sandeepmistry]

The BLE instance of BLEDevice represents the local BLE device, soBLE.connected() should return if the local device is connected to a central.

[sgbihu]

Another scenario is the BLE is work in central mode. Does it return true if peripheral connected to it?

If return true. Does this API can adapt here? If not, why peripheral and central don't have same behavior?

[jimaobian]

@sgbihu Hi, for my view BLE.connected() should be true whichever BLE is in central mode or in peripheral mode as they are actually "connected". Just keep the API simple and easy to understand.

For the ambiguous about the the connection state of central mode and peripheral mode at same time. I think it is a minor requirement. And users can use api like yours to get the different connection state of the two mode:

  BLEDevice central = BLE.central();

  if (central && central.connected()) {
    // Do what you want
  }

  BLEDevice peripheral = BLE. peripheral();

  if (peripheral && peripheral.connected()) {
    // Do what you want
  }

[sandeepmistry]

I agree with @jimaobian's comment above.

[SidLeung]

Folks:

Would like to summarize the discussion in a conclusive manner and to identify tasks to bring fruitful results,

1. First, pardon me for being repetitive, would like to clarify the terminology used. The class BLEDevice can be instantiated as a local device or a remote device. A local device is the default global object, BLE, instantiated in the CurieBLE library. A remote device is a device connected to the board (or the local device). The relationship between local and remote device is 1:n, where, 0 < n < 4.
2. A remote device can further be identified as a Central or a Peripheral.
3. It should be apparent by now that we have devices with very different behaviors being described by a single class, BLEDevice, and, hence, is the source of the issues in our discussion.
4. To confuse the issue more, would like to bring up the fact that the latest BLE stack is capable of being a Central and a Peripheral at the same time. This is not a supported feature yet. Nevertheless, the code design should be prepared for that.

To address the above issues, I would like to kick off a proposal to re-structure the BLEDevice class. Please consider the following class diagram,

This is a realization of various classes based off the parent class, BLEDevice. Using the above class structure, performed a simple exercise of putting several methods (of the BLEDevice class) to the corresponding classes.

It would be more efficient if we can first settle on the re-structuring of the BLEDevice class and that tag some of the methods to each identified class. Please note that this is a kick off of the re-structuring exercise, nothing is set in stone at this point. Please comment.

[SidLeung]

@sandeepmistry

I can see one major obstacle to my proposed changes to the BLEDevice class - the class definition is released to the public. There are sketches using this definition. If some of the methods are moved (re-classified) to the derived classes, these sketches will fail to compile. The proposed hierarchical class structure is a logical description of the class interaction and classifies the behavioral relationship accordingly. The structure validates the each method and implicitly defines its application. The existing flat structuring of the BLEDevice has the initial advantage of easy to get started. However, the baggage of describing the limitations of each method with respect to various condition is quite demanding on the documentation. The chance of confusing a user is much higher especially in the case of using the library without reading the documentation.

[sandeepmistry]

@SidLeung I propose we go with a more simpler change like PR #528 in the short term.

The class diagram you proposed makes sense logically, however I don't think we need a major refactor now. Although, some users might want to call BLE.hasService(....) to see if the local device has the desired service.

The existing flat structuring of the BLEDevice has the initial advantage of easy to get started. However, the baggage of describing the limitations of each method with respect to various condition is quite demanding on the documentation.

We'll find out soon, when the CurieBLE reference on the Arduino site is updated for the new 2.0.2 release. I don't expect too many headaches at this point.

[SidLeung]

@sandeepmistry Thank you for understanding the dilemma I was pondering upon - refactoring a public facing definition or document the operating condition for each method. Apparently, this is an unfortunate choice of the lesser evil. I lean towards the refactoring of the BLEDevice class due to the fact that the instances we ran into as we were developing our comprehensive test scripts for the BLE section. A simple exercise like the following would give a first time user a bit of head scratching,

  BLEDevice central = BLE.central();

  central.scan();    // Can't do that! It's a remote Central device.  But the method is available?  But the
                            // situation invalidates this behavior (method).
  BLE.scan();      // This works.  BLE is the local device.  But it is a Peripheral and it is connected to a 
                        // Central.  The stack supports both roles at the same time!

That's a bit of explaining for one method, scan(). In fact, @sgbihu has grouped all the BLEDevice methods with the following spread sheet,

bledeviceapi.xlsx

I am concerned with the fact that we may have a user experience issue budding here and the time is now to nip it. I would urge you to reconsider the refactoring of the BLEDevice class.

[facchinm]

Hi @SidLeung , I can understand your concerns, but the APIs make a lot of sense in the BLE world, where a device (BLEDevice in this case, in fact "the object") can change role. The remove-vs-local behaviour reflects this duality; of course some functions can't be used on the remote (or makes no sense on the local since you already have that info available as a variable). In the non-embedded world, you would throw an exception when you are calling a function on the "wrong" object, but the way it's handled right now is ok from my POW.

I'd merge https://github.com/01org/corelibs-arduino101/pull/528 since it's an easy solution to a complicated problem, and wait to see the adoption rate of the new APIs and the number of forum requests to decide if we need to enforce some behaviours.

[novashah]

Will make this change in Elnath.

[russmcinnis]

https://jira.ndg.intel.com/browse/ATLEDGE-857 I think this is addressed by PR454 and jira 857 and it was closed. I think we can close this issue @SidLeung

[russmcinnis]

Will re-test when merged for Elnath.

[russmcinnis]

testing other BLE issues. Will retest for Elnath release.