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

BLE API does not handle minimum and maximum intervals as in the specification #133

Open
andresag01 opened this issue Dec 3, 2015 · 7 comments

Comments

@andresag01
Copy link

In the BLE API Gap.h contains the following functions:

  • getMinAdvertisingInterval()
  • getMinNonConnectableAdvertisingInterval()
  • getMaxAdvertising interval()

The Bluetooth v4.2 specification states the following in [Vol 6, Part B] Section 4.4.2.2:

The advInterval shall be an integer multiple of 0.625 ms in the range of 20 ms
to 10.24 s. If the advertising event type is either a scannable undirected event
type or a non-connectable undirected event type, the advInterval shall not be
less than 100 ms. If the advertising event type is a connectable undirected
event type or connectable directed event type used in a low duty cycle mode,
the advInterval can be 20 ms or greater

Taking this into account, it is not clear from the BLE API documentation what are the differences between getMinAdvertisingInterval() and getMinNonConnectableAdvertisingInterval().

@ciarmcom
Copy link
Member

ciarmcom commented Dec 3, 2015

ARM Internal Ref: IOTSFW-1368

@rgrover
Copy link
Contributor

rgrover commented Dec 4, 2015

@andresag01
getMinNonConnectableAdvertisingInterval() returns the interval which applies to non-connectable advertising; whereas getMinAdvertisingInterval() returns the interval for other types of advertising. Is this a case of expanding on the existing documentation?

@andresag01
Copy link
Author

@rgrover: Yes, it seems that this is the case. For instance, the documentation for getMinAdvertisingInterval() states:

    /**
     * @return Minimum Advertising interval in milliseconds.
     */

Which does not mention connectable or non-connectable packets.

@rgrover
Copy link
Contributor

rgrover commented Dec 4, 2015

please submit a PR to fix this documentation

@pan-
Copy link
Member

pan- commented Dec 4, 2015

The specification state that advInterval shall be in range [20ms, 10.24s] which means that the standard mandate that the minimum advertising interval is 20ms.

The standard also say that for scannable undirected and non-connectable undirected, the advertising *shall not be less than 100ms`.

Plus, there is a low duty cycle and high duty cycle advertising rate for connectable directed events.

To summarize advertising interval:

  • ADV_CONNECTABLE_UNDIRECTED => [20:10240]
  • ADV_CONNECTABLE_DIRECTED
    • low duty cycle => [20:10240]
    • high duty cycle => [X:3.75]
  • ADV_SCANNABLE_UNDIRECTED => [100:10240]
  • ADV_NON_CONNECTABLE_UNDIRECTED => [100:10240]

I think that the current API can be improved:

  • This constants are defined by the standard, I'm not sure to understand why it has to be provided by the porter of BLE API.
  • getMinAdvertisingInterval will return a false result because if you want to use connectable directed advertising at a high duty cycle rate.
  • Other constants are defined in the file GapAdvertisingParams, the units are different and the constants not documented, this can be very misleading for the user
    static const unsigned GAP_ADV_PARAMS_INTERVAL_MIN        = 0x0020;
    static const unsigned GAP_ADV_PARAMS_INTERVAL_MIN_NONCON = 0x00A0;
    static const unsigned GAP_ADV_PARAMS_INTERVAL_MAX        = 0x4000;
    static const unsigned GAP_ADV_PARAMS_TIMEOUT_MAX         = 0x3FFF;
  • The function setAdvertisingInterval readjust advertising interval but if the user has choose a non connectable advertising type, the adjustment will not be correct:
    void setAdvertisingInterval(uint16_t interval) {
        if (interval == 0) {
            stopAdvertising();
        } else if (interval < getMinAdvertisingInterval()) {
            interval = getMinAdvertisingInterval();
        }
        _advParams.setInterval(interval);
    }
  • It will be difficult to provide a correct function for advertising interval if the advertising type and advertising interval are not set together "atomically".
  • Comment of function setAdvertisingInterval in Gap.h has issues:
    /*
     * @note: This API is now *deprecated* and will be dropped in the future.
     * You should use the parallel API from Gap directly. A former call to
     * ble.setAdvertisingInterval(...) should now be achieved using
     * ble.gap().setAdvertisingInterval(...).
     */
  • low duty cycle rates for ADV_CONNECTABLE_DIRECTED should be available for setAdvertisingInterval:
This field must be set to 0 if connectionMode is equal to ADV_CONNECTABLE_DIRECTED.

@rgrover
Copy link
Contributor

rgrover commented Dec 4, 2015

I've deleted the obsolete comment block from the documentation of setAdvertisingInterval. Let's file an issue about setAdvertisingInterval and see how we all feel about changing the API so that both the advertising type and interval can be set simultaneously.

@andresag01
Copy link
Author

@pan- I checked the source for both ble and ble-nrf51822 to see what was really going on. Indeed the setAdvertisingInterval() function does not check what the advertising type is. Therefore, the user is allowed to set any X but the API will correct the interval so that X > getMinAdvertisingInterval(). I believe this is the value to check against since getMinAdvertisingInterval() < getMinNonConnectableAdvertisingInterval(). For some reason the value is not checked agains getMaxAdvertisingInterval().

So far it looks like the API allows setting wrong values for the interval without taking into account the advertising type. However, the actual checks are performed in the implementation of startAdvertising(GapAdvertisingParams&) in ble-nrf51822 module. The code can be found here, where clearly the interval is checked against the allowed values and the advertising type and an error returned if the values are not within range.

Strictly speaking there is no bug in the API, but perhaps there is room for improvement that could inform the user of a problem with the advertising parameters, before a call to startAdvertising(). This also has its complications because something like the following could cause a failure:

setAdvertisingInterval(20);         <--- If we return an error code here, there will be a failure since the defaulf advertising type is ADV_CONNECTABLE_UNDIRECTED.
setAdvertisingType(ADV_NON_CONNECTABLE_UNDIRECTED);

@pan- pan- added the bug label Apr 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants