RE: libhci update
Iain Hibbert
> thanks for the feedback. i'm attaching revisited patch. please take a
> look and let me know if this is something you happy with.
(pls ignore typos & bad formatting, am on mobile device :)
Bt_devrecv() should probably take void * for buffer? Also manpage suggests it will only receive hci events. Do you think its worth doing some validation of received data? (eg return EIO for truncated reads?)
Bt_devinquiry() should probably restore the filter after use? Also needs to ensure hci event packets are enabled?
Bt_devreq() needs to set/restore a filter too
In bt_devreq structure event should be uint8_t? Also clen and rlen should be size_t
Do you think its worth to cook dev_class into a normalised host numeric value rather than 3 bytes ?
Probably need to specifically mention that the inquiry response to be released using free() in manpage?
Something i thought about on the train yesterday but can't visualise on the small screen. For device inquiry did you consider using a callback method (as per devenum) in stead of returning the structure array? At least i recall my nokia would show the list building (but perhaps that was when it got the names) and this windows mobile device does show the raw list in progress (though stupidly just displays a list of 'unknown device' until it gets the names :)
Regards,
iain
Iain Hibbert
> >> thanks for the feedback. i'm attaching revisited patch. please take a
> >> look and let me know if this is something you happy with.
> >
> > (pls ignore typos & bad formatting, am on mobile device :)
> >
> > Bt_devrecv() should probably take void * for buffer? Also manpage suggests it
> > will only receive hci events. Do you think its worth doing some validation of received
> > data? (eg return EIO for truncated reads?)
>
> i will change uint8_t * to void *, no problem. i also updated man page
> and removed 'event' word :)
Ok also in bt_devrecv()
- return type should be ssize_t
- size cannot be < 0 (use "size == 0")?
and what of validation of received data here? It could be worth checking
that the buffer is not truncated so that the caller does not have to worry
about it, but that does require checking the packet type..
> > Bt_devreq() needs to set/restore a filter too
>
> well, maybe. bt_devreq() operates on already opened socket. the
> assumption i'm making here is that application will set appropriate
> filter before calling bt_devreq(). otherwise, application would have
> to always set 'event' field to acceptable value (or zero). i could go
> either way here. just need to document implemented behavior better.
Mm, its a good point - there are arguments either way (bloat vs guaranteed
success) but I think since the difference between devreq() and devrecv()
is that devreq() handles all the fiddly details for you, I think its worth
doing that aswell..
> > Do you think its worth to cook dev_class into a normalised host
> > numeric value rather than 3 bytes ?
>
> right, hmm, i guess we could turn dev_class into uint32_t in le16 byte
> order (since everything else in bluetooth hci is in le16 order), but
> maybe its too much? spec breaks out dev_class as 3 bytes array and talks
> about bytes and bits within each byte. i'm kinda leaning towards leaving
> it as is, because otherwise application would have to translate byte/bit
> into uint32_t mask. it could be somewhat convenient, i guess. again, no
> strong feeling about it. could go either way.
My thought was that some of the 'fields' do seem to cross over into
multiple bytes (eg the service class flags in the default format type) and
perhaps its easier to interpret as a single value where bits can be tested
in host order by such as (class & (1 << N))? I don't know how much that
helps but the assigned numbers document does seem to show it as a bit
array. (I'm not having a strong opinion either :)
> > Probably need to specifically mention that the inquiry response to be
> > released using free() in manpage?
>
> it says so, i.e.
>
> The
> .Fa ii
> parameter specifies where to place inquiry results.
> On success, the function will return total number of inquiry results,
> will allocate buffer to store all the inquiry results and
> will return pointer to the allocated buffer in the
> .Fa ii
> parameter.
> It is up to the caller of the function to dispose of the buffer.
No, I mean to specifically mention ".Xr free 3" as the method used to
dispose of the buffer.
> > Something i thought about on the train yesterday but can't visualise
> > on the small screen. For device inquiry did you consider using a
> > callback method (as per devenum) in stead of returning the structure
> > array? At least i recall my nokia would show the list building (but
> > perhaps that was when it got the names) and this windows mobile device
> > does show the raw list in progress (though stupidly just displays a
> > list of 'unknown device' until it gets the names :)
>
> yes, i thought about it. the only difference is that it would be not so
> close to linux/bluez api. i guess, we could provide another function?
I thought about that some yesterday (on another train :) and I think there
could be some other problems in any case, eg some devices (at least, maybe
all?) can not handle RemoteNameReq etc while in inquiry mode so the amount
of things that can be done is minimal - btconfig(8) does print a dot each
time a result is returned but I think thats probably about the limit of
what is useful. So, perhaps just leave it for now.. if somebody requires
something like that its not difficult to cut and paste library code to
work something out.
iain
Iain Hibbert
> > > Bt_devreq() needs to set/restore a filter too
> >
> > well, maybe. bt_devreq() operates on already opened socket. the
> > assumption i'm making here is that application will set appropriate
> > filter before calling bt_devreq(). otherwise, application would have
> > to always set 'event' field to acceptable value (or zero). i could go
> > either way here. just need to document implemented behavior better.
>
> Mm, its a good point - there are arguments either way (bloat vs guaranteed
> success) but I think since the difference between devreq() and devrecv()
> is that devreq() handles all the fiddly details for you, I think its worth
> doing that aswell..
the bluez hci_send_req() does set the filters btw
iain
Iain Hibbert
> the revised diff is attached. please review.
Its all looking good, though some small tuneups still occurring :)
in bt_devsend() you might also want to check for "plen > UINT8_MAX"?
for bt_devrecv() should there also be a method for not timing out? I can
see that being used to wait for any packet which could be some time.
(bt_devreq() does not need that I think?)
In bt_devreq() you used buf[320] is that truly enough? I didn't check that
part of the radio specification dedicated to baseband packet size but the
Broadcom BCM2045B device in my laptop claims to have max_acl_size of 1017..
Also, I think rlen and rparam can be empty in bt_devreq(), is that
intentional and what does it mean?
and finally some language improvements in the manpage below
iain
+.Fn bt_devopen
+function opens Bluetooth device with the given
^a
+.Fa devname
+and returns connected and bound
^a
+.Dv HCI
+socket.
^handle
+The function returns -1 if an error has occurred.
+.Pp
+The
+.Fn bt_devclose
+closes passed
^the
+.Dv HCI
+socket
^handle
+.Fa s ,
+previously obtained with
+.Xr bt_devopen 3 .
+.Pp
+The
+.Fn bt_devsend
+function sends Bluetooth
^a
+.Dv HCI
+command with the given
+.Fa opcode
+to the provided socket
+.Fa s ,
+previously obtained with
+.Xr bt_devopen 3 .
+The
+.Fa opcode
+parameter is exppected to be in the host byte order.
+The
+.Fa param
+and
+.Fa plen
+parameters specify command parameters.
+The
+.Fn bt_devsend
+function does not modify
^the
+.Dv HCI
+filter on the provided socket
+.Fa s .
+The function returns 0 on success,
+or -1 if an error occurred.
+.Pp
+The
+.Fn bt_devrecv
+function receives one Bluetooth
+.Dv HCI
+packet from the socket
+.Fa s ,
+previously obtained with
+.Xr bt_devopen 3 .
+The packet is placed into the provided buffer
+.Fa buf
+of size
+.Fa size .
+The
+.Fa to
+parameter specifies receive timeout in seconds.
+The
+.Fn bt_devrecv
+function does not modify
^the
+.Dv HCI
+filter on the provided socket
+.Fa s .
+The function returns total number of bytes recevied,
+or -1 if an error occurred.
+.Pp
+The
+.Fn bt_devreq
+function makes Bluetooth
^a
+.Dv HCI
+request to the socket
+.Fa s ,
+previously obtained with
+.Xr bt_devopen 3 .
+The function will send the specified command and will wait for the specified
+event,
+or timeout
+.Fa to
+seconds to occur.
+The
+.Vt bt_devreq
+structure is defined as follows
+.Bd -literal -offset indent
+struct bt_devreq
+{
+ uint16_t opcode;
+ uint8_t event;
+ void *cparam;
+ size_t clen;
+ void *rparam;
+ size_t rlen;
+};
+.Ed
+.Pp
+The
+.Fa opcode
+field specifies the command and is expected to be in the host byte order.
+The
+.Fa cparam
+and
+.Fa clen
+fields specify command parameters data and command parameters data size
+respectively.
+The
+.Fa event
+field specifies which Bluetooth
+.Dv HCI
+event ID the function should wait for, otherwise it should be set to zero.
+The
+.Dv HCI
+Command Complete and Command Status events are enabled by default.
+The
+.Fa rparam
+and
+.Fa rlen
+parameters specify buffer and buffer size respectively where return
+parameters should be placed.
+The
+.Fn bt_devreq
+function temporarily modifies filter on the provided
+.Dv HCI
+socket
+.Fa s .
+The function returns 0 on success, or -1 if an error occurred.
+.Pp
+The
+.Fn bt_devfilter
+controls the local
+.Dv HCI
+filter associated with the socket
+.Fa s ,
+previously obtained with
+.Xr bt_devopen 3 .
+Filtering can be done on packet types, i.e.
+.Dv ACL ,
+.Dv SCO or
+.Dv HCI
+event packets, and, in addition, on
^command and
+.Dv HCI
+event IDs.
+Before applying
^the
+.Fa new
+filter (if provided) the function will try to obtain current filter
^the
+from the socket
+.Fa s
+and place it into the
+.Fa old
+parameter (if provided).
+The function returns 0 on success, or -1 if an error occurred.
+.Pp
+The
+.Fn bt_devfilter_pkt_set ,
+.Fn bt_devfilter_pkt_clr
+and
+.Fn bt_devfilter_pkt_tst
+functions can be used to modify and test
^the
+.Dv HCI
+filter
+.Fa filter .
+The
+.Fa type
+parameter specifies
+.Dv HCI
+packet type.
+.Pp
+The
+.Fn bt_devfilter_evt_set ,
+.Fn bt_devfilter_evt_clr
+and
+.Fn bt_devfilter_evt_tst
+functions can be used to modify and test
^the
+.Dv HCI
+event filter
+.Fa filter .
+The
+.Fa event
+parameter specifies
+.Dv HCI
+event ID.
+.Pp
+The
+.Fn bt_devinquiry
+function performs Bluetooth inquiry.
+The
+.Fa devname
+parameter specifies which local Bluetooth device should perform an inquiry.
+If not secified, i.e.
+.Dv NULL ,
+then first available device will be used.
+The
+.Fa length
+parameters specifies the total length of an inquiry in seconds.
+If not specified, i.e. 0, default value will be used.
+The
+.Fa num_rsp
+parameter specifies the number of responses that can be received before
+the inquiry is halted.
+If not specified, i.e. 0, default value will be used.
+The
+.Fa ii
+parameter specifies where to place inquiry results.
+On success, the function will return total number of inquiry results,
+will allocate,
+using
+.Xr calloc 3 ,
+buffer to store all the inquiry results and
+will return pointer to the allocated buffer in the
+.Fa ii
+parameter.
+It is up to the caller of the function to dispose of the buffer using
+.Xr free 3
+call.
+The function returns -1 if an error has occurred.
+The
+.Vt bt_devinquiry
+structure is defined as follows
+.Bd -literal -offset indent
+struct bt_devinquiry {
+ bdaddr_t bdaddr;
+ uint8_t pscan_rep_mode;
+ uint8_t pscan_period_mode;
+ uint8_t dev_class[3];
+ uint16_t clock_offset;
+ int8_t rssi;
+ uint8_t data[240];
+};
+.Ed
+.Pp
+The
.Fn bdaddr_same ,
.Fn bdaddr_any
and
@@ -444,6 +718,6 @@
.Sh AUTHORS
.An Maksim Yevmenkin Aq m_evm...@yahoo.com
.Sh BUGS
-These functions use static data storage;
+Some of those functions use static data storage;
if the data is needed for future use, it should be
copied before any subsequent calls overwrite it.
Index: bluetooth.h
Iain Hibbert
> > In bt_devreq() you used buf[320] is that truly enough? I didn't check that
> > part of the radio specification dedicated to baseband packet size but the
> > Broadcom BCM2045B device in my laptop claims to have max_acl_size of 1017..
>
> yes, but we are not dealing with acl packets in bt_devreq() only
> commands and event that have up to 255 bytes of payload.
Doh
> please see attached revised patch.
I've got no more comments here
iain
Maksim Yevmenkin
>> please see attached revised patch.
>
> I've got no more comments here
thanks for the review. i committed it to -head.
thanks,
max
Iain Hibbert
> > I've got no more comments here
>
> thanks for the review. i committed it to -head.
Hi Max, I'm in progress of translating for NetBSD and in bt_devinquiry() I
see
if (ii == NULL) {
errno = EINVAL;
return (-1);
}
but I think this test might be bogus? manpage implies that the caller
provides a buffer but we allocate one..
/* Calculate inquire length in 1.28 second units */
to = (time_t) ((double) length / 1.28);
if (to <= 0)
cp->inquiry_length = 4; /* 5.12 seconds */
else if (to > 254)
cp->inquiry_length = 255; /* 326.40 seconds */
else
cp->inquiry_length = to + 1;
2.1 spec says 1.28 -> 61.44 seconds range is acceptable (0x01->0x30)
Then, to avoid the floating point arithmetic, can use (to * 100 / 128) but
would need to be range checked first. I'm inclined to use something like
if (to == 0)
to = 4;
else if (to == 1)
to = 2;
else if (to > 61)
to = 61;
cp->inquiry_length = (uint8_t)(to * 100 / 128);
also, I'm not sure that the timeout is handled right; the bt_devrecv()
uses the complete timeout each time but the time endpoint might need to be
calculated so that time-to-end can be used?
Also I'm wondering what to do in the case bt_devrecv() does actually time
out - what can it mean and should we [attempt to] cancel the inquiry?
(actually the whole timeout thing is probably irrelevant for a properly
functioning device :)
iain
Maksim Yevmenkin
>> thanks for the review. i committed it to -head.
>
> Hi Max, I'm in progress of translating for NetBSD and in bt_devinquiry() I
> see
>
> if (ii == NULL) {
> errno = EINVAL;
> return (-1);
> }
>
> but I think this test might be bogus? manpage implies that the caller
> provides a buffer but we allocate one..
no, the test is correct. yes, we allocate buffer internally, but we
need to return pointer to the buffer back to the caller. so, ii is
basically ii is struct bt_devinquiry **. perhaps language in the man
page is not clear enough?
> /* Calculate inquire length in 1.28 second units */
> to = (time_t) ((double) length / 1.28);
>
> if (to <= 0)
> cp->inquiry_length = 4; /* 5.12 seconds */
> else if (to > 254)
> cp->inquiry_length = 255; /* 326.40 seconds */
> else
> cp->inquiry_length = to + 1;
>
> 2.1 spec says 1.28 -> 61.44 seconds range is acceptable (0x01->0x30)
>
> Then, to avoid the floating point arithmetic, can use (to * 100 / 128) but
> would need to be range checked first. I'm inclined to use something like
>
> if (to == 0)
> to = 4;
> else if (to == 1)
> to = 2;
> else if (to > 61)
> to = 61;
>
> cp->inquiry_length = (uint8_t)(to * 100 / 128);
i have no problem with it.
> also, I'm not sure that the timeout is handled right; the bt_devrecv()
> uses the complete timeout each time but the time endpoint might need to be
> calculated so that time-to-end can be used?
well, yes. bt_devrecv() was envisioned as "one-shot" call. the only
case, imo, where it can restart internally with the same timeout is
when select is interrupted by a signal.
> Also I'm wondering what to do in the case bt_devrecv() does actually time
> out - what can it mean and should we [attempt to] cancel the inquiry?
ahh, but in case of bt_devinquiry() it probably does not matter that
much because inquiry itself is limited by the "length" parameter. so
we have to receive something back from the device. i was thinking to
pass -1 as timeout to bt_devrecv() in this particular case, but
decided not to do it (try harder to exit the bt_devinquiry()).
> (actually the whole timeout thing is probably irrelevant for a properly
> functioning device :)
exactly :)
thanks,
max
p.s. thanks for the sdp update. i will try and take a look when i get
free minute.
Maksim Yevmenkin
<maksim.y...@gmail.com> wrote:
>> /* Calculate inquire length in 1.28 second units */
>> to = (time_t) ((double) length / 1.28);
>>
>> if (to <= 0)
>> cp->inquiry_length = 4; /* 5.12 seconds */
>> else if (to > 254)
>> cp->inquiry_length = 255; /* 326.40 seconds */
>> else
>> cp->inquiry_length = to + 1;
>>
>> 2.1 spec says 1.28 -> 61.44 seconds range is acceptable (0x01->0x30)
ok, do you like something like
Index: hci.c
===================================================================
--- hci.c (revision 191499)
+++ hci.c (working copy)
@@ -410,7 +410,6 @@
ng_hci_inquiry_response *ir;
struct bt_devinquiry *i;
int s, n;
- time_t to;
if (ii == NULL) {
errno = EINVAL;
@@ -452,17 +451,21 @@
cp->lap[1] = 0x8b;
cp->lap[2] = 0x9e;
- /* Calculate inquire length in 1.28 second units */
- to = (time_t) ((double) length / 1.28);
- if (to <= 0)
- cp->inquiry_length = 4; /* 5.12 seconds */
- else if (to > 254)
- cp->inquiry_length = 255; /* 326.40 seconds */
- else
- cp->inquiry_length = to + 1;
+ /*
+ * Calculate inquire length in 1.28 second units
+ * v2.x specification says that 1.28 -> 61.44 seconds
+ * range is acceptable
+ */
- to = (time_t)((double) cp->inquiry_length * 1.28) + 1;
+ if (length <= 0)
+ length = 5;
+ else if (length == 1)
+ length = 2;
+ else if (length > 61)
+ length = 61;
+ cp->inquiry_length = (uint8_t)((length * 100) / 128);
+
if (num_rsp <= 0 || num_rsp > 255)
num_rsp = 8;
cp->num_responses = (uint8_t) num_rsp;
@@ -484,7 +487,7 @@
wait_for_more:
- n = bt_devrecv(s, buf, sizeof(buf), to);
+ n = bt_devrecv(s, buf, sizeof(buf), length);
if (n < 0) {
free(i);
bt_devclose(s);
==
thanks,
max
Iain Hibbert
> >> 2.1 spec says 1.28 -> 61.44 seconds range is acceptable (0x01->0x30)
>
> ok, do you like something like
>
> + if (length <= 0)
> + length = 5;
> + else if (length == 1)
> + length = 2;
> + else if (length > 61)
> + length = 61;
yes that is what I have too, except I've allowed 62 (as 61 gives 0x2f)
iain
Maksim Yevmenkin
cool. its in :)
==
Author: emax
Date: Thu May 14 17:10:19 2009
New Revision: 192113
URL: http://svn.freebsd.org/changeset/base/192113
Log:
Avoid floating point arithmetic while calculating iquiry length.
Submitted by: Iain Hibbert < plunky -at- rya-online -dot- net >
MFC after: 1 week
Modified:
head/lib/libbluetooth/hci.c
Modified: head/lib/libbluetooth/hci.c
==
thanks,
max
Iain Hibbert
> Iain,
>
> >> thanks for the review. i committed it to -head.
> >
> > Hi Max, I'm in progress of translating for NetBSD and in bt_devinquiry() I
> > see
> >
> > if (ii == NULL) {
> > errno = EINVAL;
> > return (-1);
> > }
> >
> > but I think this test might be bogus? manpage implies that the caller
> > provides a buffer but we allocate one..
>
> no, the test is correct. yes, we allocate buffer internally, but we
> need to return pointer to the buffer back to the caller. so, ii is
> basically ii is struct bt_devinquiry **. perhaps language in the man
> page is not clear enough?
Ah yes, my bad. I haven't properly looked at the manpage yet (I saw a
spelling mistake earlier but I didn't note it :), will examine it in
detail later. I'm trying to think of another function that does similar to
copy the language from, perhaps asprintf()?
btw do you know what devinfo->role_switch_info might contain? I have
link_policy_info which presumably contains the following flags
/* Link policy settings */
#define HCI_LINK_POLICY_DISABLE_ALL_LM_MODES 0x0000
#define HCI_LINK_POLICY_ENABLE_ROLE_SWITCH 0x0001 /* Master/Slave switch */
#define HCI_LINK_POLICY_ENABLE_HOLD_MODE 0x0002
#define HCI_LINK_POLICY_ENABLE_SNIFF_MODE 0x0004
#define HCI_LINK_POLICY_ENABLE_PARK_MODE 0x0008
/* 0x0010 - 0x8000 - reserved for future use */
but I'm not sure what to put in role_switch_info? (for now, 0 :)
> > also, I'm not sure that the timeout is handled right; the bt_devrecv()
> > uses the complete timeout each time but the time endpoint might need to be
> > calculated so that time-to-end can be used?
>
> well, yes. bt_devrecv() was envisioned as "one-shot" call. the only
> case, imo, where it can restart internally with the same timeout is
> when select is interrupted by a signal.
I think the timeout should probably act the same way as in bt_devreq(),
but I thought there could be close calls at the end because of the integer
arithmetic.
> > Also I'm wondering what to do in the case bt_devrecv() does actually time
> > out - what can it mean and should we [attempt to] cancel the inquiry?
>
> ahh, but in case of bt_devinquiry() it probably does not matter that
> much because inquiry itself is limited by the "length" parameter. so
> we have to receive something back from the device. i was thinking to
> pass -1 as timeout to bt_devrecv() in this particular case, but
> decided not to do it (try harder to exit the bt_devinquiry()).
mm, -1 is tempting but I think its better to error timeout even if the
device is left in a weird state. At least the user won't be left waiting..
I'll continue to work on it because a successful exit must wait until the
device has exited inquiry mode. (I noticed that you can't do things like
get-remote-name when still in inquiry mode, at least on some devices)
iain
Maksim Yevmenkin
[...]
>> > but I think this test might be bogus? manpage implies that the caller
>> > provides a buffer but we allocate one..
>>
>> no, the test is correct. yes, we allocate buffer internally, but we
>> need to return pointer to the buffer back to the caller. so, ii is
>> basically ii is struct bt_devinquiry **. perhaps language in the man
>> page is not clear enough?
>
> Ah yes, my bad. I haven't properly looked at the manpage yet (I saw a
> spelling mistake earlier but I didn't note it :), will examine it in
> detail later. I'm trying to think of another function that does similar to
> copy the language from, perhaps asprintf()?
yeah, may be. please feel free to correct my english :)
> btw do you know what devinfo->role_switch_info might contain? I have
> link_policy_info which presumably contains the following flags
that is for 'forceful' role switch when we are accepting connection.
basically always try to become master on any connection (or not).
[...]
>> > also, I'm not sure that the timeout is handled right; the bt_devrecv()
>> > uses the complete timeout each time but the time endpoint might need to be
>> > calculated so that time-to-end can be used?
>>
>> well, yes. bt_devrecv() was envisioned as "one-shot" call. the only
>> case, imo, where it can restart internally with the same timeout is
>> when select is interrupted by a signal.
>
> I think the timeout should probably act the same way as in bt_devreq(),
> but I thought there could be close calls at the end because of the integer
> arithmetic.
sorry, are we talking about bt_devrecv() in isolation, or about
bt_devinquiry() usage of bt_devrecv().
in other words, are you saying we should fix bt_devinquiry() and make
sure that is decreases timeout with every call to bt_devrecv()?
>> > Also I'm wondering what to do in the case bt_devrecv() does actually time
>> > out - what can it mean and should we [attempt to] cancel the inquiry?
>>
>> ahh, but in case of bt_devinquiry() it probably does not matter that
>> much because inquiry itself is limited by the "length" parameter. so
>> we have to receive something back from the device. i was thinking to
>> pass -1 as timeout to bt_devrecv() in this particular case, but
>> decided not to do it (try harder to exit the bt_devinquiry()).
>
> mm, -1 is tempting but I think its better to error timeout even if the
> device is left in a weird state. At least the user won't be left waiting..
my thoughts exactly :)
> I'll continue to work on it because a successful exit must wait until the
> device has exited inquiry mode. (I noticed that you can't do things like
> get-remote-name when still in inquiry mode, at least on some devices)
yes, inquiry is weird (from baseband point of view) procedure. device
basically has to blast on the range of frequencies and wait for
someone to respond. technically, nothing prevents device from doing
other rf activity, but some devices choose not to do it.
thanks,
max
Iain Hibbert
> that is for 'forceful' role switch when we are accepting connection.
> basically always try to become master on any connection (or not).
Mm, I didn't put that into netbsd (yet) so it can remain 0 for now,
thanks.
> >> > also, I'm not sure that the timeout is handled right; the bt_devrecv()
> >> > uses the complete timeout each time but the time endpoint might need to be
> >> > calculated so that time-to-end can be used?
> >>
> >> well, yes. bt_devrecv() was envisioned as "one-shot" call. the only
> >> case, imo, where it can restart internally with the same timeout is
> >> when select is interrupted by a signal.
> >
> > I think the timeout should probably act the same way as in bt_devreq(),
> > but I thought there could be close calls at the end because of the integer
> > arithmetic.
>
> sorry, are we talking about bt_devrecv() in isolation, or about
> bt_devinquiry() usage of bt_devrecv().
>
> in other words, are you saying we should fix bt_devinquiry() and make
> sure that is decreases timeout with every call to bt_devrecv()?
yes, sorry - what bt_devrecv() does in isolation is fine, it is
bt_devinquiry() that should manage a cumulative target for timeout.
(btw 'close call' in quote above is 'near miss' not 'close() call' :)
> yes, inquiry is weird (from baseband point of view) procedure. device
> basically has to blast on the range of frequencies and wait for
> someone to respond. technically, nothing prevents device from doing
> other rf activity, but some devices choose not to do it.
I haven't looked but I think from skimming their list that latest linux
code (not sure if in kernel or bluetoothd) sets the device into inquiry
state and manages the connections to avoid errors in that situation. I
thought about that for netbsd but didn't get far - it also would cover
things like pausing encryption to make role switches and suchlike which
was causing me a trouble when I tried to make a MASTER mode work.
iain
Iain Hibbert
some more queries I'm not sure about..
--- hci.c.orig 2009-05-14 21:11:04.000000000 +0100
+++ hci.c 2009-05-14 21:26:57.000000000 +0100
@@ -118,7 +118,7 @@ bt_devsend(int s, uint16_t opcode, void
h.length = 0;
while (writev(s, iv, ivn) < 0) {
- if (errno == EAGAIN || errno == EINTR)
+ if (errno == EINTR)
continue;
return (-1);
@@ -163,12 +163,15 @@ bt_devrecv(int s, void *buf, size_t size
}
while ((n = read(s, buf, size)) < 0) {
- if (errno == EAGAIN || errno == EINTR)
+ if (errno == EINTR)
continue;
return (-1);
I think these two should not have EAGAIN because that can be returned if
the socket is set to nonblocking and we should just pass it back to the
caller rather than looping?
}
+ if (n == 0)
+ return (0);
+
the read() in bt_devrecv() can return 0 for EOF at least. ok it shouldn't
happen but do you think it would be proper to return it? (or call it an
error? I dunno)
@@ -273,10 +276,11 @@ bt_devreq(int s, struct bt_devreq *r, ti
if (r->rlen >= n) {
r->rlen = n;
memcpy(r->rparam, cc + 1, r->rlen);
}
+ /* else what? */
goto out;
}
break;
@@ -290,10 +294,11 @@ bt_devreq(int s, struct bt_devreq *r, ti
} else {
if (r->rlen >= n) {
r->rlen = n;
memcpy(r->rparam, cs, r->rlen);
}
+ /* else what? */
goto out;
}
}
break;
@@ -302,10 +307,11 @@ bt_devreq(int s, struct bt_devreq *r, ti
if (e->event == r->event) {
if (r->rlen >= n) {
r->rlen = n;
memcpy(r->rparam, e + 1, r->rlen);
}
+ /* else what? */
these three locations in bt_devreq() I'm not sure what should happen if
the provided buffer was not big enough. I suggest the following construct
instead:
if (r->rlen > n)
r->rlen = n;
if (r->rlen > 0)
memcpy(r->rparam, ..., r->rlen);
which gives them as much as they asked for and discards the rest, how
would that look?
Also, I'm thinking of the case where r->event=0 but a command_status is
returned. If status != 0, an error is produced but if status == 0 then we
should set rlen = 0 and return success?
@@ -504,10 +510,16 @@ wait_for_more:
case NG_HCI_EVENT_INQUIRY_RESULT:
ir = (ng_hci_inquiry_response *)(ep + 1);
for (n = 0; n < MIN(ep->num_responses, num_rsp); n ++) {
I found while writing the inquiry routine in btconfig(8) that some devices
don't filter repeat addresses properly (or maybe its that some devices
continue to respond, I've seen that too). Perhaps its worth handling that
case inside the bt_devinquiry() function by discarding dupes?
@@ -551,11 +563,11 @@ bt_devinfo(struct bt_devinfo *di)
return (-1);
}
s = bt_devopen(di->devname);
if (s < 0)
return (-1);
I'm not sure if in FreeBSD you can connect() to a device that is not
marked up, but in NetBSD you can't. In any case, there is information in
the devinfo structure that is only available from activated devices so it
may fail?
This really relates to bt_devenum() as below, are we counting 'active'
devices or 'all' devices, as you ignored errors?
@@ -659,21 +671,24 @@ bt_devenum(bt_devenum_cb_t cb, void *arg
}
for (count = 0, i = 0; i < rp.num_names; i ++) {
strlcpy(di.devname, rp.names[i].name, sizeof(di.devname));
if (bt_devinfo(&di) < 0)
- continue;
+ continue; /* or maybe error? */
count ++;
if (cb == NULL)
continue;
strlcpy(ha.hci_node, rp.names[i].name, sizeof(ha.hci_node));
if (bind(s, (struct sockaddr *) &ha, sizeof(ha)) < 0 ||
connect(s, (struct sockaddr *) &ha, sizeof(ha)) < 0)
- continue;
+ continue; /* or maybe error? */
+
I think the second one should cause an error return at least? Not sure
about the first - in NetBSD I have the flags already so I've skipped
inactive interfaces as below
while (ioctl(s, SIOCNBTINFO, &btr) != -1) {
if ((btr.btr_flags & BTF_UP) == 0)
continue;
count++;
if (cb == NULL)
continue;
memset(&di, 0, sizeof(di));
strlcpy(di.devname, btr.btr_name, HCI_DEVNAME_SIZE);
memset(&sa, 0, sizeof(sa));
sa.bt_len = sizeof(sa);
sa.bt_family = AF_BLUETOOTH;
bdaddr_copy(&sa.bt_bdaddr, &btr.btr_bdaddr);
if (bt_devinfo(&di) == -1
|| bind(s, (struct sockaddr *)&sa, sizeof(sa)) == -1
|| connect(s, (struct sockaddr *)&sa, sizeof(sa)) == -1) {
close(s);
return -1;
}
if ((*cb)(s, &di, arg) != 0)
break;
}
thats all for now :)
iain
Maksim Yevmenkin
> Hi Max,
> some more queries I'm not sure about..
>
> --- hci.c.orig 2009-05-14 21:11:04.000000000 +0100
> +++ hci.c 2009-05-14 21:26:57.000000000 +0100
> @@ -118,7 +118,7 @@ bt_devsend(int s, uint16_t opcode, void
> h.length = 0;
>
> while (writev(s, iv, ivn) < 0) {
> - if (errno == EAGAIN || errno == EINTR)
> + if (errno == EINTR)
> continue;
>
> return (-1);
> @@ -163,12 +163,15 @@ bt_devrecv(int s, void *buf, size_t size
> }
>
> while ((n = read(s, buf, size)) < 0) {
> - if (errno == EAGAIN || errno == EINTR)
> + if (errno == EINTR)
> continue;
>
> return (-1);
>
> I think these two should not have EAGAIN because that can be returned if
> the socket is set to nonblocking and we should just pass it back to the
> caller rather than looping?
i somewhat disagree. it is true that _internally_ libbluetooth does
not set hci socket to the non-blocking mode. however,
bt_devsend/recv() can be used with the hci socket that was created
outside of libbluetooth.
[....]
> + if (n == 0)
> + return (0);
> +
>
> the read() in bt_devrecv() can return 0 for EOF at least. ok it shouldn't
> happen but do you think it would be proper to return it? (or call it an
> error? I dunno)
can it really return 0? i mean specifically when reading from
bluetooth socket not in general. in any case the existing code will
return EIO, i.e. error. granted, it will try to parse data in buf,
but it will always fail if n == 0.
i did not really know what to do here. practically, this just should
not happen. caller is expected to know how big return parameters are
and pass appropriately sized buffer. if the caller really does not
care about return parameters, then no buffer should be passed.
returning something that is incomplete is a bit broken, imo. i
actually wrote the code like you suggested, but then decided not to do
it this way. cant remember why :) i also considered to return EMSGSIZE
in this case, but then deiced not do to it either. again i cant recall
the exact reason :)
since this is a relatively minor issue, lets agree on something and
move on. i could go either way.
> Also, I'm thinking of the case where r->event=0 but a command_status is
> returned. If status != 0, an error is produced but if status == 0 then we
> should set rlen = 0 and return success?
what is wrong with the existing code? it simply returns status code in
the return parameters and let caller deal with it. why give special
treatment to status == 0 code? as far as bt_devreq() is concern,
request has completed, i.e. we send something to the device and we
got something back. since caller gave us no specifics (i.e. r->event
== 0), caller should deal with the return parameters.
> @@ -504,10 +510,16 @@ wait_for_more:
>
> case NG_HCI_EVENT_INQUIRY_RESULT:
> ir = (ng_hci_inquiry_response *)(ep + 1);
>
> for (n = 0; n < MIN(ep->num_responses, num_rsp); n ++) {
>
> I found while writing the inquiry routine in btconfig(8) that some devices
> don't filter repeat addresses properly (or maybe its that some devices
> continue to respond, I've seen that too). Perhaps its worth handling that
> case inside the bt_devinquiry() function by discarding dupes?
yes, i've seen that too. broadcom chips (at least in my case) have
this behavior. how do you propose to detect the dupes? using bd_addr
only? or match all/some parameters as well? i think it would be ok to
remove dupes.
> @@ -551,11 +563,11 @@ bt_devinfo(struct bt_devinfo *di)
> return (-1);
> }
>
> s = bt_devopen(di->devname);
> if (s < 0)
> return (-1);
>
> I'm not sure if in FreeBSD you can connect() to a device that is not
> marked up, but in NetBSD you can't. In any case, there is information in
> the devinfo structure that is only available from activated devices so it
> may fail?
in freebsd, you can. bind()/connect() just sets device name in
socket's pcb. in fact, in freebsd, you can bind/connect() hci socket
to anything :)
> This really relates to bt_devenum() as below, are we counting 'active'
> devices or 'all' devices, as you ignored errors?
in freebsd, we return the list of netgraph nodes. that is, device may
be present (node exists) but not 'up'. that is what 'state' parameter
in devinfo structure is telling you. so, in freebsd, devenum() will
return the list of all devices.
> @@ -659,21 +671,24 @@ bt_devenum(bt_devenum_cb_t cb, void *arg
> }
>
> for (count = 0, i = 0; i < rp.num_names; i ++) {
> strlcpy(di.devname, rp.names[i].name, sizeof(di.devname));
> if (bt_devinfo(&di) < 0)
> - continue;
> + continue; /* or maybe error? */
>
> count ++;
>
> if (cb == NULL)
> continue;
>
> strlcpy(ha.hci_node, rp.names[i].name, sizeof(ha.hci_node));
> if (bind(s, (struct sockaddr *) &ha, sizeof(ha)) < 0 ||
> connect(s, (struct sockaddr *) &ha, sizeof(ha)) < 0)
> - continue;
> + continue; /* or maybe error? */
> +
>
> I think the second one should cause an error return at least? Not sure
> about the first - in NetBSD I have the flags already so I've skipped
> inactive interfaces as below
this is probably something that should be os/stack implementation
specific. however, i'll try very hard to make devenum() interface
behave similarly on different os's. perhaps there should be another
parameter to devenum() that tells which devices it should enumerate?
i.e. all or active?
thanks,
max
Iain Hibbert
Yes, thats the case I meant - actually I see the manpage says it should be
used with socket created by bt_devopen() but in the case where a socket
has NBIO set, bt_devrecv() will end up crazyfast looping (==bad) rather
than do poll/return as read() normally would if there is no data.
> [....]
>
> > + if (n == 0)
> > + return (0);
> > +
> >
> > the read() in bt_devrecv() can return 0 for EOF at least. ok it shouldn't
> > happen but do you think it would be proper to return it? (or call it an
> > error? I dunno)
>
> can it really return 0? i mean specifically when reading from
> bluetooth socket not in general. in any case the existing code will
> return EIO, i.e. error. granted, it will try to parse data in buf,
> but it will always fail if n == 0.
Well I thought perhaps if a (eg) USB device was removed but it turns out
on NetBSD that doesn't cause the socket to EOF, it will just error out on
sending, perhaps I'm being too careful with the edge cases..
Well, there was a case of bad bluetooth device I saw mentioned once that
gives the inquiry_rssi_result with the wrong packet size but I can't
remember the details, perhaps you are right - in fact bt_devrecv()
explicitly makes sure that you have the complete packet so perhaps this
should just be the same. I think it should probably return an error (EIO)
though? eg
if (r->rlen >= n) {
r->rlen = n;
memcpy(r->rparam, ..., r->rlen);
} else if (r->rlen > 0) {
error = EIO;
goto out;
}
> > Also, I'm thinking of the case where r->event=0 but a command_status is
> > returned. If status != 0, an error is produced but if status == 0 then we
> > should set rlen = 0 and return success?
>
> what is wrong with the existing code? it simply returns status code in
> the return parameters and let caller deal with it. why give special
> treatment to status == 0 code? as far as bt_devreq() is concern,
> request has completed, i.e. we send something to the device and we
> got something back. since caller gave us no specifics (i.e. r->event
> == 0), caller should deal with the return parameters.
No I think you right I was misreading that, too many nestings :)
> > @@ -504,10 +510,16 @@ wait_for_more:
> >
> > case NG_HCI_EVENT_INQUIRY_RESULT:
> > ir = (ng_hci_inquiry_response *)(ep + 1);
> >
> > for (n = 0; n < MIN(ep->num_responses, num_rsp); n ++) {
> >
> > I found while writing the inquiry routine in btconfig(8) that some devices
> > don't filter repeat addresses properly (or maybe its that some devices
> > continue to respond, I've seen that too). Perhaps its worth handling that
> > case inside the bt_devinquiry() function by discarding dupes?
>
> yes, i've seen that too. broadcom chips (at least in my case) have
> this behavior. how do you propose to detect the dupes? using bd_addr
> only? or match all/some parameters as well? i think it would be ok to
> remove dupes.
No just check bdaddr as it should be the same device. I've come up with
the following function to add results to the list.. to be called with
whichever data is present in relevant response and it overwrites the
earlier entry in case anything changed.
static void
bt__devresult(struct bt_devinquiry *ii, int *count, bdaddr_t *ba, uint8_t psrm,
uint8_t pspm, uint8_t *cl, uint16_t co, int8_t rssi, uint8_t *data)
{
int n;
for (n = 0; n < *count; n++)
if (bdaddr_same(&ii[n].bdaddr, ba))
break;
bdaddr_copy(&ii[n].bdaddr, ba);
if (cl != NULL)
memcpy(ii[n].dev_class, cl, HCI_CLASS_SIZE);
ii[n].pscan_rep_mode = psrm;
ii[n].pscan_period_mode = pspm;
ii[n].clock_offset = le16toh(co);
ii[n].rssi = rssi;
if (data != NULL)
memcpy(ii[n].data, data, 240);
if (n == *count)
(*count)++;
}
(I don't really like massive arglists but its better than duplicating the
code three times for inquiry_result, rssi_result and extended_result)
> > @@ -551,11 +563,11 @@ bt_devinfo(struct bt_devinfo *di)
> > return (-1);
> > }
> >
> > s = bt_devopen(di->devname);
> > if (s < 0)
> > return (-1);
> >
> > I'm not sure if in FreeBSD you can connect() to a device that is not
> > marked up, but in NetBSD you can't. In any case, there is information in
> > the devinfo structure that is only available from activated devices so it
> > may fail?
>
> in freebsd, you can. bind()/connect() just sets device name in
> socket's pcb. in fact, in freebsd, you can bind/connect() hci socket
> to anything :)
yeah I let bind do any address since if it doesn't exist then you won't
get any messages but connect seemed wrong to connect to nothing. sendto()
also fails if the destination is unknown..
I presume that for inactive devices things like features, bdaddr and
buffer_size results will just be nil?
> > This really relates to bt_devenum() as below, are we counting 'active'
> > devices or 'all' devices, as you ignored errors?
>
> in freebsd, we return the list of netgraph nodes. that is, device may
> be present (node exists) but not 'up'. that is what 'state' parameter
> in devinfo structure is telling you. so, in freebsd, devenum() will
> return the list of all devices.
Mm, I've fixed bt_devinfo() so it will work for inactive devices, will
think about the devenum some more. What is the 'state' parameter
containing is it just 0 = down, 1 = up or is there more?
I think devenum should probably return all devs as caller can check the
status but I'm not sure if its useful to have a socket connected to an
empty device and know no details in the down case.. what happens if you
send hci commands to a down device, nothing?
iain
Iain Hibbert
more queries
- in bt_devany_cb() you should perhaps make sure that the device is an
enabled one?
- I'm thinking that bt_devopen() should have an options argument, in order
to pre-set any options such as TIMESTAMP, DIRECTION etc (even NBIO)
which will get around the differences in API for that (BlueZ had a weird
thing with not supporting SOL_SOCKET, SO_TIMESTAMP even though Linux
does I don't know if they fixed it yet)
- in bt_devinquiry() we accept (dev == NULL) to mean any device, should
that happen in bt_devopen() too?
- along this line I wonder if it is possible open a promiscuous socket
(eg as used by hcidump) instead of any particular device? (could be
dev==NULL I guess, or a PROMISCUOUS option?) I'm not sure how Linux
works but on NetBSD you must explicitly bind to 00:00:00:00:00:00 to
get that behaviour (IIRC FreeBSD gives it to you by default but I was
paranoid :)
regards,
iain
Iain Hibbert
> Ah yes, my bad. I haven't properly looked at the manpage yet (I saw a
> spelling mistake earlier but I didn't note it :),
found it,
iain
--- bluetooth.3 22 Apr 2009 15:50:03 -0000 1.10
+++ bluetooth.3 17 May 2009 19:03:34 -0000
@@ -272,7 +272,7 @@
.Pp
The
.Fn bt_devinfo
-function populates prodivded
+function populates provided
.Vt bt_devinfo
structure with the information about given Bluetooth device.
The caller is expected to pass Bluetooth device name in the
Maksim Yevmenkin
[...]
>> > I think these two should not have EAGAIN because that can be returned if
>> > the socket is set to nonblocking and we should just pass it back to the
>> > caller rather than looping?
>>
>> i somewhat disagree. it is true that _internally_ libbluetooth does
>> not set hci socket to the non-blocking mode. however,
>> bt_devsend/recv() can be used with the hci socket that was created
>> outside of libbluetooth.
>
> Yes, thats the case I meant - actually I see the manpage says it should be
> used with socket created by bt_devopen() but in the case where a socket
> has NBIO set, bt_devrecv() will end up crazyfast looping (==bad) rather
> than do poll/return as read() normally would if there is no data.
oh, i see. you are right. we need to remove EAGAIN completely from
everywhere in libbluetooth and only check for EINTR. i already have
done it in my local tree.
[...]
i would prefer EMSGSIZE, i.e. what i've done here locally is
==
if (r->rlen >= n) {
r->rlen = n;
memcpy(r->rparam, e + 1, r->rlen);
- }
+ } else if (r->rlen > 0)
+ error = EMSGSIZE;
goto out;
==
[...]
>> > case NG_HCI_EVENT_INQUIRY_RESULT:
>> > ir = (ng_hci_inquiry_response *)(ep + 1);
>> >
>> > for (n = 0; n < MIN(ep->num_responses, num_rsp); n ++) {
>> >
>> > I found while writing the inquiry routine in btconfig(8) that some devices
>> > don't filter repeat addresses properly (or maybe its that some devices
>> > continue to respond, I've seen that too). Perhaps its worth handling that
>> > case inside the bt_devinquiry() function by discarding dupes?
>>
>> yes, i've seen that too. broadcom chips (at least in my case) have
>> this behavior. how do you propose to detect the dupes? using bd_addr
>> only? or match all/some parameters as well? i think it would be ok to
>> remove dupes.
>
> No just check bdaddr as it should be the same device. I've come up with
> the following function to add results to the list.. to be called with
> whichever data is present in relevant response and it overwrites the
> earlier entry in case anything changed.
right, lets suppress the dupes in inquiry results. lets only match
bd_addr and do not match any other parameters.
>> > @@ -551,11 +563,11 @@ bt_devinfo(struct bt_devinfo *di)
>> > return (-1);
>> > }
>> >
>> > s = bt_devopen(di->devname);
>> > if (s < 0)
>> > return (-1);
>> >
>> > I'm not sure if in FreeBSD you can connect() to a device that is not
>> > marked up, but in NetBSD you can't. In any case, there is information in
>> > the devinfo structure that is only available from activated devices so it
>> > may fail?
>>
>> in freebsd, you can. bind()/connect() just sets device name in
>> socket's pcb. in fact, in freebsd, you can bind/connect() hci socket
>> to anything :)
>
> yeah I let bind do any address since if it doesn't exist then you won't
> get any messages but connect seemed wrong to connect to nothing. sendto()
> also fails if the destination is unknown..
>
> I presume that for inactive devices things like features, bdaddr and
> buffer_size results will just be nil?
depends what state node is in. there might be some residual data left,
however, this really should not happen unless someone is messing with
the nodes by hand (basically disconnect netgraph hooks) or
intentionally misconfigured the system. in any case, state parameter
should be checked first before accessing anything else. i'm guessing
we need to define bluetooth device states independent of
implementation. in freebsd we have connected, initialized and ready =
connected+ready states. connected means that device hook is connected
(i.e. device is present). initialized means that we run initialization
sequence (get bd_addr, features, etc.).
>> > This really relates to bt_devenum() as below, are we counting 'active'
>> > devices or 'all' devices, as you ignored errors?
>>
>> in freebsd, we return the list of netgraph nodes. that is, device may
>> be present (node exists) but not 'up'. that is what 'state' parameter
>> in devinfo structure is telling you. so, in freebsd, devenum() will
>> return the list of all devices.
>
> Mm, I've fixed bt_devinfo() so it will work for inactive devices, will
> think about the devenum some more. What is the 'state' parameter
> containing is it just 0 = down, 1 = up or is there more?
>
> I think devenum should probably return all devs as caller can check the
> status but I'm not sure if its useful to have a socket connected to an
> empty device and know no details in the down case.. what happens if you
> send hci commands to a down device, nothing?
like i said, we have connected and initialized states. as long as
device is in the 'connected' state we will should a response from it.
from your other email.
> - in bt_devany_cb() you should perhaps make sure that the device is an
> enabled one?
what does 'enabled' means?
> - I'm thinking that bt_devopen() should have an options argument, in order
> to pre-set any options such as TIMESTAMP, DIRECTION etc (even NBIO)
> which will get around the differences in API for that (BlueZ had a weird
> thing with not supporting SOL_SOCKET, SO_TIMESTAMP even though Linux
> does I don't know if they fixed it yet)
maybe bt_dev{get,set}opts() ?
> - in bt_devinquiry() we accept (dev == NULL) to mean any device, should
> that happen in bt_devopen() too?
we could.
> - along this line I wonder if it is possible open a promiscuous socket
> (eg as used by hcidump) instead of any particular device? (could be
> dev==NULL I guess, or a PROMISCUOUS option?) I'm not sure how Linux
> works but on NetBSD you must explicitly bind to 00:00:00:00:00:00 to
> get that behaviour (IIRC FreeBSD gives it to you by default but I was
> paranoid :)
right, in freebsd we just don't bind socket to anything and use filter
to enable all event/packets/etc. this is how we get promiscuous
socket. perhaps bt_devopen(NULL) should mean create non-bound socket?
thanks,
max
Iain Hibbert
> in any case, state parameter should be checked first before accessing
> anything else. i'm guessing we need to define bluetooth device states
> independent of implementation. in freebsd we have connected, initialized
> and ready = connected+ready states. connected means that device hook is
> connected (i.e. device is present). initialized means that we run
> initialization sequence (get bd_addr, features, etc.).
Mm, I have a bunch of device flags for this situation. I followed the
example of a network interface where it must be enabled to work so you can
turn it off and save power (for PCMCIA at least it does that, in USB I
don't know if there is a way to turn off the power)
#define BTF_UP (1<<0) /* unit is up */
#define BTF_RUNNING (1<<1) /* unit is running */
#define BTF_INIT_BDADDR (1<<5) /* waiting for bdaddr */
#define BTF_INIT_BUFFER_SIZE (1<<6) /* waiting for buffer size */
#define BTF_INIT_FEATURES (1<<7) /* waiting for features */
#define BTF_POWER_UP_NOOP (1<<8) /* should wait for No-op on power up */
#define BTF_INIT_COMMANDS (1<<9) /* waiting for supported commands */
#define BTF_INIT (BTF_INIT_BDADDR \
| BTF_INIT_BUFFER_SIZE \
| BTF_INIT_FEATURES \
| BTF_INIT_COMMANDS)
but that can be easily converted to other values
(btw BlueZ/Linux seems to provide UP/INIT/RUNNING flags)
> > Mm, I've fixed bt_devinfo() so it will work for inactive devices, will
> > think about the devenum some more.
I fixed that now, it just does the best it can - if the device is not
enabled you get an unbound socket (but since sending stuff wouldn't work
anyway it probably doesn't matter for now :)
> > - in bt_devany_cb() you should perhaps make sure that the device is an
> > enabled one?
>
> what does 'enabled' means?
I guess it should wait until it finds a 'connected' device then. Otherwise
if there are N devices but the first has just been plugged in, such an
inquiry may fail. (its one of those edge cases :)
{
if ((di->state & BTF_UP)) {
memcpy(arg, di->devname, HCI_DEVNAME_SIZE);
return 1;
}
return 0;
}
> > - I'm thinking that bt_devopen() should have an options argument, in order
> > to pre-set any options such as TIMESTAMP, DIRECTION etc (even NBIO)
> > which will get around the differences in API for that (BlueZ had a weird
> > thing with not supporting SOL_SOCKET, SO_TIMESTAMP even though Linux
> > does I don't know if they fixed it yet)
>
> maybe bt_dev{get,set}opts() ?
Well, I think a flags/options arg is simpler (very rare would anybody want
to change the options during the lifetime of a socket?)
Actually, I thought about this some more and found another problem in that
bt_devrecv() does not have any way to extract the control messages.
> > - in bt_devinquiry() we accept (dev == NULL) to mean any device, should
> > that happen in bt_devopen() too?
>
> we could.
>
> > - along this line I wonder if it is possible open a promiscuous socket
> > (eg as used by hcidump) instead of any particular device? (could be
> > dev==NULL I guess, or a PROMISCUOUS option?) I'm not sure how Linux
> > works but on NetBSD you must explicitly bind to 00:00:00:00:00:00 to
> > get that behaviour (IIRC FreeBSD gives it to you by default but I was
> > paranoid :)
>
> right, in freebsd we just don't bind socket to anything and use filter
> to enable all event/packets/etc. this is how we get promiscuous
> socket.
The thing I didn't like about that is that you might get messages that you
think came from your bound device but in fact they came from some other..
> perhaps bt_devopen(NULL) should mean create non-bound socket?
Hmm, need to think about it some - again, bt_devrecv() does not have any
way to retreive the socket address, so it would need to be declared that
the descriptor bt_devopen() returns is a socket handle and using
sendto/recvfrom/sendmsg/recvmsg etc is ok in that case. That means that a
system that uses a device node instead of a socket for HCI cannot use the
API. Perhaps not a problem?
I prefer bt_devopen(NULL) to mean 'receive messages from all devices'
rather than 'find first device' though..
iain
Iain Hibbert
> On Thu, 14 May 2009, Iain Hibbert wrote:
>
> > Ah yes, my bad. I haven't properly looked at the manpage yet (I saw a
> > spelling mistake earlier but I didn't note it :),
>
> found it,
actually, more :)
iain
Index: bluetooth.3
===================================================================
RCS file: /home/ncvs/src/lib/libbluetooth/bluetooth.3,v
retrieving revision 1.10
diff -u -r1.10 bluetooth.3
--- bluetooth.3 22 Apr 2009 15:50:03 -0000 1.10
+++ bluetooth.3 20 May 2009 08:17:03 -0000
@@ -115,13 +115,13 @@
.Ft void
.Fn bt_devfilter_pkt_set "struct bt_devfilter *filter" "uint8_t type"
.Ft void
-.Fn bt_devfilter_pkt_clt "struct bt_devfilter *filter" "uint8_t type"
+.Fn bt_devfilter_pkt_clr "struct bt_devfilter *filter" "uint8_t type"
.Ft int
.Fn bt_devfilter_pkt_tst "struct bt_devfilter const *filter" "uint8_t type"
.Ft void
.Fn bt_devfilter_evt_set "struct bt_devfilter *filter" "uint8_t event"
.Ft void
-.Fn bt_devfilter_evt_clt "struct bt_devfilter *filter" "uint8_t event"
+.Fn bt_devfilter_evt_clr "struct bt_devfilter *filter" "uint8_t event"
.Ft int
.Fn bt_devfilter_evt_tst "struct bt_devfilter const *filter" "uint8_t event"
.Ft int
@@ -272,7 +272,7 @@
.Pp
The
.Fn bt_devinfo
-function populates prodivded
+function populates provided
.Vt bt_devinfo
structure with the information about given Bluetooth device.
The caller is expected to pass Bluetooth device name in the
@@ -383,7 +383,7 @@
.Xr bt_devopen 3 .
The
.Fa opcode
-parameter is exppected to be in the host byte order.
+parameter is expected to be in the host byte order.
The
.Fa param
and
Iain Hibbert
> however, bt_devsend/recv() can be used with the hci socket that was
> created outside of libbluetooth.
If this is to be the case (and I like it) then I think bt_devopen() should
be documented to return a normal socket handle and have the caller call
close() directly rather than bt_devclose() which adds nothing.
also, on bt_devsend() can it return ssize_t number of bytes written? (I
can't think a reason to want it, but it seems wasteful to discard :)
and, going back a bit..
On Sat, Feb 14, 2009 at 1:52 AM, Iain Hibbert <plu...@rya-online.net> wrote:
> (size and name could be passed in devinfo?)
In the end, I'm not sure I actually like passing the name in the devinfo
structure.. means every caller must do a copy rather than just passing it.
I think bt_devinfo(name, di) would be better?
iain