Review the API and add the comments #11

sgbihu · 2016-09-27T08:42:46Z

No description provided.

sgbihu

Hi @sandeepmistry , @SidLeung
Please help review my understanding and help answer my questions in below.

Thanks!
-Lianggao

sgbihu · 2016-09-27T08:48:14Z

api/BLE/ArduinoBLE.h


 #include <BLEDevice.h>

+typedef enum 


Add the error code.

I like this, do you think we should change all the bool type return types to int an error code can be returned?

It's better for user debug the root cause. I think some method like begin are not necessary. Because it is arduino style.

I think some method like begin are not necessary. Because it is arduino style.

Can you please clarify what you mean by this?

The begin method seem return bool. true is success and false is fail. But the error code are like this, 0 is success. 0 is false.

Good point, maybe we move everything to use bools then. Then have another mechanism to get the error code.

sgbihu · 2016-09-27T08:49:26Z

api/BLE/BLECharacteristic.h

-typedef void (*BLECharacteristicEventHandler)(BLECentral& central, BLECharacteristic& characteristic);
+typedef void (*BLECharacteristicEventHandler)(BLEDevice& bledev, BLECharacteristic& characteristic);

 class BLECharacteristic : public BLEAttributeWithValue


Does BLEAttributeWithValue is necessary? Typed characteristic can handle it.

It's needed when in central mode, because you don't necessarily know the size of the remote characteristic or descriptor. Plus the sizes can change.

Line 216 virtual bool writeValue(const byte value[], int length);
Line 359 virtual bool write(const unsigned char* value, int length);
Those API can handle it. And the BLEAttributeWithValue class are defined some typed API. So I think those 2 API can handle all scenario in BLEAttributeWithValue class.

Ok, just to clarify do you agree we need BLEAttributeWithValue?

I think BLEAttributeWithValue is not necessary. We can remove it. It can abstract the UUID and other common functions in BLEService, BLECharacteristic, and BLEDescriptor. But UUID related function will be used in BLEDevice. So I suggest implement some static method under the UUID name space.

I still think we need it to set values with out using pointers.

See: https://github.com/arduino/ArduinoCore-API/blob/master/api/BLE/examples/central/led_control/led_control.ino#L114-L123 for example:

ledCharacteristic.writeByte(0x01);

The LED characteristic is char. How about the structure and RAW format? I think your idea is use typed API if know the type. Use write when doesn't know the type. Am I correct?

In this way the method should declare like
virtual bool writeValue(const byte value[], int length)=0;

And the write is not need to expose to the user. The write request should be sent in GATT client and notification be sent in GATT server when value updated. What's your idea?

The LED characteristic is char. How about the structure and RAW format? I think your idea is use typed API if know the type. Use write when doesn't know the type. Am I correct?

Yes, this is correct.

And the write is not need to expose to the user. The write request should be sent in GATT client and notification be sent in GATT server when value updated. What's your idea?

Maybe you can provide an example sketch of what you have in mind, it's a bit unclear to me.

sgbihu · 2016-09-27T08:50:13Z

api/BLE/BLECharacteristic.h

+     *
+     * @return  bool    true - Written, false - Not changed
+     *
+     * @note  GATT server only. Need comfirm the different with valueUpdated


The difference with valueUpdated? If different, which API will trigger the written and valueUpdated? If no difference, I suggest use value updated.

I was thinking valueUpdated would be for when you do a read or receive a notification from the peripheral in central mode. I think we need to keep written for backwards compatibility.

So it depend on user call which API. Both of them will return true if value updated. Am I correct?

valueUpdated() should return true if the remote device responds to a read request or sends an notification/indication.

written() should return true if the remote device sends a write or write without response.

So you mean the valueUpdated is for the GATT client and writen is for the GATT server. Both of them are stand for the characteristic value has changed. Do you agree with the below conclusion.

GATT server call the valueUpdated always return false and written returns true when value updated and false value not changed.

GATT client call the written always return false and valueUpdated returns true when value updated and false value not changed.

sgbihu · 2016-09-27T08:51:36Z

api/BLE/BLECharacteristic.h

+     *
+     * @note  Only for GATT client. Schedule read request to the GATT server
+     */
+    virtual bool read();


The read response is asynchronous operation. If the character is a local variable. How the callback find it?
Which peer BLE will be read if support multiple devices? I add those constructor for GATT client. So if use reference. This issue still exist.

