Subject: Pull request git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream

The following changes since commit a4b1673b878a358d2492f24b46e92ea36d8f8bbf:

Fix hidd to use ServiceName attribute for device name (2010-10-20
13:37:36 +0300)

are available in the git repository at:
git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream

Jos? Antonio Santos Cadenas (6):
Receive an string instead of an integer for ChannelType
Modify test-health script to make it more interactive
Notify main channel when it is locally opened
Emit a valid path when main channel is deleted
Delete data channels when their device is removed.
Check if the channel mode is correct when opening data channels

Santiago Carot-Nemesio (6):
Remove obsolete comment from MCAP
Fix segmentation fault freeing uninitialized pointers
Check l2cap configuration when data channels are connected
Close the data channel if remote side changes the configuration
Enable support to change mode for incoming data channels connections
Change data channel mode for incoming connections

health/hdp.c | 106 +++++++++++++++++++++++++--
health/hdp_util.c | 35 +++++++---
health/mcap.c | 15 ++++-
health/mcap_lib.h | 3 +
test/test-health | 207 +++++++++++++++++++++++++++++++++++++++++++++++++++--
5 files changed, 340 insertions(+), 26 deletions(-)


2010-10-22 13:43:26

by Johan Hedberg

[permalink] [raw]
Subject: Re: Pull request git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream

Hi Jose,

On Thu, Oct 21, 2010, Jose Antonio Santos Cadenas wrote:
> The following changes since commit a4b1673b878a358d2492f24b46e92ea36d8f8bbf:
>
> Fix hidd to use ServiceName attribute for device name (2010-10-20
> 13:37:36 +0300)
>
> are available in the git repository at:
> git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream
>
> Jos? Antonio Santos Cadenas (6):
> Receive an string instead of an integer for ChannelType
> Modify test-health script to make it more interactive
> Notify main channel when it is locally opened
> Emit a valid path when main channel is deleted
> Delete data channels when their device is removed.
> Check if the channel mode is correct when opening data channels
>
> Santiago Carot-Nemesio (6):
> Remove obsolete comment from MCAP
> Fix segmentation fault freeing uninitialized pointers
> Check l2cap configuration when data channels are connected
> Close the data channel if remote side changes the configuration
> Enable support to change mode for incoming data channels connections
> Change data channel mode for incoming connections
>
> health/hdp.c | 106 +++++++++++++++++++++++++--
> health/hdp_util.c | 35 +++++++---
> health/mcap.c | 15 ++++-
> health/mcap_lib.h | 3 +
> test/test-health | 207 +++++++++++++++++++++++++++++++++++++++++++++++++++--
> 5 files changed, 340 insertions(+), 26 deletions(-)

Pushed upstream. Thanks.

Johan

2010-10-14 11:15:01

by Johan Hedberg

[permalink] [raw]
Subject: Re: Pull request git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream

Hi Jose,

On Thu, Oct 14, 2010, Jose Antonio Santos Cadenas wrote:
> The following changes since commit 6b9deca15d0824ad5b3c38f8c0dd3a3c482572ea:
>
> TODO: Device Name Characteristic for Low Energy (2010-10-13 22:26:59 +0300)
>
> are available in the git repository at:
> git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream
>
> Jos? Antonio Santos Cadenas (7):
> Connect echo channel
> Initial steps to send echo data
> Generate random echo packet
> Create a function for closing mcl correctly
> Notify channel deletion correctly
> Separate private echo data from data channels
> Add initial support for deleting an echo channel
>
> Santiago Carot-Nemesio (8):
> Add support to initiate echo
> Remove unnecessary variable
> Set watcher to check echo response
> Check echo response
> Add timeout to wait for echo reply
> Abort echo channel if connection was not successful
> Remove MDL when delete operation fails with INVALID_MDLID error
> Remove unnecessary function
>
> health/hdp.c | 431 +++++++++++++++++++++++++++++++++++++++++-----------
> health/hdp_types.h | 5 +-
> health/mcap.c | 14 ++-
> 3 files changed, 361 insertions(+), 89 deletions(-)

Pushed upstream. Thanks.

Johan

2010-10-12 10:15:41

by Johan Hedberg

[permalink] [raw]
Subject: Re: Pull request git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream

Hi Jose,

On Tue, Oct 12, 2010, Jose Antonio Santos Cadenas wrote:
> >> On Tue, Oct 12, 2010, Jose Antonio Santos Cadenas wrote:
> >>> I'm sorry but I can't connect to the IRC rigth now. Nevertheless, I
> >>> think that I've found the issues:
> >>>
> >>> - The iochannel should be unrefereed when the watcher is explicitly removed
> >>
> >> Yes, though do you need to have a reference at all? As long as you have
> >> the GLib watch callback GLib itself holds a reference, so if you don't
> >> need one just remove chan->echo_chan and do an unref after calling
> >> add_watch.
> >
> > Sorry I've forgot this one. Gonna solve it.
>
> Solved, I've removed the chan->echo_chan attribute and directly unref
> the io_channel once the watcher is set.

Thanks. The patches have now been pushed upstream.

Johan

Subject: Re: Pull request git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream

2010/10/12 Jose Antonio Santos Cadenas <[email protected]>:
> 2010/10/12 Johan Hedberg <[email protected]>:
>> Hi Jose,
>>
>> On Tue, Oct 12, 2010, Jose Antonio Santos Cadenas wrote:
>>> I'm sorry but I can't connect to the IRC rigth now. Nevertheless, I
>>> think that I've found the issues:
>>>
>>> - The iochannel should be unrefereed when the watcher is explicitly removed
>>
>> Yes, though do you need to have a reference at all? As long as you have
>> the GLib watch callback GLib itself holds a reference, so if you don't
>> need one just remove chan->echo_chan and do an unref after calling
>> add_watch.
>
> Sorry I've forgot this one. Gonna solve it.

Solved, I've removed the chan->echo_chan attribute and directly unref
the io_channel once the watcher is set.

>
>>
>>> - The commit message s/paramter/parameter
>>>
>>> Are there any more?
>>
>> s/Imcoming/Incoming/
>> s/DBus/D-Bus/
>>
>> And the following doesn't make much sense to me:
>>
>> "Delete all channels DBus interface when the instance is removed"
>>
>> Could you rephrase it somehow, maybe by putting most of it in the
>> message body instead of the summary line. Is it trying to say "Delete
>> the D-Bus interfaces of all channels"? The summary line shouldn't be too
>> complex. You can just say "Fix D-Bus channel removal when removing
>> instances" and then in the message body do the more detailed
>> explanation.
>>
>> Johan
>>
>

Subject: Re: Pull request git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream

2010/10/12 Johan Hedberg <[email protected]>:
> Hi Jose,
>
> On Tue, Oct 12, 2010, Jose Antonio Santos Cadenas wrote:
>> I'm sorry but I can't connect to the IRC rigth now. Nevertheless, I
>> think that I've found the issues:
>>
>> - The iochannel should be unrefereed when the watcher is explicitly removed
>
> Yes, though do you need to have a reference at all? As long as you have
> the GLib watch callback GLib itself holds a reference, so if you don't
> need one just remove chan->echo_chan and do an unref after calling
> add_watch.

Sorry I've forgot this one. Gonna solve it.

>
>> - The commit message s/paramter/parameter
>>
>> Are there any more?
>
> s/Imcoming/Incoming/
> s/DBus/D-Bus/
>
> And the following doesn't make much sense to me:
>
> "Delete all channels DBus interface when the instance is removed"
>
> Could you rephrase it somehow, maybe by putting most of it in the
> message body instead of the summary line. Is it trying to say "Delete
> the D-Bus interfaces of all channels"? The summary line shouldn't be too
> complex. You can just say "Fix D-Bus channel removal when removing
> instances" and then in the message body do the more detailed
> explanation.
>
> Johan
>

Subject: Re: Pull request git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream

Hi,

2010/10/12 Johan Hedberg <[email protected]>:
> Hi Jose,
>
> On Tue, Oct 12, 2010, Jose Antonio Santos Cadenas wrote:
>> I'm sorry but I can't connect to the IRC rigth now. Nevertheless, I
>> think that I've found the issues:
>>
>> - The iochannel should be unrefereed when the watcher is explicitly removed
>
> Yes, though do you need to have a reference at all? As long as you have
> the GLib watch callback GLib itself holds a reference, so if you don't
> need one just remove chan->echo_chan and do an unref after calling
> add_watch.
>
>> - The commit message s/paramter/parameter
>>
>> Are there any more?
>
> s/Imcoming/Incoming/

Solved this one

> s/DBus/D-Bus/
>
> And the following doesn't make much sense to me:
>
> "Delete all channels DBus interface when the instance is removed"
>
> Could you rephrase it somehow, maybe by putting most of it in the
> message body instead of the summary line. Is it trying to say "Delete
> the D-Bus interfaces of all channels"? The summary line shouldn't be too
> complex. You can just say "Fix D-Bus channel removal when removing
> instances" and then in the message body do the more detailed
> explanation.

I've changed the commit message and added an explanation. I hope now is clearer.

>
> Johan
>

Regards.

Jose.

2010-10-12 09:35:31

by Johan Hedberg

[permalink] [raw]
Subject: Re: Pull request git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream

Hi Jose,

On Tue, Oct 12, 2010, Jose Antonio Santos Cadenas wrote:
> I'm sorry but I can't connect to the IRC rigth now. Nevertheless, I
> think that I've found the issues:
>
> - The iochannel should be unrefereed when the watcher is explicitly removed

Yes, though do you need to have a reference at all? As long as you have
the GLib watch callback GLib itself holds a reference, so if you don't
need one just remove chan->echo_chan and do an unref after calling
add_watch.

> - The commit message s/paramter/parameter
>
> Are there any more?

s/Imcoming/Incoming/
s/DBus/D-Bus/

And the following doesn't make much sense to me:

"Delete all channels DBus interface when the instance is removed"

Could you rephrase it somehow, maybe by putting most of it in the
message body instead of the summary line. Is it trying to say "Delete
the D-Bus interfaces of all channels"? The summary line shouldn't be too
complex. You can just say "Fix D-Bus channel removal when removing
instances" and then in the message body do the more detailed
explanation.

Johan

Subject: Re: Pull request git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream

Hi,

2010/10/12 Jose Antonio Santos Cadenas <[email protected]>:
> Hi,
>
> 2010/10/11 Johan Hedberg <[email protected]>:
>> Hi,
>>
>> On Mon, Oct 11, 2010, Jose Antonio Santos Cadenas wrote:
>>> The following changes since commit 646f0c7e6b557c5413825ce7b04bee52bf0129e8:
>>>
>>> ? Move remote name and version requests to hciops (2010-10-10 22:44:25 +0100)
>>>
>>> are available in the git repository at:
>>> ? git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream
>>>
>>> Jos? Antonio Santos Cadenas (6):
>>> ? ? ? Remove mdl_conn from struct hdp_channel
>>> ? ? ? Remove app paramter from hdp_establish_mcl, it is not needed
>>> ? ? ? Imcoming connection in control channel should be ERTM
>>> ? ? ? Fix multiple emission of main channel property signal.
>>> ? ? ? Return MCAP_MDL_BUSY when health channel can't be created
>>> ? ? ? Implement callback for responding echo petitions
>>>
>>> Santiago Carot-Nemesio (6):
>>> ? ? ? Add a new function to create channels
>>> ? ? ? Process request of of echo channels creation
>>> ? ? ? Correctly notify the deletion of the reliable data channel
>>> ? ? ? Add extra checks for avoid notifiying incoming echo channels
>>> ? ? ? Delete all channels DBus interface when the instance is removed
>>> ? ? ? Check first reliable configuration during channel creation
>>>
>>> ?health/hdp.c ? ? ? | ?249 +++++++++++++++++++++++++++++++++++----------------
>>> ?health/hdp_types.h | ? ?3 +-
>>> ?health/hdp_util.c ?| ? ?3 -
>>> ?health/hdp_util.h ?| ? ?1 -
>>> ?health/mcap.c ? ? ?| ? ?1 +
>>> ?5 files changed, 174 insertions(+), 83 deletions(-)
>>
>> Could you try to ping me on IRC when possible so we can solve a few
>> issues with these patches. It seems e.g. that you're doing GIOChannel
>> reference counting incorrectly in one place. There are also some english
>> language errors in the commit messages that would be good to fix (let's
>> see if you can spot them yourselves, otherwise I'll point them out when
>> we go over the other issues ;)
>
> I'm sorry but I can't connect to the IRC rigth now. Nevertheless, I
> think that I've found the issues:
>
> - The iochannel should be unrefereed when the watcher is explicitly removed
> - The commit message s/paramter/parameter
>
> Are there any more?
>
> I will solve them in half an hour.

I've rebased the branch with the changes that I described before. I've
also fixed an other issue related with the mdep checks, now a macro is
used to make the code more readable and easy to mantain.

Regards.

Jose.

Subject: Re: Pull request git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream

Hi,

2010/10/11 Johan Hedberg <[email protected]>:
> Hi,
>
> On Mon, Oct 11, 2010, Jose Antonio Santos Cadenas wrote:
>> The following changes since commit 646f0c7e6b557c5413825ce7b04bee52bf0129e8:
>>
>> ? Move remote name and version requests to hciops (2010-10-10 22:44:25 +0100)
>>
>> are available in the git repository at:
>> ? git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream
>>
>> Jos? Antonio Santos Cadenas (6):
>> ? ? ? Remove mdl_conn from struct hdp_channel
>> ? ? ? Remove app paramter from hdp_establish_mcl, it is not needed
>> ? ? ? Imcoming connection in control channel should be ERTM
>> ? ? ? Fix multiple emission of main channel property signal.
>> ? ? ? Return MCAP_MDL_BUSY when health channel can't be created
>> ? ? ? Implement callback for responding echo petitions
>>
>> Santiago Carot-Nemesio (6):
>> ? ? ? Add a new function to create channels
>> ? ? ? Process request of of echo channels creation
>> ? ? ? Correctly notify the deletion of the reliable data channel
>> ? ? ? Add extra checks for avoid notifiying incoming echo channels
>> ? ? ? Delete all channels DBus interface when the instance is removed
>> ? ? ? Check first reliable configuration during channel creation
>>
>> ?health/hdp.c ? ? ? | ?249 +++++++++++++++++++++++++++++++++++----------------
>> ?health/hdp_types.h | ? ?3 +-
>> ?health/hdp_util.c ?| ? ?3 -
>> ?health/hdp_util.h ?| ? ?1 -
>> ?health/mcap.c ? ? ?| ? ?1 +
>> ?5 files changed, 174 insertions(+), 83 deletions(-)
>
> Could you try to ping me on IRC when possible so we can solve a few
> issues with these patches. It seems e.g. that you're doing GIOChannel
> reference counting incorrectly in one place. There are also some english
> language errors in the commit messages that would be good to fix (let's
> see if you can spot them yourselves, otherwise I'll point them out when
> we go over the other issues ;)

I'm sorry but I can't connect to the IRC rigth now. Nevertheless, I
think that I've found the issues:

- The iochannel should be unrefereed when the watcher is explicitly removed
- The commit message s/paramter/parameter

Are there any more?

I will solve them in half an hour.

>
> Johan
>

Jose.

