2005-10-19 14:07:15

by Johan Hedberg

[permalink] [raw]
Subject: [Bluez-devel] [PATCH] Make RemoteName D-BUS method non-blocking

Hi,

Here's a patch which does a similar change RemoteName as my previous
patch did to Inquiry. For the user to be able to know if the name
request fails because of e.g. page timeout I have added a status
paramer to the signal and forced the signal to be always sent (not just
when evt->status == 0).

I also updated the pygtk testing program to work with this change:
http://www.iki.fi/~jhedberg/bluez-python/bluez-python-0.4.tar.gz

There is one problem when using hci_send_req with the inquiry and remote
name HCI commands: they don't generate a command complete event, only a
command status event. This means that hci_send_req will fail with
ETIMEDOUT (I didn't notice it earlier since the UI status is immediately
updated by the InquiryStarted signal). Using hci_send_cmd is also not
(IMHO) an option since a failure status in the command status event
should be detected and handled properly.

I can think of a couple of possible solutions to this:
1. Change hci_send_req to detect the special cases that don't produce
command complete event.
2. Do the listening of command status event ourselves in dbus.c
3. Check for ETIMEDOUT error and treat it as success

I went with option 3 for now since it requires the least code changes,
but it should probably not be the final solution since it doesn't catch
the situation where command status event isn't received in time. Any
suggestions for how this should be best fixed?

Johan


Attachments:
(No filename) (1.40 kB)
remote-name.patch (4.42 kB)
Download all attachments

2005-10-22 12:58:02

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [Bluez-devel] [PATCH] Make RemoteName D-BUS method non-blocking

Hi Johan,

> > > Ok. I'll create a new RemoteNameFailed signal which has one byte
> > > parameter (the status code). Do you want yet another patch against the
> > > current CVS HEAD, or should I wait for you to commit my previous patch
> > > and then create a new patch against the new CVS revision?
> >
> > I only applied the library patch and not your remote name patch. So
> > please do it against CVS HEAD now.
>
> Here you go (patch attached).

the patch is now in the CVS. I had to catch the non D-Bus case and
changed the parameter orders of the failed function.

Regards

Marcel




-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

2005-10-20 13:37:34

by Johan Hedberg

[permalink] [raw]
Subject: Re: [Bluez-devel] [PATCH] Make RemoteName D-BUS method non-blocking

Hi,

Just thought that I'd inform that there is a new version of the pygtk
testing program which works with the latest remote-name patch that I
sent (from the usual place: http://www.iki.fi/~jhedberg/bluez-python/).

I've also managed to get adding and removing of local bluetooth devices
while the program is running working quite well.

Johan


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

2005-10-20 11:21:53

by Johan Hedberg

[permalink] [raw]
Subject: Re: [Bluez-devel] [PATCH] Make RemoteName D-BUS method non-blocking

On Thu, Oct 20, 2005, Marcel Holtmann wrote:
> > Ok. I'll create a new RemoteNameFailed signal which has one byte
> > parameter (the status code). Do you want yet another patch against the
> > current CVS HEAD, or should I wait for you to commit my previous patch
> > and then create a new patch against the new CVS revision?
>
> I only applied the library patch and not your remote name patch. So
> please do it against CVS HEAD now.

Here you go (patch attached).

> Doesn't it make sense to use the general error method? We spent so many
> time talking about the error definition. Shouldn't we use it?

I'm not sure what you mean. Are you talking about the RemoteNameFailed
signal or the error return to the RemoteName method? I checked the
bluez_new_failure_msg function and it seems Claudio has only implemented
support for system errors (i.e. errno) and Bluez D-BUS errors. If I give
the status from command status event to that function it will return
NULL. I guess another patch from Claudio would be needed to create D-BUS
error messages from HCI error codes.

> I think about renaming the InquiryResult into something more meaningful
> like RemoteDevice or something. This signal should include the name from
> the extended inquiry or a cached name or an empty string. We might also
> include the list of UUIDs and some vendor information. The future is the
> extended inquiry and we should plan the D-Bus interface around it. Any
> comments?

I guess I'll actually need to read the extended inquiry response spec.
(just printed it) and think about this :-) I'll also have to investigate
how to use the name cache and maybe send a patch later which checks for
the name in it before sending the remote name request HCI command.
However, there *should* be a way for the user to force the remote name
HCI command to be sent (i.e. overriding the cache), either with the
boolean parameter I suggested or then a separate method call.

Johan