BLECharacteristic(const char* uuid,
unsigned char properties,
unsigned char valueSize,
BLEDevice *bleDev);
The BLEDevice's get characteristic API need use reference. And let callback to find the same characteristic. So which solution will select? Reference will against the Arduino code style.

We need to make read a synchronous operation. Which ever characteristic the BLEDevice belongs to will be read.

We can have a private or protected constructor for this. This API draft only includes public facing API's.

Most stack are used asynchronous mode. And the blocked time will depend on connection interval and noise. It will block the app to do other event. You also defined value updated method to poll the response. So I don't agree with read is a synchronous operation.

I understand the concern here, but I think it makes the sketches cleaner and is easier to deal with error cases (like the peripheral disconnecting or sending an error response).

@tigoe what you you think about this, in the current designing the following are blocking API's:

peripheral.connect()

peripheral.discoverAttributes()

characteristic.read()

characteristic.write(...)

characteristic.subscribe()

characteristic.unsubscribe()

descriptor.read()

descriptor.write(...)

I think making all of those async. introduces a lot of complexity to the user.

In my understand, I think the event can be processed by event handler or poll. Why read need to select the block method?

How do you propose we get an error code in the polling case? Polling will also make the sketches more messier.

How about add an error event? The event include the operation. This error more focus on the BLE response error. The request error can be handled by return value.

Could you provide some example sketches of how this would work for both polling and callback modes?

sgbihu · 2016-09-27T08:51:55Z

api/BLE/BLECharacteristic.h

+     *
+     * @note  Only for GATT client. Schedule CCCD to the GATT server
+     */
+    bool subscribe();


Seems the client characteristic configuration descriptor(CCCD) will not include in the BLEDescriptor. Or we may need new BLECCCDescriptor class. Am I correct?

Yes, CCCD will be hidden from the user, subscribe/unsubscribe will handle writing the appropriate value to it.

sgbihu · 2016-09-27T08:52:15Z

api/BLE/BLECharacteristic.h


 #endif

-// Characteristic type to represent a bool type


The below are not necessary. See the below code in the example sketch.
BLECharacteristic ledCharacteristic = peripheral.characteristic("19b10001-e8f2-537e-4f6c-d104768a1214");

The ledCharacteristic can't call the BLECharCharacteristic's function. Only use pointer can resolve this issue.

We should avoid using pointers in all user facing API's, it's not Arduino API style.

All remote characteristic will need to be un-typed.

sgbihu · 2016-09-27T08:53:00Z

api/BLE/BLEDevice.h

+     *
+     * @note  Peripheral mode only
+     */
+    BLEDevice central();


Return the BLEDevice need rewrite the operator =. Because the BLEDevice need store the BLE characteristic and BLE descriptor. If we select deep copy. It will lost some information if the returned object add some attributes or remove. If select shallow copy, it need some other variables to implement it. Because the BLEDevice are used for different role, local BLE and peer BLE object, the shallow copy can't use a static variable to indicate the reference.
And the deep copy will need more memory. So the reference is best way. But the style may against Arduino style.
Can you give some suggestion?

The characteristic and service has the same issue. See line 352 and line 359

I would suggest using a proxy design pattern + reference counting. We should avoid using deep copying as this will get out of sync.

class BLECharacteristic { // ... private: BLECharacteristicInternal* _internal; };

But the API returned value is not reference. So you mean the BLEDevice, BLEService and BLECharacteristic class are proxy class. The _internal will point to the real class. The profile be managed in a separate space. So those resource also need to be release. I think it's better to be handled at process disconnect event. I will add "void linkLostProc()" method in BLEDevice class.
What's you opinion?

So you mean the BLEDevice, BLEService and BLECharacteristic class are proxy class. The _internal will point to the real class.

Yes, that is my recommendation.

I will add "void linkLostProc()" method in BLEDevice class. What's you opinion?

It doesn't sound like linkLostProc would be a public facing API.

Yes. The function should be called in disconnected event. So what name you want to use in this API?

Maybe you can share an example sketch to illustrate it's usage.

This is not for sketches. This is for BLE LIB implementation. For the proxy pattern will use BLECharacteristic to manage the BLECharacteristicInternal's memory. But this memory are in the profile tree. It should be managed by profile root service. So the linklost is a protect function. In you opinion, we only discuss the API that expose to user.

Thanks for clarifying. We can discuss implementation details on Basecamp or in the 101 core Github repo.

sgbihu · 2016-09-27T08:53:30Z

api/BLE/BLEDevice.h

+     *
+     * @note  Central mode only. How to distinguish the peripheral?
+     */
+    BLEDevice peripheral();


How to distinguish the peripheral if the central support the multiple devices?

I don't think we need the peripheral() API because of this reason. We should remove it.

So the sketches may change the peripheral BLE object as global if support events.

Another question about do we support the events type in central mode? From current example, seems the process are not support event drive way.

I've added a compatibility discussion in #12 .

Connected and disconnected events will be support for central mode, same applies to characteristic events.

sandeepmistry

Hi @sgbihu,

Thank you for taking the time to review the draft API and providing your feedback! As well as adding more documentation comments to the API.

I've replied to your comments above. Let's continue the discussion.

sandeepmistry · 2016-09-27T13:52:26Z

api/BLE/ArduinoBLE.h