2010-10-11 21:31:54

by Johan Hedberg

[permalink] [raw]
Subject: Re: Pull request git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream

Hi,

On Mon, Oct 11, 2010, Jose Antonio Santos Cadenas wrote:
> The following changes since commit 646f0c7e6b557c5413825ce7b04bee52bf0129e8:
>
> Move remote name and version requests to hciops (2010-10-10 22:44:25 +0100)
>
> are available in the git repository at:
> git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream
>
> Jos? Antonio Santos Cadenas (6):
> Remove mdl_conn from struct hdp_channel
> Remove app paramter from hdp_establish_mcl, it is not needed
> Imcoming connection in control channel should be ERTM
> Fix multiple emission of main channel property signal.
> Return MCAP_MDL_BUSY when health channel can't be created
> Implement callback for responding echo petitions
>
> Santiago Carot-Nemesio (6):
> Add a new function to create channels
> Process request of of echo channels creation
> Correctly notify the deletion of the reliable data channel
> Add extra checks for avoid notifiying incoming echo channels
> Delete all channels DBus interface when the instance is removed
> Check first reliable configuration during channel creation
>
> health/hdp.c | 249 +++++++++++++++++++++++++++++++++++----------------
> health/hdp_types.h | 3 +-
> health/hdp_util.c | 3 -
> health/hdp_util.h | 1 -
> health/mcap.c | 1 +
> 5 files changed, 174 insertions(+), 83 deletions(-)

Could you try to ping me on IRC when possible so we can solve a few
issues with these patches. It seems e.g. that you're doing GIOChannel
reference counting incorrectly in one place. There are also some english
language errors in the commit messages that would be good to fix (let's
see if you can spot them yourselves, otherwise I'll point them out when
we go over the other issues ;)

Johan

2010-11-18 15:03:51

by Johan Hedberg

[permalink] [raw]
Subject: Re: Pull request git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream

Hi,

On Thu, Nov 18, 2010, Santiago Carot-Nemesio wrote:
> Fix typos in adapter documentation (2010-11-16 13:39:43 +0000)
>
> are available in the git repository at:
> git://gitorious.org/bluez-mcap-hdp/mcap-hdp.git for_upstream
>
> Jos? Antonio Santos Cadenas (9):
> Add reference counter to hdp_application
> Add reference counter to hdp_channel
> Notify a channel deleted when remote side has already deleted it
> Fix typos in mcap
> Set reference counter for mcls to gint
> Fix message error
> Remove magic number to check ECHO MDEPs in HDP
> Fixes to echo
> Check if the mcl insiede the device is correctly set before use it
>
> Santiago Carot-Nemesio (10):
> Add reference counter to hdp_device
> Remove MCL's before removing the application
> Add missed unrefs for hdp_tmp_dc_data
> Add reference counter to mcap_instances
> Add reference counter to mcap_mdl
> Remove old reference to mcap session in the name of variables
> Code refactorization in MCAP
> Fix dereference to NULL pointers during data channels creation
> Return proper response code if there is an error creating echo channel
> Check if MCAP Instance is already released when a callbacks comes back.
>
> health/hdp.c | 266 ++++++++++++++++++++++++++++++++----------------
> health/hdp_types.h | 4 +
> health/hdp_util.c | 55 ++++++++++-
> health/hdp_util.h | 8 ++
> health/mcap.c | 242 +++++++++++++++++++++++++------------------
> health/mcap_internal.h | 9 +-
> health/mcap_lib.h | 13 ++-
> health/mcap_sync.c | 22 ++--
> 8 files changed, 407 insertions(+), 212 deletions(-)

In general, please try to avoid such big sets of patches and send them
in smaller bits (as they get created) to linux-bluetooth. That makes
things easier for me and I am able to do a more thorough review.
However, I did a quick skim through of the patches and only found one
coding style issue (C++ style comment) which I fixed my self. So the
patches have now all been pushed upstream.

Johan