Closed Bug 732639 Opened 12 years ago Closed 12 years ago

Create event loop thread for bluetooth dbus on gonk/linux

Categories

(Core :: DOM: Device Interfaces, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: qdot, Assigned: qdot)

References

Details

Attachments

(1 file, 7 obsolete files)

Since we can't use gtk, we'll need to start and maintain our own dbus event loop thread for receiving signals.
By "signals", you mean dbus notifications right?  (A la Qt "signals".)

This should be pretty easy.  Let's chat about it next week.  The RIL IO thread is a good model for what I think we need here.
Hardware: x86_64 → All
Version: unspecified → Trunk
Yup, just need a thread to run the pump. Main question is getting information from the signal callbacks back to the main thread. I honestly haven't researched it much yet, so it may be more trivial than I think (i.e. if it just gets a DBusMessage object). 

Most of this is implemented in Android in glue/gonk/frameworks/base/core/jni/android_server_BluetoothEventLoop.cpp
You just need to poll a socket, right?  Anything else required?
Yup. Though unlike RIL, this isn't just a simple back and forth, we're going to have to distribute signals/information to different objects. Whether that'll actually effect this, I dunno, still haven't gotten to that part of the research portion yet (I've mainly been working in either gtk, where it's handled for me, or in sync raw dbus calls so far).
This now exists on github in

https://github.com/qdot/mozilla-central/tree/bluetooth-thread

It's a mess, but it works. Will need LOTS of cleaning and libevent-izing before it can land though.
Assignee: nobody → kyle
Attached patch WIP: DBus Thread (obsolete) — Splinter Review
Looking for general feedback on this. 

Things that I see could be done still, not sure if they're needed:

- Fix function names in DBusThread
- Cleaning up signal setup/teardown (possibly making a const char*[] of the signals and looping thru)
- Make socket containers in DBusThread class a list instead of a raw array (works as is for the moment)
Attachment #613488 - Flags: feedback?(jones.chris.g)
Comment on attachment 613488 [details] [diff] [review]
WIP: DBus Thread

Feels a little short of ready for review.  High level approach looks good.  Some things to fix up

 - naming: make sure to |Follow::TheStupid(Style aGuide)|

 - comments: missing some juicy ones on interfaces, especially
   DBusEventHandler.  Would be a good place to stick a brief overview
   of how stuff fits together.

 - make sure to use the new license header for new files

 - it's not 100% clear to me what code is coming from assp vs. what
   code we've written internally.  We can't relicense assp code under
   MPL, so the distinction is important legally in addition to
   review-wise.

 - some of the code is a more C-y than we seem to need here, but
   without knowing what's ours and what's not it's hard for me to
   judge.

 - global nit: nothing in this code is gonk-specific, per se.  I would
   recommend renaming to something like "dbuslite", or "dbike".  Name
   not all that important.