 #include <BLEDevice.h>

+typedef enum 


I like this, do you think we should change all the bool type return types to int an error code can be returned?

sandeepmistry · 2016-09-27T13:54:09Z

api/BLE/BLECharacteristic.h

-typedef void (*BLECharacteristicEventHandler)(BLECentral& central, BLECharacteristic& characteristic);
+typedef void (*BLECharacteristicEventHandler)(BLEDevice& bledev, BLECharacteristic& characteristic);

 class BLECharacteristic : public BLEAttributeWithValue


It's needed when in central mode, because you don't necessarily know the size of the remote characteristic or descriptor. Plus the sizes can change.

sandeepmistry · 2016-09-27T13:56:05Z

api/BLE/BLECharacteristic.h

+     *
+     * @return  bool    true - Written, false - Not changed
+     *
+     * @note  GATT server only. Need comfirm the different with valueUpdated


I was thinking valueUpdated would be for when you do a read or receive a notification from the peripheral in central mode. I think we need to keep written for backwards compatibility.

sandeepmistry · 2016-09-27T13:58:09Z

api/BLE/BLECharacteristic.h

+     *
+     * @note  Only for GATT client. Schedule read request to the GATT server
+     */
+    virtual bool read();


We need to make read a synchronous operation. Which ever characteristic the BLEDevice belongs to will be read.

We can have a private or protected constructor for this. This API draft only includes public facing API's.

sandeepmistry · 2016-09-27T13:58:55Z

api/BLE/BLECharacteristic.h

+     *
+     * @note  Only for GATT client. Schedule CCCD to the GATT server
+     */
+    bool subscribe();


Yes, CCCD will be hidden from the user, subscribe/unsubscribe will handle writing the appropriate value to it.

sandeepmistry · 2016-09-27T14:08:42Z

api/BLE/BLEDevice.h

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


What are these for?

Can we add some arguments or variations to startScanning() instead to filter discovered peripherals by local name or service uuid?

sandeepmistry · 2016-09-27T14:10:17Z

api/BLE/BLEService.h

+     */
+    BLECharacteristic characteristic(const char * uuid, int index) const;
+
+private:


Again, let's keep these headers focused on the public facing API.

sandeepmistry · 2016-09-27T14:11:13Z

api/BLE/BLECharacteristic.h

+    BLECharacteristic(const char* uuid, 
+                      unsigned char properties, 
+                      unsigned char valueSize,
+                      BLEDevice *bleDev);


I think this version should be protected, so let's remove it from the public facing API.

Ok. I think it can help developer to understand your purpose. I was confused first. I will change it as protected.

I understand, but please keep in mind these headers for the public facing API, not implementation details.

sandeepmistry · 2016-09-27T14:11:43Z

api/BLE/BLECharacteristic.h

+                      unsigned char properties, 
+                      const char* value,
+                      BLEDevice *bleDev);
+


See comment above.

sandeepmistry · 2016-09-27T14:13:59Z

api/BLE/BLEDevice.h

+     *
+     * @note  none
+     */
+    void setAdvertisingInterval(float advertisingInterval);


Do we need floats for these? It will add some overhead.

Expose the BLE unit may make user confusion. I think it's better to make it as ms unit.

Ok, but for advertising interval we probably don't need sub-second accuracy, in the current API it's an int.

Do you mean we change the parameter type as int?

Yes, for this one: void setAdvertisingInterval(int advertisingInterval);

sandeepmistry

Hi @sgbihu, I've replied to your new comments.

sandeepmistry · 2016-09-28T16:33:44Z

api/BLE/BLECharacteristic.h

