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

Revert "Cancel incomplete BLE connection when CloseAllBleConnections() is called (#27304)" #28143

Merged

Conversation

bzbarsky-apple
Copy link
Contributor

This reverts commit ad5403e.

There are two issues:

We need to sort out why CHIPDeviceController is canceling BLE connections when starting PASE over BLE (!).

Fixes #28139

…) is called (project-chip#27304)"

This reverts commit ad5403e.

There are two issues:

* project-chip#27607 which has had no
  response for weeks.
* project-chip#28139 which breaks
  commissioning on Mac, and would break it on Linux if it implemented BLE
  connection cancellation.

We need to sort out why CHIPDeviceController is canceling BLE connections when
starting PASE over BLE (!).
@woody-apple
Copy link
Contributor

@bzbarsky-apple The change makes logical sense though, to cancel any connections being established though? Why is this causing issues?

@woody-apple woody-apple removed hotfix urgent fix needed, can bypass review fast track labels Jul 21, 2023
@bzbarsky-apple
Copy link
Contributor Author

@woody-apple Because of this stack:

  * frame #0: 0x0000000101bf916c chip-tool`chip::Ble::BleLayer::CancelBleIncompleteConnection(this=0x0000000102beb5a0) at BleLayer.cpp:372:5
    frame #1: 0x0000000101bf8cd4 chip-tool`chip::Ble::BleLayer::CloseAllBleConnections(this=0x0000000102beb5a0) at BleLayer.cpp:312:22
    frame #2: 0x0000000102260b80 chip-tool`chip::Controller::DeviceCommissioner::ReleaseCommissioneeDevice(this=0x0000000108d11900, device=0x0000000107915e80) at CHIPDeviceController.cpp:560:35
    frame #3: 0x0000000102265d64 chip-tool`chip::Controller::DeviceCommissioner::OnDiscoveredDeviceOverBleSuccess(appState=0x0000000108d11900, connObj=0x000000010bf953c0) at CHIPDeviceController.cpp:795:15

Now why we end up doing CloseAllBleConnections from establishing a BLE connection I don't know, but until we stop doing that, we can't cancel the actual connection that got established from there.... For now this gets us back to the known-good state we were in since 1.0. Once we figure out what's going on with that bit and the shutdown issues #27607 describes, we can put this back.

Copy link
Contributor

@vivien-apple vivien-apple left a comment

Choose a reason for hiding this comment

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

I think we should revert as you suggest. The original patch may probably be retargeted to call mSystemState->BleLayer()->CancelBleIncompleteConnection(); in DeviceCommissioner::OnSessionEstablishmentError.

Something like:

void DeviceCommissioner::OnSessionEstablishmentError(CHIP_ERROR err)
{
   ...
   if (mPairingDelegate != nullptr)
   {
        mPairingDelegate->OnStatusUpdate(DevicePairingDelegate::SecurePairingFailed);
    }

    auto device = self->mDeviceInPASEEstablishment;
    if (nullptr != device && device->GetDeviceTransportType() == Transport::Type::kBle)
    {
        mSystemState->BleLayer()->CancelBleIncompleteConnection();
    }

    RendezvousCleanup(err);
}

@bzbarsky-apple bzbarsky-apple merged commit f3c498f into project-chip:master Jul 24, 2023
51 checks passed
@bzbarsky-apple bzbarsky-apple deleted the revert-ble-cancel branch July 24, 2023 19:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Unable to pair M5 board / nordic using chip tool
5 participants