Last Comment Bug 732639 - Create event loop thread for bluetooth dbus on gonk/linux
: Create event loop thread for bluetooth dbus on gonk/linux
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla15
Assigned To: Kyle Machulis [:kmachulis] [:qdot]
:
:
Mentors:
Depends on: 748448
Blocks: b2g-bluetooth
  Show dependency treegraph
 
Reported: 2012-03-02 17:19 PST by Kyle Machulis [:kmachulis] [:qdot]
Modified: 2012-05-08 03:18 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP: DBus Thread (27.22 KB, patch)
2012-04-09 21:32 PDT, Kyle Machulis [:kmachulis] [:qdot]
cjones.bugs: feedback+
Details | Diff | Splinter Review
DBus Thread Implementation for B2G Bluetooth (20.52 KB, patch)
2012-04-11 01:38 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Splinter Review
DBus Thread Implementation for B2G Bluetooth (21.11 KB, patch)
2012-04-11 01:53 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Splinter Review
DBus Thread Implementation for B2G Bluetooth (23.88 KB, patch)
2012-04-12 22:57 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Splinter Review
DBus Thread Implementation for B2G Bluetooth (24.12 KB, patch)
2012-04-12 23:28 PDT, Kyle Machulis [:kmachulis] [:qdot]
cjones.bugs: review+
Details | Diff | Splinter Review
DBus Thread Implementation for B2G Bluetooth (35.74 KB, patch)
2012-04-24 13:59 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Splinter Review
DBus Thread Implementation for B2G Bluetooth (33.89 KB, patch)
2012-04-25 21:12 PDT, Kyle Machulis [:kmachulis] [:qdot]
no flags Details | Diff | Splinter Review
DBus Thread Implementation for B2G Bluetooth (33.84 KB, patch)
2012-04-25 21:38 PDT, Kyle Machulis [:kmachulis] [:qdot]
cjones.bugs: review+
Details | Diff | Splinter Review

Description Kyle Machulis [:kmachulis] [:qdot] 2012-03-02 17:19:13 PST
Since we can't use gtk, we'll need to start and maintain our own dbus event loop thread for receiving signals.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-03 07:38:33 PST
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.
Comment 2 Kyle Machulis [:kmachulis] [:qdot] 2012-03-03 09:29:20 PST
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
Comment 3 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-05 13:20:57 PST
You just need to poll a socket, right?  Anything else required?
Comment 4 Kyle Machulis [:kmachulis] [:qdot] 2012-03-05 13:36:03 PST
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).
Comment 5 Kyle Machulis [:kmachulis] [:qdot] 2012-04-03 13:29:55 PDT
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.
Comment 6 Kyle Machulis [:kmachulis] [:qdot] 2012-04-09 21:32:22 PDT
Created attachment 613488 [details] [diff] [review]
WIP: DBus Thread

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)
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-10 03:33:50 PDT
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.
Comment 8 Kyle Machulis [:kmachulis] [:qdot] 2012-04-11 01:38:17 PDT
Created attachment 613907 [details] [diff] [review]
DBus Thread Implementation for B2G Bluetooth
Comment 9 Kyle Machulis [:kmachulis] [:qdot] 2012-04-11 01:53:25 PDT
Created attachment 613911 [details] [diff] [review]
DBus Thread Implementation for B2G Bluetooth

Fixes license header
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-12 21:13:09 PDT
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.
Comment 11 Kyle Machulis [:kmachulis] [:qdot] 2012-04-12 22:57:11 PDT
Created attachment 614688 [details] [diff] [review]
DBus Thread Implementation for B2G Bluetooth

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.
Comment 12 Kyle Machulis [:kmachulis] [:qdot] 2012-04-12 23:00:26 PDT
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.
Comment 13 Kyle Machulis [:kmachulis] [:qdot] 2012-04-12 23:28:03 PDT
Created attachment 614692 [details] [diff] [review]
DBus Thread Implementation for B2G Bluetooth
Comment 14 Kyle Machulis [:kmachulis] [:qdot] 2012-04-13 23:07:01 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/db5d4c1aece7
Comment 17 Juan Gomez [:_AtilA_] (CET/CEST) 2012-04-17 15:25:05 PDT
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
Comment 18 Kyle Machulis [:kmachulis] [:qdot] 2012-04-24 13:59:19 PDT
Created attachment 618036 [details] [diff] [review]
DBus Thread Implementation for B2G Bluetooth

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.
Comment 19 Kyle Machulis [:kmachulis] [:qdot] 2012-04-25 21:12:29 PDT
Created attachment 618547 [details] [diff] [review]
DBus Thread Implementation for B2G Bluetooth
Comment 20 Kyle Machulis [:kmachulis] [:qdot] 2012-04-25 21:34:16 PDT
Adding dependency to toolchain update bug, since I rolled dbus inclusion into the cpufeatures update.
Comment 21 Kyle Machulis [:kmachulis] [:qdot] 2012-04-25 21:38:31 PDT
Created attachment 618549 [details] [diff] [review]
DBus Thread Implementation for B2G Bluetooth

Removed some unneeded ifdefs. 

Last update before this was fixing a botched merge that brought back some old jstypedarray stuff.
Comment 22 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-04-30 23:25:35 PDT
Comment on attachment 618549 [details] [diff] [review]
DBus Thread Implementation for B2G Bluetooth

Skimmed but looks OK to me.
Comment 23 Kyle Machulis [:kmachulis] [:qdot] 2012-05-07 14:05:47 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/7a8607523522
Comment 24 Ryan VanderMeulen [:RyanVM] 2012-05-07 15:15:45 PDT
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
Comment 25 Ed Morley [:emorley] 2012-05-08 03:18:14 PDT
https://hg.mozilla.org/mozilla-central/rev/6f6ba1e4eae2

Note You need to log in before you can comment on or make changes to this bug.