+    BLECharacteristic(const char* uuid, 
+                      unsigned char properties, 
+                      unsigned char valueSize,
+                      BLEDevice *bleDev);


I understand, but please keep in mind these headers for the public facing API, not implementation details.

sandeepmistry · 2016-09-28T16:35:26Z

api/BLE/BLECharacteristic.h

+     *
+     * @return  bool    true - Written, false - Not changed
+     *
+     * @note  GATT server only. Need comfirm the different with valueUpdated


valueUpdated() should return true if the remote device responds to a read request or sends an notification/indication.

written() should return true if the remote device sends a write or write without response.

sandeepmistry · 2016-09-28T16:40:08Z

api/BLE/BLECharacteristic.h

+     *
+     * @note  Only for GATT client. Schedule read request to the GATT server
+     */
+    virtual bool read();


I understand the concern here, but I think it makes the sketches cleaner and is easier to deal with error cases (like the peripheral disconnecting or sending an error response).

@tigoe what you you think about this, in the current designing the following are blocking API's:

peripheral.connect()

peripheral.discoverAttributes()

characteristic.read()

characteristic.write(...)

characteristic.subscribe()

characteristic.unsubscribe()

descriptor.read()

descriptor.write(...)

I think making all of those async. introduces a lot of complexity to the user.

sandeepmistry · 2016-09-28T16:42:31Z

api/BLE/BLEDevice.h

+  BLEDiscovered = 0,        // Discover profile completed
+  BLEConnected = 1,         // BLE device connected
+  BLEDisconnected = 2,      // BLE device disconnected 
+  BLEConParamUpdate = 3,    // Update the connection parameter 


I'm still on the fence about this. In the CoreBluetooth stack they only have the following on the peripheral side: https://developer.apple.com/reference/corebluetooth/cbperipheralmanager/1393277-setdesiredconnectionlatency

sandeepmistry · 2016-09-28T16:43:35Z

api/BLE/BLEDevice.h

+     *
+     * @note  none
+     */
+    void setAdvertisingInterval(float advertisingInterval);


Ok, but for advertising interval we probably don't need sub-second accuracy, in the current API it's an int.

sandeepmistry · 2016-09-28T16:46:28Z

api/BLE/BLEDevice.h

+     *
+     * @note  Peripheral mode only
+     */
+    BLEDevice central();


So you mean the BLEDevice, BLEService and BLECharacteristic class are proxy class. The _internal will point to the real class.

Yes, that is my recommendation.

I will add "void linkLostProc()" method in BLEDevice class. What's you opinion?

It doesn't sound like linkLostProc would be a public facing API.

sandeepmistry · 2016-09-28T16:48:00Z

api/BLE/BLEDevice.h

+     *
+     * @note  Central mode only. How to distinguish the peripheral?
+     */
+    BLEDevice peripheral();


I've added a compatibility discussion in #12 .

Connected and disconnected events will be support for central mode, same applies to characteristic events.

sandeepmistry · 2016-09-28T16:49:19Z

api/BLE/ArduinoBLE.h


 #include <BLEDevice.h>

+typedef enum 


I think some method like begin are not necessary. Because it is arduino style.

Can you please clarify what you mean by this?

sandeepmistry · 2016-09-28T16:49:30Z

api/BLE/BLECharacteristic.h

-typedef void (*BLECharacteristicEventHandler)(BLECentral& central, BLECharacteristic& characteristic);
+typedef void (*BLECharacteristicEventHandler)(BLEDevice& bledev, BLECharacteristic& characteristic);

 class BLECharacteristic : public BLEAttributeWithValue


Ok, just to clarify do you agree we need BLEAttributeWithValue?

sgbihu commented Sep 27, 2016

View reviewed changes

sandeepmistry reviewed Sep 27, 2016

View reviewed changes

sandeepmistry reviewed Sep 28, 2016

View reviewed changes

sgbihu added 4 commits October 25, 2016 15:48

Review the API and add the comments

a5fc5e7

Add link lost process to release the resource

6f66d75

Modify the constructure of BLE devices and other small changes

ac0a562

Update the BLE API after develop on Curie

ac7d589

sgbihu closed this Oct 28, 2016

Uh oh!

Review the API and add the comments #11

Review the API and add the comments #11

Uh oh!

Conversation

sgbihu commented Sep 27, 2016

Uh oh!

sgbihu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sandeepmistry Sep 27, 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!

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!

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!

sandeepmistry Sep 27, 2016 •

edited

Loading