**** BEGIN LOGGING AT Mon Feb 15 02:59:57 2010 **** ENDING LOGGING AT Mon Feb 15 12:02:31 2010 **** BEGIN LOGGING AT Mon Feb 15 12:02:51 2010 Feb 15 17:10:23 hi there Feb 15 19:41:40 holtmann: Can you review the gdbus patches? I've some changes pending based on them Feb 15 19:42:41 holtmann: Also have a look at ofono plugins/hfp.c parse_properties_reply, that or something like it might be a good addition to gdbus Feb 15 19:46:12 denkenz: jhe: we still have a issue on HFP, we need to pass the hf features to bluez. I would sugest modify RegisterAgent from (o) to (o,u) to add the features to the call. Feb 15 19:46:31 the bluez can call hfp_hf_record() with the right hf_features Feb 15 19:47:56 denkenz: I prefer we do the split properly like I did in connman/include/dbus.h Feb 15 19:48:19 Properties are a subset of dicts. Feb 15 19:48:35 And we need both. Feb 15 19:49:39 Also I do want the postfixes like basic, array, dict and fixed_array. Feb 15 19:51:48 padovan: The record has to be registered before the agent, no? Feb 15 19:52:23 denkenz: how will we know it before the agent registers itself? Feb 15 19:52:37 the features are implemeted by the agent Feb 15 19:53:01 padovan: There are two layers there, the BRSF feature exchange also carriers much about the capabilities Feb 15 19:53:20 holtmann: Not sure I understand, there are properties, dict and signal functions Feb 15 19:56:15 denkenz: I can't connect from my G1 phone if the features are not exported by bluez. Feb 15 19:56:38 padovan: And really the record doesn't give much, I'd say simply set it to something that makes sense Feb 15 19:57:15 padovan: e.g. EC/NR off, Call Waiting on, CLI on, voice recognition off and Remote volume control on Feb 15 19:59:37 denkenz: zhenhua has a patch for that. The problem is that bluez will not show the real features suported by the agent. But BRSF can deal with that. Feb 15 20:02:13 I am pretty happy with the implementation in connman/src/dbus.c that nicely implements property helpers and then dict helpers using these property helpers. Feb 15 20:09:28 holtmann: Honestly I see no difference, connman_dbus_property_append_basic vs g_dbus_properties_dict_append Feb 15 20:09:55 There are not. I am just talking about the API naming. Feb 15 20:10:22 However there is a difference between a pure property and a property as part of a dict. Feb 15 20:10:23 holtmann: So what do you want it to say, g_dbus_property_append? Feb 15 20:10:27 You need an extra container. Feb 15 20:10:43 And as I said above, I want the append_basic, append_array, etc. stuff. Feb 15 20:11:10 Similar to D-Bus itself differs between basic types etc. Feb 15 20:12:32 holtmann: I suggest you comment on the ML then, with the exception of basic they already have proper names Feb 15 20:13:16 You don't get what I am saying. You have to differentiate between a property as part of a dict. And a pure property. Feb 15 20:13:53 Neither do you though... Feb 15 20:14:00 connman API is nearly identical Feb 15 20:14:54 Also, there are no usecases where we append without a dict entry, for those we can use straight dbus functions Feb 15 20:15:46 padovan: Again, I think this is not a per-agent fix anyway Feb 15 20:16:05 padovan: You can have multiple agent types in the system, yet only 1 SDP record / adapter Feb 15 20:16:22 padovan: IMO this should be a config file option for the system integrator Feb 15 20:19:31 holtmann: Send me the API spec you want, I'm dense today Feb 15 20:19:32 denkenz: There are use cases. And one is the SetProperty call. Feb 15 20:19:59 SetProperty parses them, so not sure how that works Feb 15 20:20:43 In ConnMan we have plugins that use another daemons D-Bus API. So in that case you need to call SetProperty. Feb 15 20:21:02 The oFono plugin being one of them btw. Feb 15 20:21:22 The SetProperty method call is just using a property. Not a dict of properties. Feb 15 20:21:36 So for that one we need a third set of methods Feb 15 20:21:59 No you don't. ConnMan has these already nicely wrapped up. Feb 15 20:22:12 dbus_message_iter_init_append(message, &iter); Feb 15 20:22:12 connman_dbus_property_append_basic(&iter, "Powered", Feb 15 20:22:12 DBUS_TYPE_BOOLEAN, &powered); Feb 15 20:22:16 For example like this. Feb 15 20:23:02 And yes, I have written a proper dbus_property_set() function with callbacks and everything, but you need the basics. Otherwise you duplicate the code. Feb 15 20:23:21 We already have the basics Feb 15 20:23:31 My point is, I don't like the iter to be passed in Feb 15 20:23:43 I'd rather have g_dbus_send_set_property(key, property) Feb 15 20:23:44 What is wrong with using the iter? Feb 15 20:24:01 Because you write the same damn code all the time Feb 15 20:24:07 You can always do a set_property_basic() method. Feb 15 20:24:25 That's my point, lets do g_dbus_set_property* methods Feb 15 20:24:35 instead of append_foo Feb 15 20:24:39 The iter version is needed for the cases where you have a pretty complex D-Bus API. For example wpa_supplicant and PolicyKit. Feb 15 20:25:13 You still need the basic append version first. Feb 15 20:26:06 gboolean g_dbus_properties_dict_append(DBusMessageIter *dict, Feb 15 20:26:07 + const char *key, Feb 15 20:26:09 + int type, void *value); Feb 15 20:26:19 If I rename dict to iter, it is the same thing... Feb 15 20:27:27 If you do g_dbus_dict_append_basic(...) it looks fine. Feb 15 20:28:01 so you also want g_dbus_append_basic() that doesn't wrap it in a dict Feb 15 20:28:06 And it should use g_dbus_property_append_basic(...), but just wrap a dict container around it. Feb 15 20:28:40 Exactly. Since we will need that for the cases where we have to use properties, but don't need the dict container. For example SetProperty. Feb 15 20:29:13 Or even the PropertyChanged signal. Feb 15 20:29:18 still seems like SetProperty can be handled by a dedicated function, there's serious API creep here Feb 15 20:29:51 You can have a function for set property, but still you will need this abstraction. Otherwise you duplicate your code. Feb 15 20:30:23 not really, I already have append_*_variant functions Feb 15 20:30:31 they wrap the variants Feb 15 20:30:37 Especially for the cases where you have dict of dict or complex array, you need this to stay sane. Feb 15 20:31:11 g_dbus_properties_dict_append -> g_dbus_properties_append_basic? Feb 15 20:31:32 They are not the same. Feb 15 20:31:44 One has a dict container around it, the otherone doesn't. Feb 15 20:31:57 I know Feb 15 20:32:14 the singular should be property then Feb 15 20:32:27 Yes. Feb 15 20:32:36 g_dbus_property_append_basic and g_dbus_properties_append_basic Feb 15 20:32:40 yuck Feb 15 20:33:02 g_dbus_dict_append_basic(...) Feb 15 20:33:12 Then you don't have that yuck. Feb 15 20:33:18 And it is way shorter. Feb 15 20:33:25 what kind of dict though? Feb 15 20:33:38 our dicts are extremely specific Feb 15 20:33:51 A dict is always a key/value pair type of thing. Feb 15 20:34:05 sure, but ours are string,variant Feb 15 20:34:10 not some random dict Feb 15 20:34:38 hence the point of using properties_dict everywhere Feb 15 20:35:21 string/variant is actually the default. That you can potentially build different ones, is a mistake in the D-Bus specification. Feb 15 20:35:43 I'm sure there will be other opinions Feb 15 20:36:15 I am sure on that, too. However we mostly deal with string/variant. Or did we messed that up and have other ones? Feb 15 20:36:43 We don't, but g_dbus is used by too many projects now, might as well treat it as a proper library Feb 15 20:37:06 which is why I suggest being explicit about what the function does Feb 15 20:37:58 Also, we're arguing about sillyness anyway, the signal version (which doesn't wrap a dict) looks like this: Feb 15 20:38:05 dbus_message_iter_init_append(signal, &iter); Feb 15 20:38:06 + dbus_message_iter_append_basic(&iter, DBUS_TYPE_STRING, &name); Feb 15 20:38:08 + append_variant(&iter, type, value); Feb 15 20:38:09 + Feb 15 20:38:11 + return g_dbus_send_message(conn, signal); Feb 15 20:38:29 all the code is being reused anyway Feb 15 20:40:39 And you could just use g_dbus_property_append_basic() instead of iter_append_basic() + append_variant(). Feb 15 20:42:01 I could, but I don't see the point in exposing append_variant/iter_append_basic Feb 15 20:47:56 holtmann: imo we should add g_dbus_send_set_property_basic(service, path, interface, key, type, value) Feb 15 20:48:13 holtmann: This will take care of all present usecases Feb 15 20:48:32 Okay. However set_property needs a callback. Feb 15 20:48:56 holtmann: Sure enough, and probably the autostart flag too Feb 15 20:49:14 Keep the autostart flag to false all the time. Feb 15 20:49:20 That thing is pretty much annoying. Feb 15 20:49:36 For system services it is a pretty bad idea. Feb 15 20:50:15 holtmann: sounds good, so I can just do another patch with g_dbus_send_set_property_basic Feb 15 20:50:16 If your D-Bus services dies, you need to redo everything anyway since you loose all the states. There is no clean way to just autostart the daemon. Feb 15 20:50:27 Remove the "send". Feb 15 20:50:44 nod Feb 15 20:51:42 so nitpick the wording on the current patch so I can resubmit and get it pushed Feb 15 20:52:37 padovan: Feb 15 20:52:39 +static void hfp_disconnected_cb(gpointer user_data) Feb 15 20:52:40 +{ Feb 15 20:52:42 + struct ofono_modem *modem = user_data; Feb 15 20:52:43 + struct hfp_data *data = ofono_modem_get_data(modem); Feb 15 20:52:45 + Feb 15 20:52:46 + ofono_modem_set_powered(modem, FALSE); Feb 15 20:52:48 + Feb 15 20:52:49 + g_at_chat_unref(data->chat); Feb 15 20:52:51 + data->chat = NULL; Feb 15 20:52:52 +} Feb 15 20:52:54 + Feb 15 20:52:55 should the last part simply be clear_data()? Feb 15 21:19:42 denkenz: hum.. it should. I'll fix that. Feb 15 22:46:46 padovan: Ok, I fixed it for you, pushed **** ENDING LOGGING AT Tue Feb 16 02:59:57 2010