Attachments:
(No filename) (1.90 kB)
remote-name4.patch (6.59 kB)
Download all attachments

2005-10-20 10:16:07

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [Bluez-devel] [PATCH] Make RemoteName D-BUS method non-blocking

Hi Johan,

> > I am not that big in D-Bus programming, but I think that is how it
> > should work. The failed signal then can contain an error code or we
> > simply use the error method for it.
>
> Ok. I'll create a new RemoteNameFailed signal which has one byte
> parameter (the status code). Do you want yet another patch against the
> current CVS HEAD, or should I wait for you to commit my previous patch
> and then create a new patch against the new CVS revision?

I only applied the library patch and not your remote name patch. So
please do it against CVS HEAD now.

Doesn't it make sense to use the general error method? We spent so many
time talking about the error definition. Shouldn't we use it?

> > Another thing is that we might wanna have a method where we can request
> > a name from the cache and if not available we request it.
>
> Maybe this could be done by adding a boolean "use cache" parameter to
> the RemoteName method?

I think using the cache should be default, because the name resolving
always requires a baseband connection and this costs too much to do it
to often. My idea is that we handle all things in background without
putting too much intelligence about names into the client application.
So please come up with ideas what an application really need to now
about names.

> > Besides this we also should keep the extended inquiry in mind. This can
> > deliver us the full name via an inquiry response or at least a short
> > name for a device. So what I see is we have four different types of
> > device names inside our system.
> >
> > - Full device names (up to 248 characters)
> > - Short names or inquiry names (up to around 235 at max)
> > - Cached names (in generell only up to 248 characters)
> > - Alias names (no length requirement)
> > How do deal with them through a nice interface, because all of them can
> > be different.
>
> I think that the extendend inquiry response names could either be
> handled by adding new parameters to the end of the InquiryResult signal,
> or then by creating a new ExtendendInquiryResult signal. We might want
> to leave the "alias name" feature to be handled by higher SW layers.
> E.g. gnome-bluetooth has some support for storing the alias name under
> the GConf path /system/bluetooth/device/<bd addr>/alias.

I think about renaming the InquiryResult into something more meaningful
like RemoteDevice or something. This signal should include the name from
the extended inquiry or a cached name or an empty string. We might also
include the list of UUIDs and some vendor information. The future is the
extended inquiry and we should plan the D-Bus interface around it. Any
comments?

Regards

Marcel




-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

2005-10-20 09:50:35

by Johan Hedberg

[permalink] [raw]
Subject: Re: [Bluez-devel] [PATCH] Make RemoteName D-BUS method non-blocking

On Thu, Oct 20, 2005, Marcel Holtmann wrote:
> I am not that big in D-Bus programming, but I think that is how it
> should work. The failed signal then can contain an error code or we
> simply use the error method for it.

Ok. I'll create a new RemoteNameFailed signal which has one byte
parameter (the status code). Do you want yet another patch against the
current CVS HEAD, or should I wait for you to commit my previous patch
and then create a new patch against the new CVS revision?

> Another thing is that we might wanna have a method where we can request
> a name from the cache and if not available we request it.

Maybe this could be done by adding a boolean "use cache" parameter to
the RemoteName method?

> Besides this we also should keep the extended inquiry in mind. This can
> deliver us the full name via an inquiry response or at least a short
> name for a device. So what I see is we have four different types of
> device names inside our system.
>
> - Full device names (up to 248 characters)
> - Short names or inquiry names (up to around 235 at max)
> - Cached names (in generell only up to 248 characters)
> - Alias names (no length requirement)
> How do deal with them through a nice interface, because all of them can
> be different.

I think that the extendend inquiry response names could either be
handled by adding new parameters to the end of the InquiryResult signal,
or then by creating a new ExtendendInquiryResult signal. We might want
to leave the "alias name" feature to be handled by higher SW layers.
E.g. gnome-bluetooth has some support for storing the alias name under
the GConf path /system/bluetooth/device/<bd addr>/alias.

Johan


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

2005-10-20 09:02:57

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [Bluez-devel] [PATCH] Make RemoteName D-BUS method non-blocking

Hi Johan,

> > Do you think the status parameter is needed? Is it not better to send
> > two different responses? One for the success case and one for the
> > failure case.
>
> I'm not sure whether the status parameter is needed, perhaps if someone
> wants to make an UI which shows an accurate error message why the name
> request failed? Anyway, I don't have a strong opinion about it, so if
> you want to I can remove the status parameter and split the signal to
> RemoteName and RemoteNameFailed signals.

