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

ESP32 - Update advertising data to be closer to the spec #1911

Merged
merged 1 commit into from
Jul 29, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion config/esp32/components/chip/Kconfig
Original file line number Diff line number Diff line change
Expand Up @@ -511,7 +511,7 @@ menu "CHIP Device Layer"

config BLE_FAST_ADVERTISING_INTERVAL
int "Fast Advertising Interval"
default 800
default 320
depends on ENABLE_CHIPOBLE
help
The interval (in units of 0.625ms) at which the device will send BLE advertisements while
Expand Down
15 changes: 9 additions & 6 deletions examples/chip-tool/main.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -192,13 +192,14 @@ struct CommandArgs
{
IPAddress hostAddr;
uint16_t port;
char * name;
uint16_t discriminator;
uint8_t endpointId;
};

bool DetermineArgsBle(char * argv[], CommandArgs * commandArgs)
{
commandArgs->name = argv[2];
std::string discriminator_str(argv[2]);
commandArgs->discriminator = std::stoi(discriminator_str);
return true;
}

Expand Down Expand Up @@ -289,9 +290,11 @@ void DoEcho(DeviceController::ChipDeviceController * controller, const char * id
}
}

void DoEchoBle(DeviceController::ChipDeviceController * controller, const std::string & name)
void DoEchoBle(DeviceController::ChipDeviceController * controller, const uint16_t discriminator)
{
DoEcho(controller, name.c_str());
char name[4];
snprintf(name, sizeof(name), "%u", discriminator);
DoEcho(controller, "");
}

void DoEchoIP(DeviceController::ChipDeviceController * controller, const IPAddress & hostAddr, uint16_t port)
Expand Down Expand Up @@ -350,9 +353,9 @@ CHIP_ERROR ExecuteCommand(DeviceController::ChipDeviceController * controller, C
switch (command)
{
case Command::EchoBle:
err = controller->ConnectDevice(kRemoteDeviceId, commandArgs.name, NULL, OnConnect, OnMessage, OnError);
err = controller->ConnectDevice(kRemoteDeviceId, commandArgs.discriminator, NULL, OnConnect, OnMessage, OnError);
VerifyOrExit(err == CHIP_NO_ERROR, fprintf(stderr, "Failed to connect to the device"));
DoEchoBle(controller, commandArgs.name);
DoEchoBle(controller, commandArgs.discriminator);
break;

case Command::Echo:
Expand Down
4 changes: 2 additions & 2 deletions src/ble/BleConnectionDelegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,8 +50,8 @@ class DLL_EXPORT BleConnectionDelegate
OnConnectionErrorFunct OnConnectionError;

// Call this function to delegate the connection steps required to get a BLE_CONNECTION_OBJECT
// out of a peripheral name.
virtual void NewConnection(BleLayer * bleLayer, void * appState, const char * connName) = 0;
// out of a peripheral discriminator.
virtual void NewConnection(BleLayer * bleLayer, void * appState, const uint16_t connDiscriminator) = 0;
};

} /* namespace Ble */
Expand Down
6 changes: 3 additions & 3 deletions src/ble/BleLayer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -369,19 +369,19 @@ BLE_ERROR BleLayer::Shutdown()
return BLE_NO_ERROR;
}

