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

Review the API and add the comments #11

Closed
wants to merge 4 commits into from
Closed

Conversation

@sgbihu
Copy link

@sgbihu sgbihu commented Sep 27, 2016

No description provided.

Copy link
Author

@sgbihu sgbihu left a comment

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

Thanks!
-Lianggao

@@ -24,6 +24,23 @@

#include <BLEDevice.h>

typedef enum

This comment has been minimized.

@sgbihu

sgbihu Sep 27, 2016
Author

Add the error code.

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

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

This comment has been minimized.

@sgbihu

sgbihu Sep 28, 2016
Author

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

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

Can you please clarify what you mean by this?

This comment has been minimized.

@sgbihu

sgbihu Sep 29, 2016
Author

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.

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 29, 2016
Contributor

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

@@ -39,220 +40,439 @@ enum BLEProperty {
BLEIndicate = 0x20
};

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

class BLECharacteristic : public BLEAttributeWithValue

This comment has been minimized.

@sgbihu

sgbihu Sep 27, 2016
Author

Does BLEAttributeWithValue is necessary? Typed characteristic can handle it.

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

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.

This comment has been minimized.

@sgbihu

sgbihu Sep 28, 2016
Author

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.

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

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

This comment has been minimized.

@sgbihu

sgbihu Sep 29, 2016
Author

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.

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 29, 2016
Contributor

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);

This comment has been minimized.

@sgbihu

sgbihu Sep 30, 2016
Author

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?

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 30, 2016
Contributor

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.

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

This comment has been minimized.

@sgbihu

sgbihu Sep 27, 2016
Author

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

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.

This comment has been minimized.

@sgbihu

sgbihu Sep 28, 2016
Author

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

  • 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.

This comment has been minimized.

@sgbihu

sgbihu Sep 29, 2016
Author

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.

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 29, 2016
Contributor

I agree.

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

This comment has been minimized.

@sgbihu

sgbihu Sep 27, 2016
Author

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.

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

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.

This comment has been minimized.

@sgbihu

sgbihu Sep 28, 2016
Author

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.

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

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.

This comment has been minimized.

@sgbihu

sgbihu Sep 29, 2016
Author

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 29, 2016
Contributor

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

This comment has been minimized.

@sgbihu

sgbihu Sep 30, 2016
Author

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.

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 30, 2016
Contributor

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

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

This comment has been minimized.

@sgbihu

sgbihu Sep 27, 2016
Author

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

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

};

#endif

// Characteristic type to represent a bool type

This comment has been minimized.

@sgbihu

sgbihu Sep 27, 2016
Author

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.

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

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.

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

This comment has been minimized.

@sgbihu

sgbihu Sep 27, 2016
Author

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?

This comment has been minimized.

@sgbihu

sgbihu Sep 27, 2016
Author

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

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;
};

This comment has been minimized.

@sgbihu

sgbihu Sep 28, 2016
Author

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?

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

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.

This comment has been minimized.

@sgbihu

sgbihu Sep 29, 2016
Author

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 29, 2016
Contributor

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

This comment has been minimized.

@sgbihu

sgbihu Sep 30, 2016
Author

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.

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 30, 2016
Contributor

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

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

This comment has been minimized.

@sgbihu

sgbihu Sep 27, 2016
Author

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

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

This comment has been minimized.

@sgbihu

sgbihu Sep 28, 2016
Author

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.

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

I've added a compatibility discussion in #12 .

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

Copy link
Contributor

@sandeepmistry sandeepmistry left a comment

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.

@@ -24,6 +24,23 @@

#include <BLEDevice.h>

typedef enum

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

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

@@ -39,220 +40,439 @@ enum BLEProperty {
BLEIndicate = 0x20
};

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

class BLECharacteristic : public BLEAttributeWithValue

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

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.

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

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.

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

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.

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

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

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

What are these for?

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

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

private:

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

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

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

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

This comment has been minimized.

@sgbihu

sgbihu Sep 28, 2016
Author

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

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

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

See comment above.

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 27, 2016
Contributor

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

This comment has been minimized.

@sgbihu

sgbihu Sep 28, 2016
Author

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

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

This comment has been minimized.

@sgbihu

sgbihu Sep 29, 2016
Author

Do you mean we change the parameter type as int?

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 29, 2016
Contributor

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

Copy link
Contributor

@sandeepmistry sandeepmistry left a comment

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

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

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

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

  • 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.
*
* @note Only for GATT client. Schedule read request to the GATT server
*/
virtual bool read();

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

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.

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

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

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

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

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

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.

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

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

I've added a compatibility discussion in #12 .

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

@@ -24,6 +24,23 @@

#include <BLEDevice.h>

typedef enum

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

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

Can you please clarify what you mean by this?

@@ -39,220 +40,439 @@ enum BLEProperty {
BLEIndicate = 0x20
};

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

class BLECharacteristic : public BLEAttributeWithValue

This comment has been minimized.

@sandeepmistry

sandeepmistry Sep 28, 2016
Contributor

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

@sgbihu sgbihu closed this Oct 28, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.