>+  dbus_bus_add_match(mConnection,
>+                     "type='signal',interface='org.freedesktop.DBus'",

This code desparately wants to be a loop over a static data structure.

>+  dbus_bus_remove_match(mConnection,
>+                        "type='signal',interface='org.bluez.AudioSink'",

And here.  (Only bringing these up because they really jumped out at
me.)

Some issues with the rest of the stuff but would rather start with the
higher-level things.
Attachment #613488 - Flags: feedback?(jones.chris.g) → feedback+
Attachment #613488 - Attachment is obsolete: true
Attachment #613907 - Flags: review?(jones.chris.g)
Fixes license header
Attachment #613907 - Attachment is obsolete: true
Attachment #613911 - Flags: review?(jones.chris.g)
Attachment #613907 - Flags: review?(jones.chris.g)
Comment on attachment 613911 [details] [diff] [review]
DBus Thread Implementation for B2G Bluetooth

>diff --git a/ipc/dbus/DBusThread.cpp b/ipc/dbus/DBusThread.cpp

>+#define EVENT_LOOP_EXIT 1
>+#define EVENT_LOOP_ADD  2
>+#define EVENT_LOOP_REMOVE 3
>+

Make this a C++ enum with a descriptive typename, plz.

>+static const PollFdComparator gPollFdComparator = PollFdComparator();
>+

No need to store a global here: just use |PollFdComparator()| when you
need one.  C++ compilers know how to boil that pattern away entirely.

>+static RefPtr<DBusThread> sDBusThread;
>+

You can use nsAutoPtr<> for this.  Since DBusThread references don't
escape this file-static scope, refcounting isn't needed.  (Change
propagates to DBusThread decl too.)

>+void*
>+DBusThread::EventLoop(void *aPtr)
>+{

>+  while (1) {

>+    poll(dbt->mPollData.Elements(), dbt->mPollData.Length(), -1);

OCD nit: Put this at beginning of loop, please.

>+bool
>+DBusThread::IsEventLoopRunning()
>+{
>+  MutexAutoLock lock(mMutex);
>+  bool result = false;
>+  if (mIsRunning) {
>+    result = true;
>+  }
>+
>+  return result;

Erm how about |return mIsRunning;| ?

>diff --git a/ipc/dbus/DBusThread.h b/ipc/dbus/DBusThread.h

>+bool StartDBus();
>+bool StopDBus();

Need some brief comments here: on which threads can Start/Stop() be
called?  Is it safe to call them at any time and in any order?  Etc.

Nit: newline here.

>+}
>+}
>+#endif

>diff --git a/ipc/dbus/RawDBusConnection.h b/ipc/dbus/RawDBusConnection.h

>+// LOGE and free a D-Bus error
>+// Using #define so that __FUNCTION__ resolves usefully
>+#define LOG_AND_FREE_DBUS_ERROR_WITH_MSG(err, msg) \
>+    {   LOG("%s: D-Bus error in %s: %s (%s)", __FUNCTION__, \
>+        dbus_message_get_member((msg)), (err)->name, (err)->message); \
>+         dbus_error_free((err)); }
>+#define LOG_AND_FREE_DBUS_ERROR(err) \
>+    {   LOG("%s: D-Bus error: %s (%s)", __FUNCTION__, \
>+        (err)->name, (err)->message); \
>+        dbus_error_free((err)); }
>+

Let's make a function helper here that does the heavy lifting, to
which these macros route with a resolved __FUNCTION__.  That tends to
be very helpful when trying to break on "any dbus error": break in
your error helper.

>+class RawDBusConnection
>+{

>+  bool createDBusConnection();

Nit: |Create...()|.

>+protected:
>+  DBusConnection* mConnection;

Bare pointers make me nervous ... I assume this is a magic handle that
dbus gives us, that we're not really supposed to manage as a pointer?
(Kind of like xlib magical objects.)  If so, please doc as such.

>diff --git a/toolkit/library/Makefile.in b/toolkit/library/Makefile.in

>@@ -398,6 +402,7 @@ OS_LIBS += \
>   -lcamera_client \
>   -lbinder \
>   -lsensorservice \
>+	-ldbus \

Nit: \t here, use \x20 to match code above.

I like this a lot, overall.  Would like to see the next version.
Attachment #613911 - Flags: review?(jones.chris.g)
I broke the error macros out into DBusUtils.h, as that's where they'd be going anyways, and we're planning to put a lot more functions in there soon. Apache licensed that file, since we'll be lifting a ton more stuff out of android for it most likely.

Other than that, patch is updated with prior review comments implemented.
Attachment #613911 - Attachment is obsolete: true
Attachment #614688 - Flags: review?(jones.chris.g)
Comment on attachment 614688 [details] [diff] [review]
DBus Thread Implementation for B2G Bluetooth

Review of attachment 614688 [details] [diff] [review]:
-----------------------------------------------------------------

Forgot to fix DBusConnection* ptr. Cancelling review.
Attachment #614688 - Flags: review?(jones.chris.g)
Attachment #614688 - Attachment is obsolete: true
Attachment #614692 - Flags: review?(jones.chris.g)
Attachment #614692 - Flags: review?(jones.chris.g) → review+
I just want to point out that function: dbus_threads_init_default(), it's called twice, once in: RawDbusConnection::Create(), and again in: DBusThread::SetUpEventLoop(), just after returning from Create().

As D-Bus documentation [1] says, all other calls to this function after the first one, actually does nothing, so it's not a real bug.

[1] http://dbus.freedesktop.org/doc/api/html/group__DBusThreads.html#ga33b6cf3b8f1e41bad5508f84758818a7
This was already reviewed once, but:

- Unsubmittable due to collision with gb emulator
- Had major bug where mControlFdW wasn't actually initialized
- Bitrotted in a very short period due to .mFd->.get() change in ScopedClose

So, getting a quick re-review just in case. Can't land until gonk-gb on B2G server is updated, too.
Attachment #614692 - Attachment is obsolete: true
Attachment #618036 - Flags: review?(jones.chris.g)
Attachment #618036 - Attachment is obsolete: true
Attachment #618036 - Flags: review?(jones.chris.g)
Adding dependency to toolchain update bug, since I rolled dbus inclusion into the cpufeatures update.
Depends on: 748448
Removed some unneeded ifdefs. 

Last update before this was fixing a botched merge that brought back some old jstypedarray stuff.
Attachment #618547 - Attachment is obsolete: true
Attachment #618549 - Flags: review?(jones.chris.g)
Comment on attachment 618549 [details] [diff] [review]
DBus Thread Implementation for B2G Bluetooth

Skimmed but looks OK to me.
Attachment #618549 - Flags: review?(jones.chris.g) → review+
Backed out due to bustage:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a0b7a536677

https://tbpl.mozilla.org/php/getParsedLog.php?id=11541953&tree=Mozilla-Inbound
configure: loading cache /builds/slave/m-in-b2g/build/obj-b2g/tools/profiler/libunwind/src/config.cache
configure: warning: ignoring whitespace changes in `LIBS' since the previous run:
configure:   former value:  ` -lstlport '
configure:   current value: ` -lstlport'
configure: error: `CPPFLAGS' has changed since the previous run:
configure:   former value:  `-DANDROID -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/include/ -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/kernel/common -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/arch-arm/include -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/kernel/arch-arm -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libm/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/opengl/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/native/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/hardware/libhardware/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/hardware/libhardware_legacy/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/system/core/include -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/services/sensorservice'
configure:   current value: `-DANDROID -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/include/ -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/kernel/common -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/arch-arm/include -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/kernel/arch-arm -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libm/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/opengl/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/native/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/hardware/libhardware/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/hardware/libhardware_legacy/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/system/core/include -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/external/dbus -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/services/sensorservice'
configure: error: in `/builds/slave/m-in-b2g/build/obj-b2g/tools/profiler/libunwind/src':
configure: error: changes in the environment can compromise the build
configure: error: run `make distclean' and/or `rm /builds/slave/m-in-b2g/build/obj-b2g/tools/profiler/libunwind/src/config.cache' and start over
configure: error: /builds/slave/m-in-b2g/build/tools/profiler/libunwind/src/configure failed for tools/profiler/libunwind/src

And re-landed after clobbering all the b2g build slaves. Sorry for the churn.
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f6ba1e4eae2
https://hg.mozilla.org/mozilla-central/rev/6f6ba1e4eae2
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: