Skip to content

Commit

Permalink
nrfconnect: Fixed bug with not handling CHIPoBLE disconnect (project-…
Browse files Browse the repository at this point in the history 
…chip#9160)

Current implementation of Zephyr BLEMgr has a bug that BLEMgr
handles BLE disconnect event only if CHIPoBLE advertising
is enabled to determine whether this disconnect event is related
to CHIPoBLE or some other service. That condition is wrong as
advertising is not working when connection with commissioner
is being closed.

* Changed condition checking if advertising is enabled to check
if CHIPoBLE GATT service is registered (that one persists during
connection).
* Refactored DFUoverSMP module to implement ChipEventHandler
and not force handling advertisments restart in the AppTask.cpp.
* Aligned lock-app and lighting-app examples to those changes.
* Created new platform event kCHIPoBLEConnectionClosed to inform
about closing CHIPoBLE connection and let perform some actions
related to this in the application (e.g. turn on some other
advertising).
  • Loading branch information
@kkasperczyk-no @sharadb-amazon
kkasperczyk-no authored and sharadb-amazon committed Aug 23, 2021
1 parent 82dde52 commit ac9f62a
Show file tree
Hide file tree
Showing 6 changed files with 75 additions and 45 deletions.
24 changes: 8 additions & 16 deletions examples/lighting-app/nrfconnect/main/AppTask.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ int AppTask::Init()
ConfigurationMgr().LogDeviceConfig();
PrintOnboardingCodes(chip::RendezvousInformationFlags(chip::RendezvousInformationFlag::kBLE));

#if defined(CONFIG_CHIP_NFC_COMMISSIONING) || defined(CONFIG_MCUMGR_SMP_BT)
#if defined(CONFIG_CHIP_NFC_COMMISSIONING)
PlatformMgr().AddEventHandler(ChipEventHandler, 0);
#endif

Expand Down Expand Up @@ -392,25 +392,13 @@ void AppTask::StartBLEAdvertisementHandler(AppEvent * aEvent)
}
}

#ifdef CONFIG_CHIP_NFC_COMMISSIONING
void AppTask::ChipEventHandler(const ChipDeviceEvent * event, intptr_t /* arg */)
{
if (event->Type != DeviceEventType::kCHIPoBLEAdvertisingChange)
return;

if (event->CHIPoBLEAdvertisingChange.Result == kActivity_Stopped)
{
#ifdef CONFIG_CHIP_NFC_COMMISSIONING
NFCMgr().StopTagEmulation();
#endif
#ifdef CONFIG_MCUMGR_SMP_BT
// After CHIPoBLE advertising stop, start advertising SMP in case Thread is enabled or there are no active CHIPoBLE
// connections (exclude the case when CHIPoBLE advertising is stopped on the connection time)
if (GetDFUOverSMP().IsEnabled() && (ConnectivityMgr().IsThreadProvisioned() || ConnectivityMgr().NumBLEConnections() == 0))
sAppTask.RequestSMPAdvertisingStart();
#endif
}
#ifdef CONFIG_CHIP_NFC_COMMISSIONING
else if (event->CHIPoBLEAdvertisingChange.Result == kActivity_Started)
if (event->CHIPoBLEAdvertisingChange.Result == kActivity_Started)
{
if (NFCMgr().IsTagEmulationStarted())
{
Expand All @@ -421,8 +409,12 @@ void AppTask::ChipEventHandler(const ChipDeviceEvent * event, intptr_t /* arg */
ShareQRCodeOverNFC(chip::RendezvousInformationFlags(chip::RendezvousInformationFlag::kBLE));
}
}
#endif
else if (event->CHIPoBLEAdvertisingChange.Result == kActivity_Stopped)
{
NFCMgr().StopTagEmulation();
}
}
#endif

void AppTask::CancelTimer()
{
Expand Down
24 changes: 8 additions & 16 deletions examples/lock-app/nrfconnect/main/AppTask.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ int AppTask::Init()
ConfigurationMgr().LogDeviceConfig();
PrintOnboardingCodes(chip::RendezvousInformationFlag(chip::RendezvousInformationFlag::kBLE));

#if defined(CONFIG_CHIP_NFC_COMMISSIONING) || defined(CONFIG_MCUMGR_SMP_BT)
#ifdef CONFIG_CHIP_NFC_COMMISSIONING
PlatformMgr().AddEventHandler(ChipEventHandler, 0);
#endif
return 0;
Expand Down Expand Up @@ -392,25 +392,13 @@ void AppTask::StartBLEAdvertisementHandler(AppEvent * aEvent)
}
}

#ifdef CONFIG_CHIP_NFC_COMMISSIONING
void AppTask::ChipEventHandler(const ChipDeviceEvent * event, intptr_t /* arg */)
{
if (event->Type != DeviceEventType::kCHIPoBLEAdvertisingChange)
return;

if (event->CHIPoBLEAdvertisingChange.Result == kActivity_Stopped)
{
#ifdef CONFIG_CHIP_NFC_COMMISSIONING
NFCMgr().StopTagEmulation();
#endif
#ifdef CONFIG_MCUMGR_SMP_BT
// After CHIPoBLE advertising stop, start advertising SMP in case Thread is enabled or there are no active CHIPoBLE
// connections (exclude the case when CHIPoBLE advertising is stopped on the connection time)
if (GetDFUOverSMP().IsEnabled() && (ConnectivityMgr().IsThreadProvisioned() || ConnectivityMgr().NumBLEConnections() == 0))
sAppTask.RequestSMPAdvertisingStart();
#endif
}
#ifdef CONFIG_CHIP_NFC_COMMISSIONING
else if (event->CHIPoBLEAdvertisingChange.Result == kActivity_Started)
if (event->CHIPoBLEAdvertisingChange.Result == kActivity_Started)
{
if (NFCMgr().IsTagEmulationStarted())
{
Expand All @@ -421,8 +409,12 @@ void AppTask::ChipEventHandler(const ChipDeviceEvent * event, intptr_t /* arg */
ShareQRCodeOverNFC(chip::RendezvousInformationFlags(chip::RendezvousInformationFlag::kBLE));
}
}
#endif
else if (event->CHIPoBLEAdvertisingChange.Result == kActivity_Stopped)
{
NFCMgr().StopTagEmulation();
}
}
#endif

void AppTask::CancelTimer()
{
Expand Down
43 changes: 37 additions & 6 deletions examples/platform/nrfconnect/util/DFUOverSMP.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@

#include <support/logging/CHIPLogging.h>

using namespace ::chip::DeviceLayer;

DFUOverSMP DFUOverSMP::sDFUOverSMP;

void DFUOverSMP::Init(DFUOverSMPRestartAdvertisingHandler startAdvertisingCb)
Expand All @@ -44,6 +46,8 @@ void DFUOverSMP::Init(DFUOverSMPRestartAdvertisingHandler startAdvertisingCb)
bt_conn_cb_register(&mBleConnCallbacks);

restartAdvertisingCallback = startAdvertisingCb;

PlatformMgr().AddEventHandler(ChipEventHandler, 0);
}

void DFUOverSMP::ConfirmNewImage()
Expand Down Expand Up @@ -81,7 +85,7 @@ void DFUOverSMP::StartServer()
ChipLogProgress(DeviceLayer, "Enabled software update");

// Start SMP advertising only in case CHIPoBLE advertising is not working.
if (!chip::DeviceLayer::ConnectivityMgr().IsBLEAdvertisingEnabled())
if (!ConnectivityMgr().IsBLEAdvertisingEnabled())
StartBLEAdvertising();
}
else
Expand All @@ -92,7 +96,7 @@ void DFUOverSMP::StartServer()

void DFUOverSMP::StartBLEAdvertising()
{
if (!mIsEnabled)
if (!mIsEnabled && !mIsAdvertisingEnabled)
return;

const char * deviceName = bt_get_name();
Expand All @@ -119,20 +123,47 @@ void DFUOverSMP::StartBLEAdvertising()
else
{
ChipLogProgress(DeviceLayer, "Started SMP service BLE advertising");
mIsAdvertisingEnabled = true;
}
}

void DFUOverSMP::OnBleDisconnect(struct bt_conn * conId, uint8_t reason)
{
chip::DeviceLayer::PlatformMgr().LockChipStack();
PlatformMgr().LockChipStack();

// After BLE disconnect SMP advertising needs to be restarted. Before making it ensure that BLE disconnect was not triggered
// by closing CHIPoBLE service connection (in that case CHIPoBLE advertising needs to be restarted).
if (!chip::DeviceLayer::ConnectivityMgr().IsBLEAdvertisingEnabled() &&
chip::DeviceLayer::ConnectivityMgr().NumBLEConnections() == 0)
if (!ConnectivityMgr().IsBLEAdvertisingEnabled() && (ConnectivityMgr().NumBLEConnections() == 0))
{
sDFUOverSMP.restartAdvertisingCallback();
}

chip::DeviceLayer::PlatformMgr().UnlockChipStack();
PlatformMgr().UnlockChipStack();
}

