_______________________________________________
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 Pieter,
> > And for the core part, please be brief with any additional comments. My
> > rule for comments is to put them where the code is not clear by itself.
> > What you have done is actually over-commenting and that makes the code
> > harder to read and understand.
> >
> I've read the code and for me the comments are very welcome. As well as
> your enthousiasm & code, Fabian.
the right amount of comments and the coding style has always been
something people disagree on. However the kernel follows the policy to
have clean and easy code. You should understand the code and there
should be no need for comments. The comments are used to explain parts
that are not easy to guess.
Example for these are the reasons why to put a memory barrier here or
why not using some kind of locking or the reason why this isn't a race
conditions even if it looks like one etc.
Regards
Marcel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Marcel Holtmann wrote:
> And for the core part, please be brief with any additional comments. My
> rule for comments is to put them where the code is not clear by itself.
> What you have done is actually over-commenting and that makes the code
> harder to read and understand.
>
>
I've read the code and for me the comments are very welcome. As well as
your enthousiasm & code, Fabian.
Cheers, Pieter
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Fabien,
> I just finished a patch to bluez that implements flow control for SCO.
please split the changes to Bluetooth core and SCO module. Your changes
to the core will conflict with the automatic sniff mode support and I
need to sort that out.
And for the core part, please be brief with any additional comments. My
rule for comments is to put them where the code is not clear by itself.
What you have done is actually over-commenting and that makes the code
harder to read and understand.
Regards
Marcel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Fabien,
> A just thought to a few more things related to a sco socket.
> I just though to a real advantage of using socket for SCO (vs ALSA
> device or character device).
> Using a character or block device usually means that the device is
> connected. You can plug your USB mass storage device, and the /dev/sda
> will be usefull.
> Sockets are usually used to connect to devices that the OS does not know
> if they exist or not (think to a distant IP host: the kernel has no way
> of knowing if it is valid are not. The connect call will just ask the
> kernel to 'give a try', and bail out if the device does not exist).
> Having said that, this is really quite similar to the bluetooth
> technology, where before you have tryed to connect, you don't know if
> the device is on or off...)
lets stay with the sockets for now.
> And for kfifo use: it make sense, however the bad point is that all
> bluez stack is coded with sk_buffs all over the place. Using kfifo
> today will mean having to do the following convertion:
>
> (hci core) kfifo --> (hci device) sk_buff
>
> I think for now i'm gonna stick with sk_buffs :-)
> However i will have a look to see how to queue sk_buffs at hci core
> layer instead of socket layer :-)
Sounds like a good plan.
Regards
Marcel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Fabien,
> > We might leave the socket for now. The alternative is a character device
> > or directly an ALSA sound card. Haven't fully thought about it either.
>
> For the ALSA sound card, this is basically what snd-bt-sco does. The
> issue with this alternative, is that the headset&handsfree profile
> mandates the use of an RFCOMM channel before to use SCO. Doing this at
> kernel level will mean that SCO layer will be dependant on RFCOMM
> sockets, or that user space will have to handle RFCOMM channel before to
> open ALSA device. This is roughly the way btsco works today. The result
> is not very pretty :-(
except that we don't would use the SCO socket interface in the kernel
like the current snd-bt-sco is doing. And yes, we would need an extra
configuration channel, because using AT commands the kernel is not an
option at all.
Regards
Marcel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Marcel,
A just thought to a few more things related to a sco socket.
I just though to a real advantage of using socket for SCO (vs ALSA
device or character device).
Using a character or block device usually means that the device is
connected. You can plug your USB mass storage device, and the /dev/sda
will be usefull.
Sockets are usually used to connect to devices that the OS does not know
if they exist or not (think to a distant IP host: the kernel has no way
of knowing if it is valid are not. The connect call will just ask the
kernel to 'give a try', and bail out if the device does not exist).
Having said that, this is really quite similar to the bluetooth
technology, where before you have tryed to connect, you don't know if
the device is on or off...)
And for kfifo use: it make sense, however the bad point is that all
bluez stack is coded with sk_buffs all over the place. Using kfifo
today will mean having to do the following convertion:
(hci core) kfifo --> (hci device) sk_buff
I think for now i'm gonna stick with sk_buffs :-)
However i will have a look to see how to queue sk_buffs at hci core
layer instead of socket layer :-)
Cheers,
Fabien
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Marcel,
>> I'm not against this, even if it means more work...
>
> I think it will be worth it.
>
>> However i don't have any precise idea on which communication mechanism
>> to use between userspace/kernelspace. Any ideas ??
>
> We might leave the socket for now. The alternative is a character device
> or directly an ALSA sound card. Haven't fully thought about it either.
For the ALSA sound card, this is basically what snd-bt-sco does. The
issue with this alternative, is that the headset&handsfree profile
mandates the use of an RFCOMM channel before to use SCO. Doing this at
kernel level will mean that SCO layer will be dependant on RFCOMM
sockets, or that user space will have to handle RFCOMM channel before to
open ALSA device. This is roughly the way btsco works today. The result
is not very pretty :-(
>
>>> Let's have another patch on top of 2.6.16-mh1 and maybe you also wanna
>>> adapt scotest for testing it.
>>>
>> Ok, i'm gonna work on that then...
>
> The 2.6.16-mh2 is missing the automatic sniff mode patch, but only for
> testing. It is coming back for sure.
Ok, i will stick with mh1 then....
Cheers,
Fabien
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Fabien,
> >>> Another question is if we really should queue the data in sk_write_queue
> >>> and not directly in the core. I would prefer to send the data to the
> >>> core and then stream it from there.
> >> That's an interesting question... :-)
> >> I asked it myself when i started to design this feature.
> >> In fact having outgoing packets stored in a socket queue has some
> >> advantages:
> >> * it makes use of the BSD socket send queue, and thus can reuse some
> >> generic socket queue management related functions, which rely on some
> >> well known sk_buff variable (sk_sndbuff, sk_wmem_alloc, sk_rcvbuff,
> >> sk_rmem_alloc ...)
> >> * it makes the thing symetrical, both uplink and downlink path make
> >> use socket queues to buffer data.
> >>
> >> As such I thought there were not any valid reason to put in the core...
> >> however if you have suggestions i would be very happy to hear them. :-)
> >
> > Actually I never thought about SCO as socket is the best interface. So
> > we might wanna move away from it sometime in the future. And in this
> > case it would be great if it uses simple SKB queues or maybe kfifo for
> > it. Even if this sounds like more work right now, I might be worth it.
>
> It might make sense: however we must take into account the following
> constraints:
> - we must be aple to setup fifo lengths from userspace, so that to
> simulate a variable size ring buffer as found on sound card hardware.
> - we must be able to put the sending task to sleep, and wake it up
> later, when the fifo gets 'full'.
I don't see any problems with both. We can use workqueues or tasklets
for the second one.
> I'm not against this, even if it means more work...
I think it will be worth it.
> However i don't have any precise idea on which communication mechanism
> to use between userspace/kernelspace. Any ideas ??
We might leave the socket for now. The alternative is a character device
or directly an ALSA sound card. Haven't fully thought about it either.
> > Let's have another patch on top of 2.6.16-mh1 and maybe you also wanna
> > adapt scotest for testing it.
> >
> Ok, i'm gonna work on that then...
The 2.6.16-mh2 is missing the automatic sniff mode patch, but only for
testing. It is coming back for sure.
Regards
Marcel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Marcel,
>>> Another question is if we really should queue the data in sk_write_queue
>>> and not directly in the core. I would prefer to send the data to the
>>> core and then stream it from there.
>> That's an interesting question... :-)
>> I asked it myself when i started to design this feature.
>> In fact having outgoing packets stored in a socket queue has some
>> advantages:
>> * it makes use of the BSD socket send queue, and thus can reuse some
>> generic socket queue management related functions, which rely on some
>> well known sk_buff variable (sk_sndbuff, sk_wmem_alloc, sk_rcvbuff,
>> sk_rmem_alloc ...)
>> * it makes the thing symetrical, both uplink and downlink path make
>> use socket queues to buffer data.
>>
>> As such I thought there were not any valid reason to put in the core...
>> however if you have suggestions i would be very happy to hear them. :-)
>
> Actually I never thought about SCO as socket is the best interface. So
> we might wanna move away from it sometime in the future. And in this
> case it would be great if it uses simple SKB queues or maybe kfifo for
> it. Even if this sounds like more work right now, I might be worth it.
It might make sense: however we must take into account the following
constraints:
- we must be aple to setup fifo lengths from userspace, so that to
simulate a variable size ring buffer as found on sound card hardware.
- we must be able to put the sending task to sleep, and wake it up
later, when the fifo gets 'full'.
I'm not against this, even if it means more work...
However i don't have any precise idea on which communication mechanism
to use between userspace/kernelspace. Any ideas ??
>
> Let's have another patch on top of 2.6.16-mh1 and maybe you also wanna
> adapt scotest for testing it.
>
Ok, i'm gonna work on that then...
Cheers,
Fabien
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Fabien,
> Sorry to be a little late on this answer, however a busy work schedule
> and a sunny Sunday prevented me to answer my e-mails for a few days :-)
no big deal. I should do this more often by myself ;)
> > Can we change the core and add a hci_stream_sco() without breaking the
> > API. This would make it easier for me to apply the changes.
>
> I see your point, however i don't see how to bring this feature in
> without breaking the hci core API for SCO sockets... :-(
No problem. We can break the API, but at least I need some good reason
for it.
> > Another question is if we really should queue the data in sk_write_queue
> > and not directly in the core. I would prefer to send the data to the
> > core and then stream it from there.
>
> That's an interesting question... :-)
> I asked it myself when i started to design this feature.
> In fact having outgoing packets stored in a socket queue has some
> advantages:
> * it makes use of the BSD socket send queue, and thus can reuse some
> generic socket queue management related functions, which rely on some
> well known sk_buff variable (sk_sndbuff, sk_wmem_alloc, sk_rcvbuff,
> sk_rmem_alloc ...)
> * it makes the thing symetrical, both uplink and downlink path make
> use socket queues to buffer data.
>
> As such I thought there were not any valid reason to put in the core...
> however if you have suggestions i would be very happy to hear them. :-)
Actually I never thought about SCO as socket is the best interface. So
we might wanna move away from it sometime in the future. And in this
case it would be great if it uses simple SKB queues or maybe kfifo for
it. Even if this sounds like more work right now, I might be worth it.
> For now i would suggest we do the following:
> - for you: have a look at the "whole picture", including the SCO part
> patch. This way you would be able to better understand the interaction
> between SCO sockets & hci layer. This way you will be able to send in
> other comments too. :-)
> - for me: * port the patch on top of the 2.6.16-mh1 :-)
> * have it to work also when HZ % 333 != 0 ( i've just seen
> it wouldn't work properly in this case... )
Let's have another patch on top of 2.6.16-mh1 and maybe you also wanna
adapt scotest for testing it.
Regards
Marcel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Marcel,
Sorry to be a little late on this answer, however a busy work schedule
and a sunny Sunday prevented me to answer my e-mails for a few days :-)
> so far I only looked at the changes to the HCI core. Please don't rename
> the disconnect timer. This will collide with the changes for automatic
> sniff mode support.
Ok, however as now sniff mode is out, i would suggest i build on top of
it :-)
>
> Can we change the core and add a hci_stream_sco() without breaking the
> API. This would make it easier for me to apply the changes.
I see your point, however i don't see how to bring this feature in
without breaking the hci core API for SCO sockets... :-(
>
> Another question is if we really should queue the data in sk_write_queue
> and not directly in the core. I would prefer to send the data to the
> core and then stream it from there.
That's an interesting question... :-)
I asked it myself when i started to design this feature.
In fact having outgoing packets stored in a socket queue has some
advantages:
* it makes use of the BSD socket send queue, and thus can reuse some
generic socket queue management related functions, which rely on some
well known sk_buff variable (sk_sndbuff, sk_wmem_alloc, sk_rcvbuff,
sk_rmem_alloc ...)
* it makes the thing symetrical, both uplink and downlink path make
use socket queues to buffer data.
As such I thought there were not any valid reason to put in the core...
however if you have suggestions i would be very happy to hear them. :-)
For now i would suggest we do the following:
- for you: have a look at the "whole picture", including the SCO part
patch. This way you would be able to better understand the interaction
between SCO sockets & hci layer. This way you will be able to send in
other comments too. :-)
- for me: * port the patch on top of the 2.6.16-mh1 :-)
* have it to work also when HZ % 333 != 0 ( i've just seen
it wouldn't work properly in this case... )
What do you think of the idea ?
Cheers,
Fabien
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel
Hi Fabien,
> > please split the changes to Bluetooth core and SCO module. Your changes
> > to the core will conflict with the automatic sniff mode support and I
> > need to sort that out.
>
> Ok, i've just done that, i've split the patch into three smaller bits.
> - Patch part 1 are changes to hci core required for SCO flow control
> itself.
> - Patch part 2 are the changes to the SCO socket layer.
> - Patch part 3 are additionnal comments on hci core code.
>
> Please note however that part 1 and part 2 can not be applied seperately.
so far I only looked at the changes to the HCI core. Please don't rename
the disconnect timer. This will collide with the changes for automatic
sniff mode support.
Can we change the core and add a hci_stream_sco() without breaking the
API. This would make it easier for me to apply the changes.
Another question is if we really should queue the data in sk_write_queue
and not directly in the core. I would prefer to send the data to the
core and then stream it from there.
Regards
Marcel
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel