Improve API to facilitate full shutdown procedure

[andresag01]

The BLE API exposes a shutdown() function in BLE.h. This function is meant to
be overwridden by platform-specific sub-classes to clear all GAP and GATT
state. However, from the platform-specific implementation it is dificult to
achieve this because the Gap, GattClient, GattServer and SecurityManager
components of the API do not expose any functionality to shutdown.

This commit introduces the following changes:

• Add a static member pointer to Gap, GattClient, GattServer and
  SecurityManager that is used to keep track of the initialized objects.
• Add a function member cleanup() to Gap, GattClient, GattServer and
  SecurityManager to allow easy reset of the instance's state. This function
  is meant to be overriden and called from the derived classes to fully clear the
  state of the BLE API and the platform-specific implementation.
• Add a static member function shutdown() to Gap, GattClient, GattServer and
  SecurityManager. This function shall be called from the shutdown()
  overriding BLE::shutdown() for Gap, GattClient, GattServer and SecurityManager
  that will in-turn clear the state of each of the components.

NOTE: Platform-specific implementations of this API must be modified to
this changes into account.
@rgrover @LiyouZhou @pan-

[andresag01]

Please note that there is a pending pull request for ble-nrf51822 module.

[pan-]

• I do not like the design of the virtual function cleanup, it seems weak to change and porters are forced to explicitly call the function of the base class. There is pattern to solve this problem, the most known is of course NVI (Non Virtual Interface).

The idea is simple, functions exposed are public and non virtual. Functions to overload are private (or protected and virtual). In our case, it will be something like this:

public:
ble_error_t cleanup(void) {
       ble_error_t err = doCleanup();
       if (err) {
           return err;
       }

        /* Clear Gap state */
        state.advertising = 0;
        state.connected = 0;

        /* Clear scanning state */
        scanningActive = false;

        /* Clear advertising and scanning data */
        _advPayload.clear();
        _scanResponse.clear();

        return BLE_ERROR_NONE;
}

private:
virtual ble_error_t doCleanup() = 0;

You can read more about this pattern here: http://www.gotw.ca/publications/mill18.htm

• I'm concerned about the initialization of all the instances, is it the responsibility of the porter to set the pointer of the instances ? I would rather prefer to have something more explicit and visible on the interface side.
• This lead to another question: If the shutdown function is not static why would we keep the instance somewhere in the interface side ? Removing the static instance field also solve the problem of multiples instances of BLE. I think that if pointer should be kept somewhere, it should be in a BLE instance, a GattClient, GattServer or SM belongs to a particular instance of BLE and it is already the case because it is possible to call ble.gap().
• I don't see where shutdown or cleanup are called in the BLE shutdown function:

ble_error_t BLE::shutdown(void)
{
    if (!transport) {
        error("bad handle to underlying transport");
    }

    ble_error_t err = gap().shutdown();
    if (err) { return err; }

    err = gattClient().shutdown();
    if (err) { return err; }

    // OTHER SHUTDOWN PROCEDURE

    return transport->shutdown();
}

• How the user is supposed to cleanup the callbacks or state registered in all these objects ?

[rgrover]

should the callback states also be reset? similar to what you've done for GattClient.

[andresag01]

Agreed, I missed this ones. They should also be cleared.

[pan-]

maybe it would be interesting to call clearAdvertisingPayload and clearScanResponse, it seems that these functions does more things.

[rgrover]

is this still needed: https://github.com/ARMmbed/ble/blob/master/source/BLE.cpp#L134?

[rgrover]

Now that I look at the pull request as a whole, I realize that I may have partly misguided Andres.

Gap should not have

private:
    static Gap *gapInstance;

The same for GattServer, GattClient, etc.

A Gap object should be able to offer an API for cleanup() [we can also call it reset()]. cleanup() can be public. Same for others.

The class nRF5xn, which extends BLEInstanceBase should privately maintain pointers to objects of type nRF5xGap, nRF5xGattServer, etc. These pointers start out as NULL, and can be initialized in the accessors like getGap() etc.

nRF5xn::shutdown() can then choose to call the cleanup() [or reset()] methods for the nRF5xGap, nRF5xGattServer.. objects if they have been allocated previously.

Within nRF5xGap::cleanup(), we can call Gap::cleanup() to reset the state of the parent class, before doing other things to clean the state of the derived nRF5xGap.

Does this make sense?

I'm suggesting that we rename cleanup() --> reset(), but this is a mild suggestion. I will be OK with cleanup if you choose to stick with it.

Please comment.

[andresag01]

@rgrover: https://github.com/ARMmbed/ble/blob/master/source/BLE.cpp#L134 is required depending on the actual implementation. For instance in the nRF51 I have the impression that is required because it clears the advertising data in the SoftDevice.

[pan-]

it just call the function clearAdvertisingPayload from Gap.

   void clearAdvertisingPayload(void) {
        gap().clearAdvertisingPayload();
    }

I think it belongs to Gap cleanup

[rgrover]

@andresag01 yes, I believe https://github.com/ARMmbed/ble/blob/master/source/BLE.cpp#L134 should be transferred to Gap::cleanup()

[andresag01]

@rgrover @pan- : The clearAdvertisingPayload() call https://github.com/ARMmbed/ble/blob/master/source/BLE.cpp#L134 has been completely removed. The reason for this is that in the ble-nrf51822 port it results in a call to the softdevice. This is problematic because we would have already disabled the softdevice by then. The same effect can be achieved by the code I have placed Gap. For more information have a look here and here.

[andresag01]

@rgrover, @pan- : I have made the changes discussed.

[pan-]

Like we discuss yesterday, a big question remain: How to notify user code that a shutdown has been made ?

This is really important to cleanup pending operations like read or write. For instance, I have added a function in DiscoveredCharacteristic which allow the user to pass a continuation function in a read or write operation.

Internally, a new callback object is dynamically allocated, registered into GattClient::onread or GattClient::onWrite, it will filter events from the stack until the event from the registered operation occur, then it will call the user callback and delete itself.

If there are no means to know when a shutdown occur, objects like this will never been released and memory will leak.

[rgrover]

@pan- can we create a generic abstraction to capture such temporary state? Can you register such temporary objects with GattClient such that GattClient::shutdown() [or nRFGattClient::shutdown()] will clean them up?

In general, it will be the user's code which calls shutdown(); and so the application should be aware of when shutdown is triggered. shutdown is a multi-stage process. It won't be actualized until we call softdevice_shutdown(); but once we call softdevice_shutdown() then all system state is reset and we won't get any callbacks from the softdevice to indicate state teardown. Any state that needs to be torn down alongside shutdown should be handled in Gap::shutdown() etc.

[rgrover]

@andresag01 +1 for this PR. We'll wait on @pan- to approve.

[andresag01]

@rgrover @pan- Should we merge these changes?

[rgrover]

I'm happy for this to go ahead as it stands now. And then we can add onShutdown() as a next step.

[rgrover]

Andres, please wait for @pan- to approve.

[rgrover]

published "ble-2.1.16". I had to do this because I had merged in the corresponding change to ble-nrf51822 without realizing that it needed this PR to build.