Re: NimBLE host GAP event listeners

2018-09-10 Thread Andrzej Kaczmarek
Hi,

On Mon, Sep 10, 2018 at 9:56 AM Łukasz Rymanowski <
lukasz.rymanow...@codecoup.pl> wrote:

> Hi
> On Fri, 7 Sep 2018 at 21:12, Andrzej Kaczmarek <
> andrzej.kaczma...@codecoup.pl> wrote:
>
> > Hi Chris,
> >
> > On Fri, Sep 7, 2018 at 8:00 PM Christopher Collins 
> > wrote:
> >
> > > Hello all,
> > >
> > > TL;DR: Proposal for registration of GAP event listeners in the NimBLE
> > > host.  Currently, GAP event callbacks are specified by the code which
> > > creates a connection.  This proposal allows code to listen for GAP
> > > events without creating a connection.
> > >
> > > The NimBLE host allows the application to associate callbacks with GAP
> > > events.  Callbacks are configured *per connection*.  When the
> > > application initiates a connection, it specifies a single callback to
> > > be executed for all GAP events involving the connection.  Similarly,
> > > when the application starts advertising, it specifies the callback to
> > > use with the connection that may result.
> > >
> > > This design provides some degree of modularity.  If a package
> > > independently creates a connection, it can handle all the relevant GAP
> > > events without involving other packages in the system.  However, there
> > > is a type of modularity that this does not provide: *per event*
> > > callbacks.  If a package doesn't want to initiate any GAP events, but
> > > it wants to be notified every time a connection is established (for
> > > example), there is no way to achieve this.  The problem here is that
> > > there is only a single callback associated with each connection.  What
> > > we really need is a list of callbacks that all get called whenever a
> > > GAP event occurs.
> > >
> > > My proposal is to add the following to the NimBLE host API:
> > >
> > > struct ble_gap_listener {
> > > /*** Public. */
> > > ble_gap_event_fn *fn;
> > > void *arg;
> > >
> > > /*** Internal. */
> > > STAILQ_ENTRY(ble_gap_listener) next;
> > > };
> > >
> > > /**
> > >  * @brief Registers a BLE GAP listener.
> > >  *
> > >  * When a GAP event occurs, callbacks are executed in the following
> > >  * order:
> > >  * 1. Connection-specific GAP event callback.
> > >  * 2. Each registered listener, in the order they were registered.
> > >  *
> > >  * @param listener The listener to register.
> > >  */
> > > void ble_gap_register(struct ble_gap_listener *listener);
> > >
> > > /**
> > >  * @brief Unregisters a BLE GAP listener.
> > >  *
> > >  * @param listener The listener to unregister.
> > >  *
> > >  * @return  0 on success;
> > >  *  BLE_HS_ENOENT if the listener is
> > >  *  not registered.
> > >  */
> > > int ble_gap_unregister(struct ble_gap_listener *listener);
> > >
> > > Initially, I thought this functionality could be achieved with a new
> > > package implemented on top of the host (call it `blemux`).
> > > Unfortunately, I think there are some issues that make such an approach
> > > unwieldy for the user.  I imagined such a package would be used as
> > > follows:
> > >
> > > 1. The `blemux` package exposes a GAP callback (call it
> > >`blemux_gap_event`).
> > > 2. Elsewhere in the sytem, for each GAP function call, the caller
> > >specifies `blemux_gap_event` as the callback.
> > > 3. Each package that is interested in GAP events registers one or
> > >more listeners via `blemux_register()`.
> > > 4. When a GAP event occurs, `blemux_gap_event` is executed.  This
> > >callback executes every registered listener.
> > >
> > > The problem is that packages have no guarantee that the app is using
> > > blemux.  A package can depend on `blemux` and can register listeners,
> > > but it really only works if every other BLE-aware package in the system
> > > also uses `blemux`.  If any package doesn't use `blemux`, then the GAP
> > > procedures that it initiates won't be reported to the `blemux`
> > > listeners.  A secondary problem is that this just feels like a clumsy
> > > API.  It is confusing and error prone to need to specify
> > > `blemux_gap_event` as the GAP event callback.
> > >
> > > So, I think this functionality should be implemented directly in the
> > > host.
> > >
> >
> >
> Like the idea. We could use it for mesh as well - for now we have dedicated
> API for Mesh to register for GAP events.
> With this change we could get rid of it.
>

Indeed. Also we can simplify existing services.


> > Sounds exactly like something I wrote some time ago, so I can only agree
> > with above :-)
> > Typical scenario here (and what I was doing) is that external package,
> like
> > service, wants to be notified about connection or characteristic
> > subscription state. Currently we do this by exposing extra API from
> package
> > which app needs to 

Re: NimBLE host GAP event listeners

2018-09-10 Thread Łukasz Rymanowski
Hi
On Fri, 7 Sep 2018 at 21:12, Andrzej Kaczmarek <
andrzej.kaczma...@codecoup.pl> wrote:

> Hi Chris,
>
> On Fri, Sep 7, 2018 at 8:00 PM Christopher Collins 
> wrote:
>
> > Hello all,
> >
> > TL;DR: Proposal for registration of GAP event listeners in the NimBLE
> > host.  Currently, GAP event callbacks are specified by the code which
> > creates a connection.  This proposal allows code to listen for GAP
> > events without creating a connection.
> >
> > The NimBLE host allows the application to associate callbacks with GAP
> > events.  Callbacks are configured *per connection*.  When the
> > application initiates a connection, it specifies a single callback to
> > be executed for all GAP events involving the connection.  Similarly,
> > when the application starts advertising, it specifies the callback to
> > use with the connection that may result.
> >
> > This design provides some degree of modularity.  If a package
> > independently creates a connection, it can handle all the relevant GAP
> > events without involving other packages in the system.  However, there
> > is a type of modularity that this does not provide: *per event*
> > callbacks.  If a package doesn't want to initiate any GAP events, but
> > it wants to be notified every time a connection is established (for
> > example), there is no way to achieve this.  The problem here is that
> > there is only a single callback associated with each connection.  What
> > we really need is a list of callbacks that all get called whenever a
> > GAP event occurs.
> >
> > My proposal is to add the following to the NimBLE host API:
> >
> > struct ble_gap_listener {
> > /*** Public. */
> > ble_gap_event_fn *fn;
> > void *arg;
> >
> > /*** Internal. */
> > STAILQ_ENTRY(ble_gap_listener) next;
> > };
> >
> > /**
> >  * @brief Registers a BLE GAP listener.
> >  *
> >  * When a GAP event occurs, callbacks are executed in the following
> >  * order:
> >  * 1. Connection-specific GAP event callback.
> >  * 2. Each registered listener, in the order they were registered.
> >  *
> >  * @param listener The listener to register.
> >  */
> > void ble_gap_register(struct ble_gap_listener *listener);
> >
> > /**
> >  * @brief Unregisters a BLE GAP listener.
> >  *
> >  * @param listener The listener to unregister.
> >  *
> >  * @return  0 on success;
> >  *  BLE_HS_ENOENT if the listener is
> >  *  not registered.
> >  */
> > int ble_gap_unregister(struct ble_gap_listener *listener);
> >
> > Initially, I thought this functionality could be achieved with a new
> > package implemented on top of the host (call it `blemux`).
> > Unfortunately, I think there are some issues that make such an approach
> > unwieldy for the user.  I imagined such a package would be used as
> > follows:
> >
> > 1. The `blemux` package exposes a GAP callback (call it
> >`blemux_gap_event`).
> > 2. Elsewhere in the sytem, for each GAP function call, the caller
> >specifies `blemux_gap_event` as the callback.
> > 3. Each package that is interested in GAP events registers one or
> >more listeners via `blemux_register()`.
> > 4. When a GAP event occurs, `blemux_gap_event` is executed.  This
> >callback executes every registered listener.
> >
> > The problem is that packages have no guarantee that the app is using
> > blemux.  A package can depend on `blemux` and can register listeners,
> > but it really only works if every other BLE-aware package in the system
> > also uses `blemux`.  If any package doesn't use `blemux`, then the GAP
> > procedures that it initiates won't be reported to the `blemux`
> > listeners.  A secondary problem is that this just feels like a clumsy
> > API.  It is confusing and error prone to need to specify
> > `blemux_gap_event` as the GAP event callback.
> >
> > So, I think this functionality should be implemented directly in the
> > host.
> >
>
>
Like the idea. We could use it for mesh as well - for now we have dedicated
API for Mesh to register for GAP events.
With this change we could get rid of it.


> Sounds exactly like something I wrote some time ago, so I can only agree
> with above :-)
> Typical scenario here (and what I was doing) is that external package, like
> service, wants to be notified about connection or characteristic
> subscription state. Currently we do this by exposing extra API from package
> which app needs to call from GAP callbacks in order to everything work
> properly. It works, but it's really clunky. Hooks provided by host are imo
> good way to handle this.
>
> I did not finish my service so forgot to send a PR with NimBLE changes, but
> here they are for reference - I can create PR if they look ok.
>
> 

NimBLE host GAP event listeners

2018-09-07 Thread Christopher Collins
Hello all,

TL;DR: Proposal for registration of GAP event listeners in the NimBLE
host.  Currently, GAP event callbacks are specified by the code which
creates a connection.  This proposal allows code to listen for GAP
events without creating a connection.

The NimBLE host allows the application to associate callbacks with GAP
events.  Callbacks are configured *per connection*.  When the
application initiates a connection, it specifies a single callback to
be executed for all GAP events involving the connection.  Similarly,
when the application starts advertising, it specifies the callback to
use with the connection that may result.

This design provides some degree of modularity.  If a package
independently creates a connection, it can handle all the relevant GAP
events without involving other packages in the system.  However, there
is a type of modularity that this does not provide: *per event*
callbacks.  If a package doesn't want to initiate any GAP events, but
it wants to be notified every time a connection is established (for
example), there is no way to achieve this.  The problem here is that
there is only a single callback associated with each connection.  What
we really need is a list of callbacks that all get called whenever a
GAP event occurs.

My proposal is to add the following to the NimBLE host API:

struct ble_gap_listener {
/*** Public. */
ble_gap_event_fn *fn;
void *arg;

/*** Internal. */
STAILQ_ENTRY(ble_gap_listener) next;
};

/**
 * @brief Registers a BLE GAP listener.
 *
 * When a GAP event occurs, callbacks are executed in the following
 * order:
 * 1. Connection-specific GAP event callback.
 * 2. Each registered listener, in the order they were registered.
 *
 * @param listener The listener to register.
 */
void ble_gap_register(struct ble_gap_listener *listener);

/**
 * @brief Unregisters a BLE GAP listener.
 *
 * @param listener The listener to unregister.
 *
 * @return  0 on success;
 *  BLE_HS_ENOENT if the listener is
 *  not registered.
 */
int ble_gap_unregister(struct ble_gap_listener *listener);

Initially, I thought this functionality could be achieved with a new
package implemented on top of the host (call it `blemux`).
Unfortunately, I think there are some issues that make such an approach
unwieldy for the user.  I imagined such a package would be used as
follows:

1. The `blemux` package exposes a GAP callback (call it
   `blemux_gap_event`).
2. Elsewhere in the sytem, for each GAP function call, the caller
   specifies `blemux_gap_event` as the callback.  
3. Each package that is interested in GAP events registers one or
   more listeners via `blemux_register()`.
4. When a GAP event occurs, `blemux_gap_event` is executed.  This
   callback executes every registered listener.

The problem is that packages have no guarantee that the app is using
blemux.  A package can depend on `blemux` and can register listeners,
but it really only works if every other BLE-aware package in the system
also uses `blemux`.  If any package doesn't use `blemux`, then the GAP
procedures that it initiates won't be reported to the `blemux`
listeners.  A secondary problem is that this just feels like a clumsy
API.  It is confusing and error prone to need to specify
`blemux_gap_event` as the GAP event callback.

So, I think this functionality should be implemented directly in the
host.

Thoughts?

Thanks,
Chris