Hi Chris,

On 13 January 2017 at 03:19, Christopher Collins <ccoll...@apache.org>
wrote:

> Hi Łukasz,
>
> It sounds good to me.  I just have a few questions:
>
> On Thu, Jan 12, 2017 at 09:41:49PM +0100, Łukasz Rymanowski wrote:
> > Outgoing connection:
> > 1. *chan = ble_l2cap_connect(conn_handle, psm, mtu,
> >                             ble_l2cap_chan_ops *ops,
> >                             struct os_mbuf *sdu_rx);
>
> Currently, the struct ble_l2cap_chan type is hidden from the user.  Does
> the application need to do anything with the channel object?


struct ble_l2cap_chan is still hidden from the user. It is used as the
channel
identifier only, that's also why we need to always pass it to any  API
function.


> Also, what
> is the purpose of the sdu_rx parameter?  I am just wondering why the
> user passes it to this function rather than calling
> ble_l2cap_recv_ready() immediately afterwards.
>

My thinking is, when user sends ble_l2cap_connect() he is already ready for
receiving, therefore no
need for additional call. But I will not be pushing on this solution, just
give me any argument to convince me :)

>
> > 2. ops->connected(*chan) callback is called when channel has been
> established. If connection has
> > been rejected, ops->disconnected(*chan, coc_err) is called with coc_err
> which contains reason of reject
>
> If I understand correctly, there is a separate callback for each of the
> three events (connect, disconnect, recv).  I don't think there is a
> problem with this, but it is not totally consistent with how host
> executes the GAP callback.  In GAP, there is a single callback per
> connection.  The function distinguishes the event type by inspecting the
> supplied struct ble_gap_event argument.  I don't think GAP is
> necessarily done correctly; this is just what we ended up with.
>
> For connection-oriented channels, I wonder if it would better to follow
> the GAP convention.


Consistency argument is important. I must say I missed that. Will do in the
way
GAP is done.


>   I'm even wondering if it would be good to indicate
> connection-oriented channel events in the GAP callback itself.  I'm not
> sure what the right answer is, so I'd be interested in what others think
> as well.
>
> Well, I would keep L2CAP separately to not mess up the layers, but maybe
there is some good argument out
there to have it in the same callback.

Finally, there is one more minor inconsistency.  When a GAP connect
> attempt fails, a "connect" event is indicated with a nonzero error code.
> If I understand correctly, a failed CoC-connect results in the
> "disconnect" callback getting executed.  I don't want to push
> consistency above all else, but I thought I should at least mention it.
>

My thinking was to not call connect() callback when in fact we are not
connected and user
needs to do some cleaning which is usually doing on disconnect() callback.
But to keep consistency I will follow GAP way.


> > Open points:
> > 1. As for now we assume that application is responsible for security on
> the link.
>
> Works for me!
>
> Thanks,
> Chris
>


Thanks for comments
Łukasz

Reply via email to