I am not that big in D-Bus programming, but I think that is how it
should work. The failed signal then can contain an error code or we
simply use the error method for it.

Another thing is that we might wanna have a method where we can request
a name from the cache and if not available we request it.

Besides this we also should keep the extended inquiry in mind. This can
deliver us the full name via an inquiry response or at least a short
name for a device. So what I see is we have four different types of
device names inside our system.

- Full device names (up to 248 characters)
- Short names or inquiry names (up to around 235 at max)
- Cached names (in generell only up to 248 characters)
- Alias names (no length requirement)

How do deal with them through a nice interface, because all of them can
be different.

Regards

Marcel




-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

2005-10-20 08:37:03

by Johan Hedberg

[permalink] [raw]
Subject: Re: [Bluez-devel] [PATCH] Make RemoteName D-BUS method non-blocking

On Thu, Oct 20, 2005, Marcel Holtmann wrote:
> > > Why don't you set rq.event to EVT_CMD_STATUS?
> >
> > Because hci_send_req has a switch statement which handles the
> > EVT_CMD_STATUS case separately and would never reach the case where it
> > tests for hdr->evt == r->event.
>
> This looks like a bug to me. We should check r->event == EVT_CMD_STATUS
> and then exit nicely.

Yeah, that sounds like the best solution to me. Attached is a patch to
fix it, as well as a new patch for hcid which sets rq.event to
EVT_CMD_STATUS.

> > Attached is a new patch which should apply cleanly to CVS.
>
> Do you think the status parameter is needed? Is it not better to send
> two different responses? One for the success case and one for the
> failure case.

I'm not sure whether the status parameter is needed, perhaps if someone
wants to make an UI which shows an accurate error message why the name
request failed? Anyway, I don't have a strong opinion about it, so if
you want to I can remove the status parameter and split the signal to
RemoteName and RemoteNameFailed signals.

Johan


Attachments:
(No filename) (1.06 kB)
send-req.patch (514.00 B)
remote-name3.patch (5.09 kB)
Download all attachments

2005-10-20 07:52:28

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [Bluez-devel] [PATCH] Make RemoteName D-BUS method non-blocking

Hi Johan,

> > Even if I repeat myself, but I take this whitespace thing serious and
> > you should tell your editor to visualize tabs and spaces for you.
>
> I do too. Were there some whitespace problems with my patch? (I couldn't
> find any)

I think your patches were fine. It was Claudio introducing them again
after I cleaned them up.

> > Why don't you set rq.event to EVT_CMD_STATUS?
>
> Because hci_send_req has a switch statement which handles the
> EVT_CMD_STATUS case separately and would never reach the case where it
> tests for hdr->evt == r->event.

This looks like a bug to me. We should check r->event == EVT_CMD_STATUS
and then exit nicely.

> Attached is a new patch which should apply cleanly to CVS.

Do you think the status parameter is needed? Is it not better to send
two different responses? One for the success case and one for the
failure case.

Regards

Marcel




-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

2005-10-20 07:05:32

by Johan Hedberg

[permalink] [raw]
Subject: Re: [Bluez-devel] [PATCH] Make RemoteName D-BUS method non-blocking

Hi Marcel,

On Thu, Oct 20, 2005, Marcel Holtmann wrote:
> Even if I repeat myself, but I take this whitespace thing serious and
> you should tell your editor to visualize tabs and spaces for you.

I do too. Were there some whitespace problems with my patch? (I couldn't
find any)

> Why don't you set rq.event to EVT_CMD_STATUS?

Because hci_send_req has a switch statement which handles the
EVT_CMD_STATUS case separately and would never reach the case where it
tests for hdr->evt == r->event.

Attached is a new patch which should apply cleanly to CVS.

Johan


Attachments:
(No filename) (563.00 B)
remote-name2.patch (4.39 kB)
Download all attachments

2005-10-19 22:58:56

by Marcel Holtmann

[permalink] [raw]
Subject: Re: [Bluez-devel] [PATCH] Make RemoteName D-BUS method non-blocking

Hi Johan,

> Here's a patch which does a similar change RemoteName as my previous
> patch did to Inquiry. For the user to be able to know if the name
> request fails because of e.g. page timeout I have added a status
> paramer to the signal and forced the signal to be always sent (not just
> when evt->status == 0).

I like to have this patch in, but it didn't apply cleanly. This might be
my fault, because I did some coding style cleanups while I was without
Internet and committed them today.

