On Mon, 2011-12-12 at 16:46 +0100, Dalleau, Frederic wrote: > Hi Tanu, > > >> Im fine with the changes except with this thing with > >> org.bluez.FMRadio, IMO that should belong to the module that requires > >> it > > > > It's a valid argument that "special" stuff like the FMRadio interface > > adds clutter in the generic bluetooth-util.h API. But if it's > > implemented separately, either it will reimplement a lot of stuff > > already done by bluetooth-util, or alternatively bluetooth-util.h will > > need to provide an API for extending the functionality, and that would > > add another kind of clutter. I believe the extension API would be worse > > than having FMRadio directly in bluetooth-util. > > > > Another argument is that having FMRadio in bluetooth-util causes > > overhead in systems where the WL1273 chip isn't used. There is some > > additional memory required, which I think is negligible, but there's > > also useless D-Bus communication involved, which I think is a > > significant drawback. > > An intermediate position can be that bluetooth-util.c notifies > module-wl1273-support that an adapter has been added or removed > via hooks similar to what already exists. > If this happens, module-wl1273-support could simply look for the > org.bluez.FMRadio interface by itself. > This way, if module-wl1273-support is not loaded, then no > overhead is added, and if it is loaded, it still benefit the shared architecture > of bluetooth-util.c. That still requires that bluetooth-util.h provides an API for this kind of extensions. But maybe it's not so bad. I haven't thought yet through what exactly is needed, so let's do it now. When a new adapter appears, fmradio-util will have to check if it supports the FMRadio interface. AFAIK, there's no explicit API for checking that, except the introspection API, but I don't want to parse XML. So, the best way to do the check is probably by calling blindly org.bluez.FMRadio.GetProperties. If the FMRadio interface isn't supported, fmradio-util can detect that by looking at the error that bluetoothd will send back. Sending the GetProperties message requires fmradio-util to connect to the system bus, which would have already been done if the functionality was implemented in bluetooth-util. Well, connecting to the system bus is very simple, so here the duplication isn't a big deal. Sending the message requires also the bus name of bluetoothd. It seems that bluetooth-util uses "org.bluez" instead of the unique bus name. That seems suspicious - bluetooth-util caches the state of bluetoothd, so the interaction with the daemon isn't stateless, so it would probably be a better idea to use the unique bus name. Bluetooth-util seems to do a reset whenever the "org.bluez" name owner changes, and that should get rid of most problems. I guess there can be at most some rare corner case problems when bluetoothd restarts and Pulseaudio sends a message after the old instance has died but before the NameOwnerChanged signal has been received. It may also be that that won't be a problem in practice. But worrying about these issues could be avoided entirely if bluetooth-util used the unique bus name. If I'll do that change, then fmradio-util will need to get the current unique bus name from bluetooth-util somehow (or duplicate the name owner tracking). The current bus name could be simply a field in the pa_bluetooth_core struct, so this is pretty harmless. Another thing that is required is the adapter object path. That can be made available as a field in pa_bluetooth_adapter. I don't think calling SetProperty or subscribing to FMRadio property changes will bring any further requirements, so the "extension API" that I was worried about is actually very small and simple to implement. So, I'm not against separating the FMRadio handling to fmradio-util after all. -- Tanu
