* CSP implementation for MCAP
@ 2010-09-02 18:58 Elvis Pfützenreuter
2010-09-02 20:28 ` Johan Hedberg
` (2 more replies)
0 siblings, 3 replies; 12+ messages in thread
From: Elvis Pfützenreuter @ 2010-09-02 18:58 UTC (permalink / raw)
To: johan.hedberg, linux-bluetooth
This is the repository for the CSP implementation, rebased over the recently accepted MCAP, for your appreciation:
git://gitorious.org/bluez-epx/bluez-epx.git csp
or
http://www.gitorious.org/bluez-epx/bluez-epx/commits/csp
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CSP implementation for MCAP
2010-09-02 18:58 CSP implementation for MCAP Elvis Pfützenreuter
@ 2010-09-02 20:28 ` Johan Hedberg
2010-09-02 20:36 ` Elvis Pfützenreuter
2010-09-03 8:03 ` Santiago Carot-Nemesio
2010-09-15 22:45 ` Elvis Pfützenreuter
2 siblings, 1 reply; 12+ messages in thread
From: Johan Hedberg @ 2010-09-02 20:28 UTC (permalink / raw)
To: Elvis Pfützenreuter; +Cc: linux-bluetooth
Hi Elvis,
On Thu, Sep 02, 2010, Elvis Pfützenreuter wrote:
> This is the repository for the CSP implementation, rebased over the
> recently accepted MCAP, for your appreciation:
>
> git://gitorious.org/bluez-epx/bluez-epx.git csp
A couple of quick comments:
- Several of the commits in the tree have the author as
"epx <epx@xenicall.(none)>". That needs fixing.
- I'm not so happy about all the ifdefs in the code and the way that
they are used. So this may need some rethinking. Probably we could
have the code always compiled in and have a runtime variable to
disable its use.
Johan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CSP implementation for MCAP
2010-09-02 20:28 ` Johan Hedberg
@ 2010-09-02 20:36 ` Elvis Pfützenreuter
2010-09-02 20:40 ` Johan Hedberg
0 siblings, 1 reply; 12+ messages in thread
From: Elvis Pfützenreuter @ 2010-09-02 20:36 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
On Sep 2, 2010, at 5:28 PM, Johan Hedberg wrote:
> Hi Elvis,
>
> On Thu, Sep 02, 2010, Elvis Pfützenreuter wrote:
>> This is the repository for the CSP implementation, rebased over the
>> recently accepted MCAP, for your appreciation:
>>
>> git://gitorious.org/bluez-epx/bluez-epx.git csp
>
> A couple of quick comments:
>
> - Several of the commits in the tree have the author as
> "epx <epx@xenicall.(none)>". That needs fixing.
Ouch :(
> - I'm not so happy about all the ifdefs in the code and the way that
> they are used. So this may need some rethinking. Probably we could
> have the code always compiled in and have a runtime variable to
> disable its use.
Such variable already exists, and starts turned off. So could I remove the --enable-mcap-csp flag altogether?
> Johan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CSP implementation for MCAP
2010-09-02 20:36 ` Elvis Pfützenreuter
@ 2010-09-02 20:40 ` Johan Hedberg
2010-09-02 21:49 ` Elvis Pfützenreuter
0 siblings, 1 reply; 12+ messages in thread
From: Johan Hedberg @ 2010-09-02 20:40 UTC (permalink / raw)
To: Elvis Pfützenreuter; +Cc: linux-bluetooth
Hi Elvis,
On Thu, Sep 02, 2010, Elvis Pfützenreuter wrote:
> > - I'm not so happy about all the ifdefs in the code and the way that
> > they are used. So this may need some rethinking. Probably we could
> > have the code always compiled in and have a runtime variable to
> > disable its use.
>
> Such variable already exists, and starts turned off. So could I remove
> the --enable-mcap-csp flag altogether?
Yes, I think a single --enable-mcap that covers also csp should be
enough.
Johan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CSP implementation for MCAP
2010-09-02 20:40 ` Johan Hedberg
@ 2010-09-02 21:49 ` Elvis Pfützenreuter
0 siblings, 0 replies; 12+ messages in thread
From: Elvis Pfützenreuter @ 2010-09-02 21:49 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
On Sep 2, 2010, at 5:40 PM, Johan Hedberg wrote:
> Hi Elvis,
>
> On Thu, Sep 02, 2010, Elvis Pfützenreuter wrote:
>>> - I'm not so happy about all the ifdefs in the code and the way that
>>> they are used. So this may need some rethinking. Probably we could
>>> have the code always compiled in and have a runtime variable to
>>> disable its use.
>>
>> Such variable already exists, and starts turned off. So could I remove
>> the --enable-mcap-csp flag altogether?
>
> Yes, I think a single --enable-mcap that covers also csp should be
> enough.
Ok, those first changes have been made and tested. Waiting for the next round.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CSP implementation for MCAP
2010-09-02 18:58 CSP implementation for MCAP Elvis Pfützenreuter
2010-09-02 20:28 ` Johan Hedberg
@ 2010-09-03 8:03 ` Santiago Carot-Nemesio
2010-09-03 12:53 ` Elvis Pfützenreuter
2010-09-15 22:45 ` Elvis Pfützenreuter
2 siblings, 1 reply; 12+ messages in thread
From: Santiago Carot-Nemesio @ 2010-09-03 8:03 UTC (permalink / raw)
To: Elvis Pfützenreuter; +Cc: johan.hedberg, linux-bluetooth
Hi Elvis,
On 09/02/10 20:58, Elvis Pfützenreuter wrote:
> This is the repository for the CSP implementation, rebased over the recently accepted MCAP, for your appreciation:
>
> git://gitorious.org/bluez-epx/bluez-epx.git csp
>
I have been divin in CSP code and at first look I found some issues that
I would like discuss with you.
The call to function mcap_sync_stop(mcl) it's only neccesary in static
close_mcl function. You can remove the second call in line 799 when user
closes explicitly the mcl due that first sync_stop will be called from
watcher set in control channel when its socket is closed.
Second thing is related to code structure. Because standard op codes and
close synchronization protocol have separate logic, it may better put
csp parameters away from mcl in a separate structure to avoid mess all
protocol logic in a big mcl structure, I was thinking in something like
this:
struct mcap_mcl {
/* Op code parameters */
....
struct mcap_csp *csp;
}
struct mcap_csp {
uint64_t base_tmstamp;
struct timespec base_time;
guint local_caps;
guint remote_caps;
guint rem_req_acc;
guint ind_expected;
MCAPCtrl csp_req;
guint ind_timer;
guint set_timer;
void *set_data;
gint dev_id;
gint dev_hci_fd;
void *csp_priv_data;
};
Because CSP is optional we can reserve memory for csp only when we will
use Clock Synchronization protocol.
Of course, I know that it depends on invidivual taste, but I would like
to comment this issue.
Comments are welcome.
Regards.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CSP implementation for MCAP
2010-09-03 8:03 ` Santiago Carot-Nemesio
@ 2010-09-03 12:53 ` Elvis Pfützenreuter
0 siblings, 0 replies; 12+ messages in thread
From: Elvis Pfützenreuter @ 2010-09-03 12:53 UTC (permalink / raw)
To: Santiago Carot-Nemesio; +Cc: linux-bluetooth
On Sep 3, 2010, at 5:03 AM, Santiago Carot-Nemesio wrote:
> Hi Elvis,
>
> On 09/02/10 20:58, Elvis Pfützenreuter wrote:
>> This is the repository for the CSP implementation, rebased over the recently accepted MCAP, for your appreciation:
>>
>> git://gitorious.org/bluez-epx/bluez-epx.git csp
>>
>
> I have been divin in CSP code and at first look I found some issues that I would like discuss with you.
>
> The call to function mcap_sync_stop(mcl) it's only neccesary in static close_mcl function. You can remove the second call in line 799 when user closes explicitly the mcl due that first sync_stop will be called from watcher set in control channel when its socket is closed.
>
> Second thing is related to code structure. Because standard op codes and close synchronization protocol have separate logic, it may better put csp parameters away from mcl in a separate structure to avoid mess all protocol logic in a big mcl structure, I was thinking in something like this:
>
> struct mcap_mcl {
> /* Op code parameters */
> ....
> struct mcap_csp *csp;
> }
>
> struct mcap_csp {
> uint64_t base_tmstamp;
> struct timespec base_time;
> guint local_caps;
> guint remote_caps;
> guint rem_req_acc;
> guint ind_expected;
> MCAPCtrl csp_req;
> guint ind_timer;
> guint set_timer;
> void *set_data;
> gint dev_id;
> gint dev_hci_fd;
> void *csp_priv_data;
> };
>
> Because CSP is optional we can reserve memory for csp only when we will use Clock Synchronization protocol.
> Of course, I know that it depends on invidivual taste, but I would like to comment this issue.
Sounds sensible.
>
> Comments are welcome.
>
> Regards.
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CSP implementation for MCAP
2010-09-02 18:58 CSP implementation for MCAP Elvis Pfützenreuter
2010-09-02 20:28 ` Johan Hedberg
2010-09-03 8:03 ` Santiago Carot-Nemesio
@ 2010-09-15 22:45 ` Elvis Pfützenreuter
2010-09-16 12:39 ` Johan Hedberg
2 siblings, 1 reply; 12+ messages in thread
From: Elvis Pfützenreuter @ 2010-09-15 22:45 UTC (permalink / raw)
To: linux-bluetooth
On 02/09/2010, at 15:58, Elvis Pfützenreuter wrote:
> This is the repository for the CSP implementation, rebased over the recently accepted MCAP, for your appreciation:
>
> git://gitorious.org/bluez-epx/bluez-epx.git csp
>
> or
>
> http://www.gitorious.org/bluez-epx/bluez-epx/commits/csp--
And I am happy to announce that this CSP implementation passed on PTS :) Fixes are on topmost patch of the repository.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CSP implementation for MCAP
2010-09-15 22:45 ` Elvis Pfützenreuter
@ 2010-09-16 12:39 ` Johan Hedberg
2010-09-16 12:55 ` José Antonio Santos Cadenas
2010-09-16 14:14 ` Elvis Pfützenreuter
0 siblings, 2 replies; 12+ messages in thread
From: Johan Hedberg @ 2010-09-16 12:39 UTC (permalink / raw)
To: Elvis Pfützenreuter; +Cc: linux-bluetooth
Hi Elvis,
On Wed, Sep 15, 2010, Elvis Pfützenreuter wrote:
> > This is the repository for the CSP implementation, rebased over the
> > recently accepted MCAP, for your appreciation:
> >
> > git://gitorious.org/bluez-epx/bluez-epx.git csp
> >
> > or
> >
> > http://www.gitorious.org/bluez-epx/bluez-epx/commits/csp--
>
> And I am happy to announce that this CSP implementation passed on PTS
> :) Fixes are on topmost patch of the repository.
Good to hear about the progress with the PTS :)
Unfortunately you'll need to rebase again since I pushed some cleanups
to the MCAP code. Could you also get rid of the unnecessary
double-pointers and type casts which aren't needed anymore now that
mcap_send_data accepts void *.
Johan
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CSP implementation for MCAP
2010-09-16 12:39 ` Johan Hedberg
@ 2010-09-16 12:55 ` José Antonio Santos Cadenas
2010-09-16 14:14 ` Elvis Pfützenreuter
1 sibling, 0 replies; 12+ messages in thread
From: José Antonio Santos Cadenas @ 2010-09-16 12:55 UTC (permalink / raw)
To: Johan Hedberg; +Cc: Elvis Pfützenreuter, linux-bluetooth
El Thursday 16 September 2010 14:39:00 Johan Hedberg escribió:
> Hi Elvis,
>
> On Wed, Sep 15, 2010, Elvis Pfützenreuter wrote:
> > > This is the repository for the CSP implementation, rebased over the
> > > recently accepted MCAP, for your appreciation:
> > >
> > > git://gitorious.org/bluez-epx/bluez-epx.git csp
> > >
> > > or
> > >
> > > http://www.gitorious.org/bluez-epx/bluez-epx/commits/csp--
> >
> > And I am happy to announce that this CSP implementation passed on PTS
> >
> > :) Fixes are on topmost patch of the repository.
>
> Good to hear about the progress with the PTS :)
>
> Unfortunately you'll need to rebase again since I pushed some cleanups
> to the MCAP code. Could you also get rid of the unnecessary
> double-pointers and type casts which aren't needed anymore now that
> mcap_send_data accepts void *.
There are also a lot of (! condition) with an extra space between the ! and
the condition variable.
Also some use of g_malloc0 instead of g_new0. About this issue, remember that
g_new0 initializes all the structure with 0 so the initialization don't need
to do for some fields in mcap_sync_init function.
Regards.
>
> Johan
> --
> To unsubscribe from this list: send the line "unsubscribe linux-bluetooth"
> in the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CSP implementation for MCAP
2010-09-16 12:39 ` Johan Hedberg
2010-09-16 12:55 ` José Antonio Santos Cadenas
@ 2010-09-16 14:14 ` Elvis Pfützenreuter
2010-09-16 15:03 ` Johan Hedberg
1 sibling, 1 reply; 12+ messages in thread
From: Elvis Pfützenreuter @ 2010-09-16 14:14 UTC (permalink / raw)
To: Johan Hedberg; +Cc: linux-bluetooth
On 16/09/2010, at 09:39, Johan Hedberg wrote:
> Hi Elvis,
>
> On Wed, Sep 15, 2010, Elvis Pfützenreuter wrote:
>>> This is the repository for the CSP implementation, rebased over the
>>> recently accepted MCAP, for your appreciation:
>>>
>>> git://gitorious.org/bluez-epx/bluez-epx.git csp
>>>
>>> or
>>>
>>> http://www.gitorious.org/bluez-epx/bluez-epx/commits/csp--
>>
>> And I am happy to announce that this CSP implementation passed on PTS
>> :) Fixes are on topmost patch of the repository.
>
> Good to hear about the progress with the PTS :)
>
> Unfortunately you'll need to rebase again since I pushed some cleanups
> to the MCAP code. Could you also get rid of the unnecessary
> double-pointers and type casts which aren't needed anymore now that
> mcap_send_data accepts void *.
It's done, and tested.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: CSP implementation for MCAP
2010-09-16 14:14 ` Elvis Pfützenreuter
@ 2010-09-16 15:03 ` Johan Hedberg
0 siblings, 0 replies; 12+ messages in thread
From: Johan Hedberg @ 2010-09-16 15:03 UTC (permalink / raw)
To: Elvis Pfützenreuter; +Cc: linux-bluetooth
Hi Elvis,
On Thu, Sep 16, 2010, Elvis Pfützenreuter wrote:
> > On Wed, Sep 15, 2010, Elvis Pfützenreuter wrote:
> >>> This is the repository for the CSP implementation, rebased over the
> >>> recently accepted MCAP, for your appreciation:
> >>>
> >>> git://gitorious.org/bluez-epx/bluez-epx.git csp
> >>>
> >>> or
> >>>
> >>> http://www.gitorious.org/bluez-epx/bluez-epx/commits/csp--
> >>
> >> And I am happy to announce that this CSP implementation passed on PTS
> >> :) Fixes are on topmost patch of the repository.
> >
> > Good to hear about the progress with the PTS :)
> >
> > Unfortunately you'll need to rebase again since I pushed some cleanups
> > to the MCAP code. Could you also get rid of the unnecessary
> > double-pointers and type casts which aren't needed anymore now that
> > mcap_send_data accepts void *.
>
> It's done, and tested.
Thanks. I still had to do quite a lot of coding style fixes as well as
plug a memory leak, but the patches are now pushed upstream.
Next steps:
- Get rid of the forward declarations of static functions
wherever possible. It looked like you could accomplish that
with a simple reordering of the functions.
- Get rid of raw HCI access (hci_open_dev). bluetoothd shouldn't use
this type of sockets in the future at all. You'll need to add a proper
callback to the adapter_opts and a function to plugins/hciopts.c and
use that instead. Then export a function like btd_adapter_read_clock
for the plugin to use.
Johan
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2010-09-16 15:03 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-02 18:58 CSP implementation for MCAP Elvis Pfützenreuter
2010-09-02 20:28 ` Johan Hedberg
2010-09-02 20:36 ` Elvis Pfützenreuter
2010-09-02 20:40 ` Johan Hedberg
2010-09-02 21:49 ` Elvis Pfützenreuter
2010-09-03 8:03 ` Santiago Carot-Nemesio
2010-09-03 12:53 ` Elvis Pfützenreuter
2010-09-15 22:45 ` Elvis Pfützenreuter
2010-09-16 12:39 ` Johan Hedberg
2010-09-16 12:55 ` José Antonio Santos Cadenas
2010-09-16 14:14 ` Elvis Pfützenreuter
2010-09-16 15:03 ` Johan Hedberg
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.