Even if I repeat myself, but I take this whitespace thing serious and
you should tell your editor to visualize tabs and spaces for you. This
will make developing a lot easier.

> There is one problem when using hci_send_req with the inquiry and remote
> name HCI commands: they don't generate a command complete event, only a
> command status event. This means that hci_send_req will fail with
> ETIMEDOUT (I didn't notice it earlier since the UI status is immediately
> updated by the InquiryStarted signal). Using hci_send_cmd is also not
> (IMHO) an option since a failure status in the command status event
> should be detected and handled properly.

Why don't you set rq.event to EVT_CMD_STATUS?

Regards

Marcel




-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

2005-10-19 17:45:49

by Johan Hedberg

[permalink] [raw]
Subject: Re: [Bluez-devel] [PATCH] Make RemoteName D-BUS method non-blocking

Hi,

On Wed, Oct 19, 2005, P. Durante wrote:
> - receive the dbus method_call asking to perform some hci command
> - save the call_message somewhere ( a pointer )
> - fill the hci command packet
> - save a structure containing the packet and the aforementioned
> pointer in a queue ( this structure contains other things, like a
> timeout object to know when the command didn't complete in time, but
> let's get over that )
> - send the hci command packet
> - move it to another queue
> - return to the main loop
> [you could use the glib main loop if you like ( or that 'scary'
> glib-ectomy in bluez-utils :) but I wanted to avoid such a dependency
> for a daemon and wrote my own main loop adapter.]
>
> when some data is ready on the hci control socket ( you have one of
> those for each active device )
> - read the generated event
> - see if it corresponds to one of the pending commands in the queue (
> this part includes some dirty magic to avoid messing things up if you
> send twice the same command )
> - if so, read the event data and send the method_reply over dbus (
> using the pointer you saved before )

What you describe is pretty much as I have implemented the bluetooth
D-BUS applications that are part of maemo (http://www.maemo.org, osso-bttools
and osso-gwconnect packages). For those I had the luxury of using glib
so doing things asynchronously was quite simple.

The results of the Inquiry and RemoteName methods are already sent as
D-BUS signals, so it doesn't necessarely make sense to send the
information a second time in the D-BUS method returns. However, there
will for sure come scerarios where we need to store information in some
queue, return to the mainloop, and retreive the information from the
queue when the appropriate event comes.

Since I don't have a very clear picture of what kind of mainloop Marcel
would like to use when we move the development over to bluetoothd from
hcid, I have intentionally tried to stay away from touching features
which would require doing things asychronously as you describe ;-)

Johan


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel

2005-10-19 15:03:34

by P. Durante

[permalink] [raw]
Subject: Re: [Bluez-devel] [PATCH] Make RemoteName D-BUS method non-blocking

Hi Johan,

I've also written a bluetooth daemon with dbus support and have
stumbled in exactly the same problem a couple of months ago, the
blocking nature of the bluez api has been a great obstacle for me, of
course I could not accept that the daemon became completely
unresponsive while waiting for a lengthy command ( like an inquiry or
a remote_name_request ) to complete, for this reason I could not use
hci_send_req ( and sdp_send_req_w4_rsp, but that's for another post )
and had to rewrite all the functions using them in a sane ( at least
for what I was trying to do ) way, here's more or less my approach:

- receive the dbus method_call asking to perform some hci command
- save the call_message somewhere ( a pointer )
- fill the hci command packet
- save a structure containing the packet and the aforementioned
pointer in a queue ( this structure contains other things, like a
timeout object to know when the command didn't complete in time, but
let's get over that )
- send the hci command packet
- move it to another queue
- return to the main loop
[you could use the glib main loop if you like ( or that 'scary'
glib-ectomy in bluez-utils :) but I wanted to avoid such a dependency
for a daemon and wrote my own main loop adapter.]

when some data is ready on the hci control socket ( you have one of
those for each active device )
- read the generated event
- see if it corresponds to one of the pending commands in the queue (
this part includes some dirty magic to avoid messing things up if you
send twice the same command )
- if so, read the event data and send the method_reply over dbus (
using the pointer you saved before )

that's it!

There are surely better ways to do it, ( and I'm looking for some now
that I'm rewriting the whole thing ) but I hope this can be a starting
point for a discussion.

regards


-------------------------------------------------------
This SF.Net email is sponsored by:
Power Architecture Resource Center: Free content, downloads, discussions,
and more. http://solutions.newsforge.com/ibmarch.tmpl
_______________________________________________
Bluez-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/bluez-devel