BLE_ERROR BleLayer::NewBleConnection(void * appState, const char * connName,
BLE_ERROR BleLayer::NewBleConnection(void * appState, const uint16_t connDiscriminator,
BleConnectionDelegate::OnConnectionCompleteFunct onConnectionComplete,
BleConnectionDelegate::OnConnectionErrorFunct onConnectionError)
{
BLE_ERROR err = BLE_NO_ERROR;

VerifyOrExit(mState == kState_Initialized, err = BLE_ERROR_INCORRECT_STATE);
VerifyOrExit(connName != nullptr, err = BLE_ERROR_BAD_ARGS);
VerifyOrExit(connDiscriminator != 0, err = BLE_ERROR_BAD_ARGS);
VerifyOrExit(mConnectionDelegate != nullptr, err = BLE_ERROR_INCORRECT_STATE);

mConnectionDelegate->OnConnectionComplete = onConnectionComplete;
mConnectionDelegate->OnConnectionError = onConnectionError;
mConnectionDelegate->NewConnection(this, appState, connName);
mConnectionDelegate->NewConnection(this, appState, connDiscriminator);

exit:
return err;
Expand Down
2 changes: 1 addition & 1 deletion src/ble/BleLayer.h
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ class DLL_EXPORT BleLayer
BleApplicationDelegate * appDelegate, chip::System::Layer * systemLayer);
BLE_ERROR Shutdown(void);

BLE_ERROR NewBleConnection(void * appState, const char * connName,
BLE_ERROR NewBleConnection(void * appState, const uint16_t connDiscriminator,
BleConnectionDelegate::OnConnectionCompleteFunct onConnectionComplete,
BleConnectionDelegate::OnConnectionErrorFunct onConnectionError);
BLE_ERROR NewBleEndPoint(BLEEndPoint ** retEndPoint, BLE_CONNECTION_OBJECT connObj, BleRole role, bool autoClose);
Expand Down
6 changes: 3 additions & 3 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ CHIP_ERROR ChipDeviceController::Shutdown()
return err;
}

CHIP_ERROR ChipDeviceController::ConnectDevice(NodeId remoteDeviceId, const char * deviceName, void * appReqState,
CHIP_ERROR ChipDeviceController::ConnectDevice(NodeId remoteDeviceId, const uint16_t deviceDiscriminator, void * appReqState,
NewConnectionHandler onConnected, MessageReceiveHandler onMessageReceived,
ErrorHandler onError)
{
Expand All @@ -161,8 +161,8 @@ CHIP_ERROR ChipDeviceController::ConnectDevice(NodeId remoteDeviceId, const char
mOnNewConnection = onConnected;

transport = new Transport::BLE();
err = transport->Init(
Transport::BleConnectionParameters(this, DeviceLayer::ConnectivityMgr().GetBleLayer()).SetConnectionName(deviceName));
err = transport->Init(Transport::BleConnectionParameters(this, DeviceLayer::ConnectivityMgr().GetBleLayer())
.SetConnectionDiscriminator(deviceDiscriminator));
SuccessOrExit(err);
mUnsecuredTransport = transport->Retain();

Expand Down
6 changes: 3 additions & 3 deletions src/controller/CHIPDeviceController.h
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,15 @@ class DLL_EXPORT ChipDeviceController : public SecureSessionMgrCallback
* Connect to a CHIP device with a given name for Rendezvous
*
* @param[in] remoteDeviceId The remote device Id.
* @param[in] deviceName The name of the requested Device
* @param[in] deviceDiscriminator The discriminator of the requested Device
* @param[in] appReqState Application specific context to be passed back when a message is received or on error
* @param[in] onConnected Callback for when the connection is established
* @param[in] onMessageReceived Callback for when a message is received
* @param[in] onError Callback for when an error occurs
* @return CHIP_ERROR The connection status
*/
CHIP_ERROR ConnectDevice(NodeId remoteDeviceId, const char * deviceName, void * appReqState, NewConnectionHandler onConnected,
MessageReceiveHandler onMessageReceived, ErrorHandler onError);
CHIP_ERROR ConnectDevice(NodeId remoteDeviceId, const uint16_t deviceDiscriminator, void * appReqState,
NewConnectionHandler onConnected, MessageReceiveHandler onMessageReceived, ErrorHandler onError);

/**
* @brief
Expand Down
8 changes: 4 additions & 4 deletions src/darwin/CHIPTool/CHIPTool/QRCode/QRCodeViewController.m
Original file line number Diff line number Diff line change
Expand Up @@ -340,22 +340,22 @@ - (void)handleRendezVous:(CHIPSetupPayload *)payload
break;
case kRendezvousInformationBLE:
NSLog(@"Rendezvous BLE");
[self handleRendezVousBLE:[self getNetworkName:payload.discriminator]];
[self handleRendezVousBLE:payload.discriminator.unsignedShortValue];
break;
}
}

- (NSString *)getNetworkName:(NSNumber *)discriminator
{
NSString * peripheralDiscriminator = [NSString stringWithFormat:@"%hX", discriminator.unsignedShortValue];
NSString * peripheralDiscriminator = [NSString stringWithFormat:@"%04u", discriminator.unsignedShortValue];
NSString * peripheralFullName = [NSString stringWithFormat:@"%@%@", NETWORK_CHIP_PREFIX, peripheralDiscriminator];
return peripheralFullName;
}

- (void)handleRendezVousBLE:(NSString *)name
- (void)handleRendezVousBLE:(uint16_t)discriminator
{
NSError * error;
[self.chipController connect:name error:&error];
[self.chipController connect:discriminator error:&error];
}

- (void)handleRendezVousWiFi:(NSString *)name
Expand Down
2 changes: 1 addition & 1 deletion src/darwin/Framework/CHIP/CHIPDeviceController.h
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ typedef void (^ControllerOnErrorBlock)(NSError * error);
local_key:(NSData *)local_key
peer_key:(NSData *)peer_key
error:(NSError * __autoreleasing *)error;
- (BOOL)connect:(NSString *)deviceName error:(NSError * __autoreleasing *)error;
- (BOOL)connect:(uint16_t)deviceDiscriminator error:(NSError * __autoreleasing *)error;
- (nullable AddressInfo *)getAddressInfo;
- (BOOL)sendMessage:(NSData *)message error:(NSError * __autoreleasing *)error;
- (BOOL)sendOnCommand;
Expand Down
4 changes: 2 additions & 2 deletions src/darwin/Framework/CHIP/CHIPDeviceController.mm
Original file line number Diff line number Diff line change
Expand Up @@ -229,13 +229,13 @@ - (BOOL)connect:(NSString *)ipAddress
return YES;
}

- (BOOL)connect:(NSString *)deviceName error:(NSError * __autoreleasing *)error
- (BOOL)connect:(uint16_t)deviceDiscriminator error:(NSError * __autoreleasing *)error
{
CHIP_ERROR err = CHIP_NO_ERROR;

[self.lock lock];
err = self.cppController->ConnectDevice(
kRemoteDeviceId, [deviceName UTF8String], (__bridge void *) self, onConnected, onMessageReceived, onInternalError);
kRemoteDeviceId, deviceDiscriminator, (__bridge void *) self, onConnected, onMessageReceived, onInternalError);
[self.lock unlock];

if (err != CHIP_NO_ERROR) {
Expand Down
2 changes: 1 addition & 1 deletion src/platform/Darwin/BleConnectionDelegate.h
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ namespace Internal {
class BleConnectionDelegateImpl : public BleConnectionDelegate
{
public:
virtual void NewConnection(Ble::BleLayer * bleLayer, void * appState, const char * connName);
virtual void NewConnection(Ble::BleLayer * bleLayer, void * appState, const uint16_t connDiscriminator);
};

} // namespace Internal
Expand Down
63 changes: 51 additions & 12 deletions src/platform/Darwin/BleConnectionDelegateImpl.mm
Original file line number Diff line number Diff line change
Expand Up @@ -34,14 +34,14 @@ @interface BleConnection : NSObject <CBCentralManagerDelegate, CBPeripheralDeleg

@property (strong, nonatomic) dispatch_queue_t workQueue;
@property (strong, nonatomic) CBCentralManager * centralManager;
@property (strong, nonatomic) NSString * deviceName;
@property (strong, nonatomic) CBPeripheral * peripheral;
@property (unsafe_unretained, nonatomic) uint16_t deviceDiscriminator;
@property (unsafe_unretained, nonatomic) void * appState;
@property (unsafe_unretained, nonatomic) BleConnectionDelegate::OnConnectionCompleteFunct onConnectionComplete;
@property (unsafe_unretained, nonatomic) BleConnectionDelegate::OnConnectionErrorFunct onConnectionError;
@property (unsafe_unretained, nonatomic) chip::Ble::BleLayer * mBleLayer;

- (id)initWithName:(NSString *)deviceName;
- (id)initWithDiscriminator:(uint16_t)deviceDiscriminator;
- (void)setBleLayer:(chip::Ble::BleLayer *)bleLayer;
- (void)start;
- (void)stop;
Expand All @@ -51,10 +51,10 @@ - (void)stop;
namespace chip {
namespace DeviceLayer {
namespace Internal {
void BleConnectionDelegateImpl::NewConnection(Ble::BleLayer * bleLayer, void * appState, const char * connName)
void BleConnectionDelegateImpl::NewConnection(Ble::BleLayer * bleLayer, void * appState, const uint16_t deviceDiscriminator)
{
ChipLogProgress(Ble, "%s", __FUNCTION__);
BleConnection * ble = [[BleConnection alloc] initWithName:@(connName)];
BleConnection * ble = [[BleConnection alloc] initWithDiscriminator:deviceDiscriminator];
[ble setBleLayer:bleLayer];
ble.appState = appState;
ble.onConnectionComplete = OnConnectionComplete;
Expand All @@ -69,11 +69,11 @@ @interface BleConnection ()

@implementation BleConnection

- (id)initWithName:(NSString *)deviceName
- (id)initWithDiscriminator:(uint16_t)deviceDiscriminator
{
self = [super init];
if (self) {
_deviceName = deviceName;
_deviceDiscriminator = deviceDiscriminator;
_workQueue = dispatch_queue_create("com.chip.ble.work_queue", DISPATCH_QUEUE_SERIAL);
_centralManager = [CBCentralManager alloc];
[_centralManager initWithDelegate:self queue:_workQueue];
Expand Down Expand Up @@ -115,12 +115,30 @@ - (void)centralManager:(CBCentralManager *)central
advertisementData:(NSDictionary *)advertisementData
RSSI:(NSNumber *)RSSI
{
BOOL isConnectable = !![advertisementData objectForKey:CBAdvertisementDataIsConnectable];
NSString * localNameKey = [advertisementData objectForKey:CBAdvertisementDataLocalNameKey];
if (isConnectable && [localNameKey isEqual:_deviceName]) {
ChipLogProgress(Ble, "Connecting to device: %@", _deviceName);
[self connect:peripheral];
[self stopScanning];
NSNumber * isConnectable = [advertisementData objectForKey:CBAdvertisementDataIsConnectable];
if ([isConnectable boolValue]) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

supernit: how do people feel about early return and continues to decrease code indents? I find it easier to understand code if it is less indented once it starts growing, however I also see a pattern in other chip places of 'only one exit point' (but does that apply to non-resource constrained code?)

like:

if (![isConnectable boolValue]) {
  return;  // only connectable devices are useful
}

CBUUID * .....
for (....) {
  if (!... isEqualToData...) { 
     continue;
  }

  if (....) { 
  }
}

vs.

if ([isConnectable boolValue]) {
  CBUUID * .....;

  for (....) {
     if (...) { 
       if (...) {
       }
    }
  }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my part I'm 100% in favour or early returns and to declared variables just before they are used, if possible.
I think @gerickson advocates for the 'single exit point' patterns thought.

CBUUID * shortChipServiceUUID = [BleConnection getShortestServiceUUID:&chip::Ble::CHIP_BLE_SVC_ID];

NSDictionary * servicesData = [advertisementData objectForKey:CBAdvertisementDataServiceDataKey];
for (CBUUID * serviceUUID in servicesData) {
if ([serviceUUID.data isEqualToData:shortChipServiceUUID.data]) {
NSData * serviceData = [servicesData objectForKey:serviceUUID];

int length = [serviceData length];
if (length == 3) {
const uint8_t * bytes = (const uint8_t *) [serviceData bytes];
uint8_t opCode = bytes[0];
uint16_t discriminator = ((bytes[1] & 0x0F) << 8) | bytes[2];
if (opCode == 0 && discriminator == _deviceDiscriminator) {
ChipLogProgress(Ble, "Connecting to device: %@", peripheral);
[self connect:peripheral];
[self stopScanning];
}
}

break;
}
}
}
}

Expand Down Expand Up @@ -343,6 +361,27 @@ + (void)fillServiceWithCharacteristicUuids:(CBCharacteristic *)characteristic
memcpy(svcId->bytes, serviceFullUUID, sizeof(svcId->bytes));
}

+ (CBUUID *)getShortestServiceUUID:(const chip::Ble::ChipBleUUID *)svcId
{
// shorten the 16-byte UUID reported by BLE Layer to shortest possible, 2 or 4 bytes
// this is the BLE Service UUID Base. If a 16-byte service UUID partially matches with this 12 bytes,
// it could be shorten to 2 or 4 bytes.
static const uint8_t bleBaseUUID[12] = { 0x00, 0x00, 0x10, 0x00, 0x80, 0x00, 0x00, 0x80, 0x5F, 0x9B, 0x34, 0xFB };
if (0 == memcmp(svcId->bytes + 4, bleBaseUUID, sizeof(bleBaseUUID))) {
// okay, let's try to shorten it
if ((0 == svcId->bytes[0]) && (0 == svcId->bytes[1])) {
// the highest 2 bytes are both 0, so we just need 2 bytes
return [CBUUID UUIDWithData:[NSData dataWithBytes:(svcId->bytes + 2) length:2]];
} else {
// we need to use 4 bytes
return [CBUUID UUIDWithData:[NSData dataWithBytes:svcId->bytes length:4]];
}
} else {
// it cannot be shorten as it doesn't match with the BLE Service UUID Base
return [CBUUID UUIDWithData:[NSData dataWithBytes:svcId->bytes length:16]];
}
}

- (void)setBleLayer:(chip::Ble::BleLayer *)bleLayer
{
_mBleLayer = bleLayer;
Expand Down
Loading