void DFUOverSMP::ChipEventHandler(const ChipDeviceEvent * event, intptr_t /* arg */)
{
if (!GetDFUOverSMP().IsEnabled())
return;

switch (event->Type)
{
case DeviceEventType::kCHIPoBLEAdvertisingChange:
if (event->CHIPoBLEAdvertisingChange.Result == kActivity_Stopped)
{
// Check if CHIPoBLE advertising was stopped permanently or it just a matter of opened BLE connection.
if (ConnectivityMgr().NumBLEConnections() == 0)
sDFUOverSMP.restartAdvertisingCallback();
}
break;
case DeviceEventType::kCHIPoBLEConnectionClosed:
// Check if after closing CHIPoBLE connection advertising is working, if no start SMP advertising.
if (!ConnectivityMgr().IsBLEAdvertisingEnabled())
{
sDFUOverSMP.restartAdvertisingCallback();
}
break;
default:
break;
}
}
2 changes: 2 additions & 0 deletions examples/platform/nrfconnect/util/include/DFUOverSMP.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,10 @@ class DFUOverSMP

static int UploadConfirmHandler(uint32_t offset, uint32_t size, void * arg);
static void OnBleDisconnect(bt_conn * conn, uint8_t reason);
static void ChipEventHandler(const chip::DeviceLayer::ChipDeviceEvent * event, intptr_t arg);

bool mIsEnabled;
bool mIsAdvertisingEnabled;
bt_conn_cb mBleConnCallbacks;
DFUOverSMPRestartAdvertisingHandler restartAdvertisingCallback;

Expand Down
7 changes: 7 additions & 0 deletions src/include/platform/CHIPDeviceEvent.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,13 @@ enum PublicEventTypes
*/
kCHIPoBLEConnectionEstablished,

/**
* CHIPoBLE Connection Closed
*
* Signals that an external entity has closed existing CHIPoBLE connection with the device.
*/
kCHIPoBLEConnectionClosed,

/**
* Thread State Change
*
Expand Down
20 changes: 13 additions & 7 deletions src/platform/Zephyr/BLEManagerImpl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -177,9 +177,14 @@ void BLEManagerImpl::DriveBLEState()
SuccessOrExit(err);
}
}
// Otherwise, stop advertising if currently active.
else if (mFlags.Has(Flags::kAdvertising))
else
{
if (mFlags.Has(Flags::kAdvertising))
{
err = StopAdvertising();
SuccessOrExit(err);
}

// If no connections are active unregister also CHIPoBLE GATT service
if (NumConnections() == 0 && mFlags.Has(Flags::kChipoBleGattServiceRegister))
{
Expand All @@ -193,9 +198,6 @@ void BLEManagerImpl::DriveBLEState()
mFlags.Clear(Flags::kChipoBleGattServiceRegister);
}
}

err = StopAdvertising();
SuccessOrExit(err);
}

exit:
Expand Down Expand Up @@ -465,6 +467,10 @@ CHIP_ERROR BLEManagerImpl::HandleGAPDisconnect(const ChipDeviceEvent * event)

ChipLogProgress(DeviceLayer, "Current number of connections: %" PRIu16 "/%" PRIu16, NumConnections(), CONFIG_BT_MAX_CONN);

ChipDeviceEvent disconnectEvent;
disconnectEvent.Type = DeviceEventType::kCHIPoBLEConnectionClosed;
PlatformMgr().PostEvent(&disconnectEvent);

// Force a reconfiguration of advertising in case we switched to non-connectable mode when
// the BLE connection was established.
mFlags.Set(Flags::kAdvertisingRefreshNeeded);
Expand Down Expand Up @@ -798,7 +804,7 @@ void BLEManagerImpl::HandleConnect(struct bt_conn * conId, uint8_t err)
PlatformMgr().LockChipStack();

// Don't handle BLE connecting events when it is not related to CHIPoBLE
VerifyOrExit(ConnectivityMgr().IsBLEAdvertisingEnabled(), );
VerifyOrExit(sInstance.mFlags.Has(Flags::kChipoBleGattServiceRegister), );

event.Type = DeviceEventType::kPlatformZephyrBleConnected;
event.Platform.BleConnEvent.BtConn = bt_conn_ref(conId);
Expand All @@ -817,7 +823,7 @@ void BLEManagerImpl::HandleDisconnect(struct bt_conn * conId, uint8_t reason)
PlatformMgr().LockChipStack();

// Don't handle BLE disconnecting events when it is not related to CHIPoBLE
VerifyOrExit(ConnectivityMgr().IsBLEAdvertisingEnabled(), );
VerifyOrExit(sInstance.mFlags.Has(Flags::kChipoBleGattServiceRegister), );

event.Type = DeviceEventType::kPlatformZephyrBleDisconnected;
event.Platform.BleConnEvent.BtConn = bt_conn_ref(conId);
Expand Down

0 comments on commit ac9f62a

Please sign in to comment.