Pairing mode starts advertising and shows the tick before pairing info is saved to flash

[martinwork]

In MicroBitBLEManager, bleDisconnectionCallback calls advertise() immediately but defers saving of the pairing info to the idle callback. The delay is only a few milliseconds, but seems to be long enough for a waiting client device to reconnect before the save to flash. I noticed this seemed to cause a problem when pairing with Windows and I understand this may be causing a problem with some Android clients.

securitySetupCompletedCallback sets the flags that trigger the LEDs tick and the disconnect-and-save sequence in the idle callback. If "PAIRING MODE!" has finished scrolling, the delay between tick and save is probably long enough for a user waiting for the tick to reset micro:bit before the pairing info is saved.

The disconnect-and-save idle callback is not created until "PAIRING MODE!" has finished scrolling. If a user or client device does not need to wait for the LEDs histogram or pairing code display, the client device may be informed that pairing has completed 6 seconds or more before the pairing info is saved.

[jaustin]

@microbit-sam can you take a look at this while you're looking at Pairing mode, please?

[microbit-sam]

In MicroBitBLEManager, bleDisconnectionCallback calls advertise() immediately but defers saving of the pairing info to the idle callback. The delay is only a few milliseconds, but seems to be long enough for a waiting client device to reconnect before the save to flash. I noticed this seemed to cause a problem when pairing with Windows and I understand this may be causing a problem with some Android clients.

Saving is deferred to avoid using the SD to write during a BLE event. As the data is eventually written to the flash using MicroBitFlash (edit: syncronous if SD is in use), saving could be invoked immediately and MicroBitFlash will continuously attempt to write until successful. I'm not sure if this will actually have much effect on the delay as the MB may end up waiting for a successful write, rather than waiting for an idle tick. https://github.com/lancaster-university/microbit-dal/blob/2cff906f019944ae15d532157c1301c940f67862/source/drivers/MicroBitFlash.cpp#L154

securitySetupCompletedCallback sets the flags that trigger the LEDs tick and the disconnect-and-save sequence in the idle callback. If "PAIRING MODE!" has finished scrolling, the delay between tick and save is probably long enough for a user waiting for the tick to reset micro:bit before the pairing info is saved.

Is a solution to only display the tick once the pairing info is saved. Or are you suggesting that MICROBIT_BLE_DISCONNECT_AFTER_PAIRING_DELAY needs to be reduced from 500ms?

The disconnect-and-save idle callback is not created until "PAIRING MODE!" has finished scrolling. If a user or client device does not need to wait for the LEDs histogram or pairing code display, the client device may be informed that pairing has completed 6 seconds or more before the pairing info is saved.

    ble->gap().setAdvertisingTimeout(0);
    ble->gap().startAdvertising();

    fiber_add_idle_component(this);

    // Stop any running animations on the display
    display.stopAnimation();
    display.scroll(msg);

    // Display our name, visualised as a histogram in the display to aid identification.
    showNameHistogram(display);

    // fiber_add_idle_component(this);

[martinwork]

The disconnection callback probably needs to return and the deferred flash is required (flash_burn is synchronous). I was thinking more that advertising maybe should be delayed too, when the pairing data is going to be saved (not always the case when the callback is called). Advertising before saving doesn't cause a problem with iOS but seemed to with Windows standard pairing dialogues. However just delaying advertising on it's own doesn't allow Windows to pair reliably. I think it was @jaustin who knew of Android problems and wondered if this could be the cause. If this doesn't seem to be an issue with Android, then maybe it's not worth changing.

I think the problem is that if the client reconnects bleSysAttrMissingCallback gets called, which changes the attributes that would be read and saved in storeSystemAttributes. A secondary problem is that storeSystemAttributes saves data for all deviceIDs at once and the first save stores junk for all the other deviceIDs, so a second device trying to pair and reconnecting too soon could behave differently from the first.

MICROBIT_BLE_DISCONNECT_AFTER_PAIRING_DELAY was added to solve a problem with some Android devices, so probably is required.

moving fiber_add_idle_component before the scrolling message seems right to me. Could the message be cut short if pairing completes?

The LEDs tick is supposed to indicate all-OK so really shouldn't show until the pairing info is saved. The tick can be delayed by changing when pairingStatus gets MICROBIT_BLE_PAIR_COMPLETE added as follows:

void MicroBitBLEManager::pairingComplete(bool success) { pairing_completed_at_time = system_timer_current_time();

if (success)
{
    this->pairingStatus = MICROBIT_BLE_PAIR_SUCCESSFUL;
    this->status |= MICROBIT_BLE_STATUS_DISCONNECT;
}
else
{
    this->pairingStatus = MICROBIT_BLE_PAIR_COMPLETE;
}

}

And the last bit of idleTick()

if (this->status & MICROBIT_BLE_STATUS_STORE_SYSATTR)
{
    storeSystemAttributes(pairingHandle);
    this->status &= ~MICROBIT_BLE_STATUS_STORE_SYSATTR;
    if ( this->pairingStatus == MICROBIT_BLE_PAIR_SUCCESSFUL)
        this->pairingStatus |= MICROBIT_BLE_PAIR_COMPLETE;
}

[martinwork]

Thinking again... Maybe the only reason pairing mode disconnects before calling storeSystemAttributes is that flash_burn used not to work with BLE connected. I'm not sure if it could be called inside pairingComplete or if the order of the steps in idleTick should be reversed.

[microbit-pauline]

@sam can you test and confirm its integrated?

[microbit-pauline]

@microbit-sam - you test and confirm its integrated?

[microbit-sam]

Nothing in master for this yet

@martinwork does something like this make sense: https://github.com/lancaster-university/microbit-dal/compare/master...microbit-sam:pairing-delay-tick-advertise

[martinwork]

It makes sense to me that it shouldn't advertise until the pairing info has saved, but has anyone identified a problem that this change alone will fix? See my previous comment from 16 Apr.

Is using fibre_sleep() safe here? I'm not saying it isn't! I would have just delayed setting the COMPLETE flag as above.

The only problem I can see is that bleDisconnectionCallback is called whenever BLE disconnects and still needs to call advertise() if idleTick() won't be called because fiber_add_idle_component() hasn't been called.

MicroBitBLEManager::manager-> is redundant inside MicroBitBLEManager::idleTick().

[microbit-sam]

I've taken your suggestions from 16th April and included them

Shifted around storeSystemAttributes and MICROBIT_BLE_PAIR_COMPLETE as I think you're right about avoiding simultaneous BLE and flash write activity

https://github.com/lancaster-university/microbit-dal/compare/master...microbit-sam:pairing-delay-mw

[martinwork]

Will you ensure that advertise() always gets called?

[microbit-sam]

Do you mean when disconnecting?

    if (this->status & MICROBIT_BLE_STATUS_DISCONNECT)
    {
        if((system_timer_current_time() - pairing_completed_at_time) >= MICROBIT_BLE_DISCONNECT_AFTER_PAIRING_DELAY) {
            if (ble)
                ble->disconnect(pairingHandle, Gap::REMOTE_DEV_TERMINATION_DUE_TO_POWER_OFF);
            this->status &= ~MICROBIT_BLE_STATUS_DISCONNECT;
        }
        // Once disconnected, restart advertising
        advertise();
    }

[martinwork]

No. That would be advertising before the pairing data is saved. Pairing complete triggers disconnection which triggers saving.

I think the situation is... bleDisconnectionCallback is hooked up in init() and must always trigger advertise() eventually whether in pairing mode or application mode, but the advertise() in idleTick() will get called only if fiber_add_idle_component() has been called, which only happens in pairing mode.

bleDisconnectionCallback might get called because of intentional disconnection after pairing or because the client app has disconnected or because the connection has broken. It does always set MICROBIT_BLE_STATUS_STORE_SYSATTR.

So maybe bleDisconnectionCallback needs to check if MicroBitBLEManager::manager is an idle component and still call advertise() if not.

bool fiber_is_idle_component(MicroBitComponent *component)
{
    for ( int i = 0; i < MICROBIT_IDLE_COMPONENTS; i++)
    {
      if ( idleThreadComponents[i] == component)
        return true;
    }
    return false;
}

The line while(status == MICROBIT_BLE_STATUS_STORE_SYSATTR); makes me nervous! The pause image seems like a good idea but I don't think it will be displayed until after the saving is done, because the idleTick code will be called inside fiber_sleep. I would just set COMPLETE after SYSATTR is cleared. Would it work if the pause image was shown in a new block before the COMPLETE one:

if (pairingStatus & MICROBIT_BLE_PAIR_SUCCESSFUL)
 {
  // Display while writing 
  MicroBitImage pause("0,0,0,0,0\n0,0,255,0,0\n0,255,0,255,0\n0,0,255,0,0\n0,0,0,0,0\n");
  display.print(pause, 0, 0, 0);
}

if (pairingStatus & MICROBIT_BLE_PAIR_COMPLETE)...

[microbit-sam]

Ok.. I think this might be right?

• onBLEDisconnect only calls advertise if there is no idle thread
• If there is an idle thread advertise is called after storeSystemAttr
• MICROBIT_BLE_PAIR_COMPLETE is only set once storeSystemAttr is called

https://github.com/lancaster-university/microbit-dal/compare/master...microbit-sam:pairing-delay-mw

[martinwork]

Looks good.

[jaustin]

@microbit-sam can we have an independent PR with just this changeset for review please? #409 is handy for builds but a bit more tricky to review as it gets quite large.