_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Guillaume,
>> Marcel looked at this patch and came back with the comments below.
>> Can
>> you revisit it? I think some other people are seeing the same issues.
>> The patch won't go upstream until Marcel likes it.
>>
>> the patch you sent me is fully broken. First of all the coding style
>> is wrong. Does nobody have learned this by now? I always look for
>> that
>> first before even reading the patch. Second the case where an
>> ESCO_LINK returns NULL is broken and will fall over and crash.
>>
> It was about the patch on the wiki (one of mine, shame on me).
>
> Marcel is right, this patch is an awful and moreover buggy patch
> (and I
> found nothing in the specification stating a SCO connection shall be
> opened as a fallback for an eSCO one, or shall not, so I abandonned
> that
> way).
>
> However, I think the first patch
> (force-sco-link-until-headset-features-are-known.patch), despite badly
> styled and named, was correct.
No. That is wrong. Using sync setup commands apply to SCO and eSCO. No
need to force anything here. Let the Bluetooth firmware inside the
chip do the right thing.
> Everywhere I found a eSCO feature test for the local device, I added a
> test for the remote device eSCO/EV3 feature.
>
> An eSCO connection is then used only when both the local and the
> remote
> devices supports the feature, else a SCO connection is requested with
> headsets not supporting eSCO.
As I said, wrong approach. Don't put that logic into the host stack.
Let the firmware negotiate that.
> The headset's features mask should be already available, as the
> request
> is sent just after establishing the underlying ACL connection.
>
> And it shall be known before establishing a synchronous connection
> with
> the right packet type(s), according to the spec (maybe we should
> ensure
> we get a response to this features request ?).
>
> I tried to fix style issues in this new patch, let me now if some
> remain.
>
> The other patch (also-accept-sco-links-v2.patch) is not needed at all,
> as the case should not occur anymore with the attached patch applied.
Wrong. This case can still happen. Nobody says that the remote device
has to establish an eSCO link only because the link manager feature
says that the chip supports. In short it means that if the host stack
on the remote doesn't want eSCO that is perfectly fine and valid.
Regards
Marcel
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Louis
I should clarify. It looks like the one Marcel saw was:
- if (!conn)
- goto unlock;
+ if (!conn) {
+ if (ev->link_type==SCO_LINK) {
+ conn = hci_conn_hash_lookup_ba(hdev, ESCO_LINK, &ev->bdaddr);
+ if (!conn)
+ goto unlock;
+ else
+ conn->type=SCO_LINK;
+ }
+ }
but yours was
- if (!conn)
- goto unlock;
+ if (!conn) {
+ if (ev->link_type != ACL_LINK) {
+ int t = (ev->link_type == ESCO_LINK) ? SCO_LINK : ESCO_LINK;
+ conn = hci_conn_hash_lookup_ba(hdev, t, &ev->bdaddr);
+ if (conn)
+ conn->type = ev->link_type;
+ }
+ if (!conn)
+ goto unlock;
+ }
they both have indent/space issues but I'm not sure Marcel's other
comment makes sense for what you did.
It seems like the problem is ev->link_type might have the wrong value
for the lookup to succeed, but I'm not sure what the big picture looks
like here. Is this the best place to solve it or was this a quick
workaround?
Brad
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Louis
> When I tried to connect SCO channel with
> HCI_OP_SETUP_SYNC_CONN(LINK_TYPE_ESCO), some bluetooth headsets responds
> with LINK_TYPE_SCO because it did not support ESCO. But bluez couldn't
> handle this situation, and patch_hci_event.c is for this.
Marcel looked at this patch and came back with the comments below. Can
you revisit it? I think some other people are seeing the same issues.
The patch won't go upstream until Marcel likes it.
the patch you sent me is fully broken. First of all the coding style
is wrong. Does nobody have learned this by now? I always look for that
first before even reading the patch. Second the case where an
ESCO_LINK returns NULL is broken and will fall over and crash.
--
Brad
-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel