Skip to contentSkip to conversation

Phonebook functions for BlueZ

PATCH Felix Huber 4 participants 7 messages Feb 28, 2010 – Mar 9, 2010 Bluetooth drivers

lore
Share & tools
lore

Felix Huber

author
#0 lore raw

Hello,

I have analyzed the data traffic of my car handsfree set and implemented the functions for the phonebook retrieval via the CPBR and CPBS commands. In addition, I added a telephony driver for the FSO middleware with which I tested the new functions using a Parrot CK3100 car kit and a Jabra ear plug using version 4.61 of BlueZ. The other telephony drivers just have empty functions that reject the new commands in order not to break the APIs.

Attached is the patch file for the git repository.

Felix Huber

Quoted 1 line

From a1e5b0d81b36242ac1a4c7362dd5146395bd7f1f Mon Sep 17 00:00:00 2001

From: Felix Huber <[email protected]>
Date: Fri, 26 Feb 2010 12:15:31 +0100
Subject: [PATCH] Added support for HFP phonebook functions and FSO middleware

---
 audio/headset.c         |   80 +++
 audio/telephony-dummy.c |   11 +
 audio/telephony-fso.c   | 1411 +++++++++++++++++++++++++++++++++++++++++++++++
 audio/telephony-maemo.c |   10 +
 audio/telephony-ofono.c |   11 +
 audio/telephony.h       |   12 +
 6 files changed, 1535 insertions(+), 0 deletions(-)
 create mode 100644 audio/telephony-fso.c
audio/headset.c +80 -0
index 7002a3a..442a73d 100644
@@ -1154,6 +1154,84 @@ static int voice_dial(struct audio_device *device, const char *buf)
 	return 0;
 }
 
+int get_ATtype(const char *buf, int *offset)
+{
+	const char *ATquery = "=?", *ATcheck ="?", *ATset = "=";
+
+	if (!strncmp(buf, ATquery, 2)) {
+		*offset = 2;
+		return (ATQUERY);
+	} else if (!strncmp(buf, ATcheck, 1)) {
+		*offset = 1;
+		return (ATCHECK);
+	} else if (!strncmp(buf, ATset, 1)) {
+		*offset = 1;
+		return (ATSET);
+	} else {
+		*offset = 0;
+		return (ATNONE);
+	}
+}
+
+
+int telephony_phonebook_storage_rsp(void *telephony_device, cme_error_t err)
+{
+	return telephony_generic_rsp(telephony_device, err);
+}
+
+int telephony_phonebook_storage_ind(void *telephony_device, const char *storagelist)
+{
+	struct audio_device *device = telephony_device;
+	struct headset *hs = device->headset;
+
+	headset_send(hs, "\r\n+CPBS: %s \r\n", storagelist);
+
+	return 0;
+}
+
+static int phonebook_storage(struct audio_device *device, const char *buf)
+{
+	int ATtype, offset;
+
+	if (strlen(buf) < 8) {
+		return -EINVAL;
+	}
+
+	ATtype = get_ATtype(&buf[7], &offset);
+	telephony_phonebook_storage_req(device, &buf[7+offset], ATtype);
+
+	return 0;
+}
+
+int telephony_phonebook_read_rsp(void *telephony_device, cme_error_t err)
+{
+	return telephony_generic_rsp(telephony_device, err);
+}
+
+int telephony_phonebook_read_ind(void *telephony_device, const char *entrylist)
+{
+	struct audio_device *device = telephony_device;
+	struct headset *hs = device->headset;
+
+	headset_send(hs, "\r\n+CPBR: %s\r\n", entrylist);
+
+	return 0;
+}
+
+static int phonebook_read(struct audio_device *device, const char *buf)
+{
+	int ATtype, offset;
+
+	if (strlen(buf) < 8) {
+		return -EINVAL;
+	}
+	ATtype = get_ATtype(&buf[7], &offset);
+	telephony_phonebook_read_req(device, &buf[7+offset], ATtype);
+
+	return 0;
+}
+
+
 static struct event event_callbacks[] = {
 	{ "ATA", answer_call },
 	{ "ATD", dial_number },

@@ -1175,6 +1253,8 @@ static struct event event_callbacks[] = {
 	{ "AT+COPS", operator_selection },
 	{ "AT+NREC", nr_and_ec },
 	{ "AT+BVRA", voice_dial },
+	{ "AT+CPBS", phonebook_storage },
+	{ "AT+CPBR", phonebook_read },
 	{ 0 }
 };
audio/telephony-dummy.c +11 -0
index 2409a49..71d1d2b 100644
@@ -225,6 +225,17 @@ void telephony_key_press_req(void *telephony_device, const char *keys)
 	telephony_key_press_rsp(telephony_device, CME_ERROR_NONE);
 }
 
+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)
+{
+	telephony_phonebook_read_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+void telephony_phonebook_storage_req(void *telephony_device, const char *readindex, int ATtype)
+{
+	telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+
 /* D-Bus method handlers */
 static DBusMessage *outgoing_call(DBusConnection *conn, DBusMessage *msg,
 					void *data)
audio/telephony-fso.c new file +1411 -0

1,411 lines not on this page · raw message

audio/telephony-maemo.c +10 -0
index f7531f3..0d990d1 100644
@@ -922,6 +922,16 @@ void telephony_voice_dial_req(void *telephony_device, gboolean enable)
 	telephony_voice_dial_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
 }
 
+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)
+{
+        telephony_phonebook_read_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+void telephony_phonebook_storage_req(void *telephony_device, const char *readindex, int ATtype)
+{
+        telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
 static void handle_incoming_call(DBusMessage *msg)
 {
 	const char *number, *call_path;
audio/telephony-ofono.c +11 -0
index 3df76c3..6aedf12 100644
@@ -387,6 +387,17 @@ void telephony_key_press_req(void *telephony_device, const char *keys)
 	telephony_key_press_rsp(telephony_device, CME_ERROR_NONE);
 }
 
+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)
+{
+	telephony_phonebook_read_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+void telephony_phonebook_storage_req(void *telephony_device, const char *readindex, int ATtype)
+{
+	telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+
 void telephony_voice_dial_req(void *telephony_device, gboolean enable)
 {
 	debug("telephony-ofono: got %s voice dial request",
audio/telephony.h +12 -0
index 0bc4769..9e4c3d3 100644
@@ -127,6 +127,12 @@ typedef enum {
 	CME_ERROR_NETWORK_NOT_ALLOWED	= 32,
 } cme_error_t;
 
+/* AT command types */
+#define ATNONE  0
+#define ATQUERY 1
+#define ATCHECK 2
+#define ATSET   3
+
 struct indicator {
 	const char *desc;
 	const char *range;

@@ -157,6 +163,8 @@ void telephony_call_hold_req(void *telephony_device, const char *cmd);
 void telephony_nr_and_ec_req(void *telephony_device, gboolean enable);
 void telephony_voice_dial_req(void *telephony_device, gboolean enable);
 void telephony_key_press_req(void *telephony_device, const char *keys);
+void telephony_phonebook_storage_req(void *telephony_device, const char *storage, int ATtype);
+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype);
 
 /* AG responses to HF requests. These are implemented by headset.c */
 int telephony_event_reporting_rsp(void *telephony_device, cme_error_t err);

@@ -173,6 +181,8 @@ int telephony_call_hold_rsp(void *telephony_device, cme_error_t err);
 int telephony_nr_and_ec_rsp(void *telephony_device, cme_error_t err);
 int telephony_voice_dial_rsp(void *telephony_device, cme_error_t err);
 int telephony_key_press_rsp(void *telephony_device, cme_error_t err);
+int telephony_phonebook_storage_rsp(void *telephony_device, cme_error_t err);
+int telephony_phonebook_read_rsp(void *telephony_device, cme_error_t err);
 
 /* Event indications by AG. These are implemented by headset.c */
 int telephony_event_ind(int index);

@@ -188,6 +198,8 @@ int telephony_subscriber_number_ind(const char *number, int type,
 					int service);
 int telephony_call_waiting_ind(const char *number, int type);
 int telephony_operator_selection_ind(int mode, const char *oper);
+int telephony_phonebook_storage_ind(void *device, const char *storagelist);
+int telephony_phonebook_read_ind(void *device, const char *entrylist);
 
 /* Helper function for quick indicator updates */
 static inline int telephony_update_indicator(struct indicator *indicators,
Signature · 1 line
1.6.4.2
#1 lore raw

Hi Felix,

Quoted 9 lines · Felix Huber

I have analyzed the data traffic of my car handsfree set and implemented the functions for the phonebook retrieval via the CPBR and CPBS commands. In addition, I added a telephony driver for the FSO middleware with which I tested the new functions using a Parrot CK3100 car kit and a Jabra ear plug using version 4.61 of BlueZ. The other telephony drivers just have empty functions that reject the new commands in order not to break the APIs.

Attached is the patch file for the git repository.

Thanks for this contribution! In general the patch looks quite ok, however before pushing this upstream there are a some coding style issues that'd need to be fixed:

audio/headset.c static int voice_dial(struct audio_device *device, const char *buf) · line 1159

Quoted 3 lines
audio/headset.c +3 -0
+int get_ATtype(const char *buf, int *offset)
+{
+	const char *ATquery = "=?", *ATcheck ="?", *ATset = "=";

We usually don't use capital letters in any variable or function names. Pre-processor defines are an exception. So use something like get_at_type, at_query, etc. Also, since get_ATtype is only used within headset.c you have to declare it static.

audio/headset.c static int voice_dial(struct audio_device *device, const char *buf) · line 1163

Quoted 3 lines
audio/headset.c +3 -0
+	if (!strncmp(buf, ATquery, 2)) {
+		*offset = 2;
+		return (ATQUERY);

No () around the value given to return;

audio/headset.c static int voice_dial(struct audio_device *device, const char *buf) · line 1176

Quoted 13 lines
audio/headset.c +13 -0
+	} else if (!strncmp(buf, ATcheck, 1)) {
+		*offset = 1;
+		return (ATCHECK);
+	} else if (!strncmp(buf, ATset, 1)) {
+		*offset = 1;
+		return (ATSET);
+	} else {
+		*offset = 0;
+		return (ATNONE);
+	}
+}
+
+

There shouldn't at any point be the need to have more than one empty line in the code, so please remove the other.

Quoted 1 line

+int telephony_phonebook_storage_ind(void *telephony_device, const char *storagelist)

This line looks like it's longer than 80 columns. Please split it to two.

audio/headset.c static int voice_dial(struct audio_device *device, const char *buf) · line 1187

Quoted 1 line
audio/headset.c +1 -0
+	headset_send(hs, "\r\n+CPBS: %s \r\n", storagelist);

Why is there a space before the "\r\n"?

Review

Quoted 1 line
(quoted diff) +1 -0
+	int ATtype, offset;

Again the capital letter thing.

audio/headset.c static int voice_dial(struct audio_device *device, const char *buf) · line 1198

Quoted 3 lines
audio/headset.c +3 -0
+	if (strlen(buf) < 8) {
+		return -EINVAL;
+	}

No braces for single-line scopes.

Review

Quoted 1 line
(quoted diff) +1 -0
+	ATtype = get_ATtype(&buf[7], &offset);

No capital letters.

audio/headset.c static int voice_dial(struct audio_device *device, const char *buf) · line 1198

Quoted 3 lines
audio/headset.c +3 -0
+	if (strlen(buf) < 8) {
+		return -EINVAL;
+	}

No braces.

audio/headset.c static int voice_dial(struct audio_device *device, const char *buf) · line 1234

Quoted 7 lines
audio/headset.c +7 -0
+	ATtype = get_ATtype(&buf[7], &offset);
+	telephony_phonebook_read_req(device, &buf[7+offset], ATtype);
+
+	return 0;
+}
+
+

Unnecessary extra empty line.

Quoted 1 line

+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)

Looks like a > 80 column line.

Quoted 1 line

+void telephony_phonebook_storage_req(void *telephony_device, const char *readindex, int ATtype)

Same issue.

audio/telephony-dummy.c void telephony_key_press_req(void *telephony_device, const char *keys) · line 238

Quoted 5 lines
audio/telephony-dummy.c +5 -0
+{
+	telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+

Extra empty line again.

audio/telephony-fso.c line 97

Quoted 2 lines
audio/telephony-fso.c new file +2 -0
+char *fso_categories[NUM_CATEGORIES] = {"contacts", "emergency", "fixed", "dialed", "received", "missed", "own"};
+char *gsm_categories[NUM_CATEGORIES] = {"\"SM\"",   "\"EN\"",    "\"FD\"","\"DC\"", "\"RC\"",   "\"MC\"", "\"ON\""};

Probably you could split these to fit within 80 columns.

audio/telephony-fso.c line 143

Quoted 5 lines
audio/telephony-fso.c new file +5 -0
+static struct {
+	uint8_t status;         // act  'GSM'
+	uint32_t cell_id;       // cid  '51FD'
+	uint32_t operator_code; // code '26203'
+	uint16_t lac;           // lac  '233b'

We don't use C++ style comments. So please change to /* .. */. Also, there's an extra space at the end of the "lac" line.

audio/telephony-fso.c line 159

Quoted 12 lines
audio/telephony-fso.c new file +12 -0
+} net = {
+	.status = NETWORK_REG_STATUS_UNREGISTERED,
+	.cell_id = 0,
+	.operator_code = 0,
+	.lac = 0,
+	.mode = NULL,
+	.operator_name = NULL,
+	.registration = NULL,
+	.signals_bar = 0,
+};
+
+

Extra empty line.

audio/telephony-fso.c line 194

Quoted 9 lines
audio/telephony-fso.c new file +9 -0
+static void vc_free(struct voice_call *vc)
+{
+	if (!vc)
+		return;
+	g_free(vc->number);
+	g_free(vc);
+}
+
+

Extra empty line.

audio/telephony-fso.c line 266

Quoted 4 lines
audio/telephony-fso.c new file +4 -0
+static int find_category(char **phonebooks, const char *category)
+{
+	int i;
+	for (i=0; i < NUM_CATEGORIES; i++) {

Space before and after '='.

audio/telephony-fso.c line 267

Quoted 1 line
audio/telephony-fso.c new file +1 -0
+		if (!strcmp(phonebooks[i], category))

The convention has usually been to use == 0 in the case of strcmp for readability.

audio/telephony-fso.c line 272

Quoted 5 lines
audio/telephony-fso.c new file +5 -0
+			return i;
+	}
+	return -1;
+
+}

Why the empty line after the return statement? I'd put it after the for-loop for consistency with the rest of the code base.

Review

Quoted 2 lines
(quoted diff) +2 -0
+
+

Extra empty line.

audio/telephony-fso.c line 316

Quoted 4 lines
audio/telephony-fso.c new file +4 -0
+static int send_method_call(const char *dest, const char *path,
+				const char *interface, const char *method,
+				DBusPendingCallNotifyFunction cb,
+				void *user_data, int type, ...)

Since this and many other functions are shared with (or actually copied from) telephony-maemo.c would it make sense to put them to some shared c-file (it's fine if this is a separate commit later).

audio/telephony-fso.c line 364

Quoted 4 lines
audio/telephony-fso.c new file +4 -0
+	if ((vc = find_vc_with_status(CALL_STATUS_ACTIVE))) {
+	} else if ((vc = find_vc_with_status(CALL_STATUS_DIALING))) {
+	} else if ((vc = find_vc_with_status(CALL_STATUS_INCOMING))) {
+	}

The purpose of this construction isn't imediately clear imo. Wouldn't something like doing specific NULL checks after each find() call be more readable? I.e.

vc = find(...);
if (vc == NULL)

	vc = find(...);
if (vc == NULL)
	vc = find(...);

Alternatively creating something like find_active_call() which would match any of those states could also make the code a bit more readable.

audio/telephony-fso.c line 379

Quoted 7 lines
audio/telephony-fso.c new file +7 -0
+	ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+					FSO_GSMC_INTERFACE,
+					"Release", NULL, NULL,
+					DBUS_TYPE_INT32, &vc->call_index,
+					DBUS_TYPE_INVALID);
+
+

Extra empty line.

audio/telephony-fso.c line 413

Quoted 3 lines
audio/telephony-fso.c new file +3 -0
+void telephony_dial_number_req(void *telephony_device, const char *number)
+{
+	const char *clir, *callType = "voice";

No capital letters in variable names.

audio/telephony-fso.c line 427

Quoted 10 lines
audio/telephony-fso.c new file +10 -0
+#if 0
+	if (!strncmp(number, "*31#", 4)) {
+		number += 4;
+		clir = "enabled";
+	} else if (!strncmp(number, "#31#", 4)) {
+		number += 4;
+		clir =  "disabled";
+	} else
+		clir = "default";
+#endif

Is this really code that you think can be enabled later? If not I'd just remove it instead of having it commented out.

audio/telephony-fso.c line 454

Quoted 5 lines
audio/telephony-fso.c new file +5 -0
+	ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+                        FSO_GSMC_INTERFACE,
+			"SendDtmf", NULL, NULL,
+			DBUS_TYPE_STRING, &tone_string,
+			DBUS_TYPE_INVALID);

Looks like the indentation is somehow screwed up in the first continuation line. In general the rule for indenting continuation lines
is: same indentation for all continuation lines, at least two tabs more than the first line and as much as possible as long as the lines stay less than 80 columns.

audio/telephony-fso.c line 533

Quoted 7 lines
audio/telephony-fso.c new file +7 -0
+	if (vc = find_vc_with_status(CALL_STATUS_INCOMING)) {
+		cmd = act;
+	} else if (vc = find_vc_with_status(CALL_STATUS_ACTIVE)) {
+		cmd = rel;
+	} else if (vc = find_vc_with_status(CALL_STATUS_DIALING)) {
+		cmd = rel;
+	}

No braces for one-line scopes.

audio/telephony-fso.c line 544

Quoted 11 lines
audio/telephony-fso.c new file +11 -0
+	if (cmd) {
+		err = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+					FSO_GSMC_INTERFACE,
+					cmd, NULL, NULL,
+					DBUS_TYPE_INT32, &vc->call_index,
+					DBUS_TYPE_INVALID);
+	}
+	if (err < 0)
+		telephony_key_press_rsp(telephony_device, CME_ERROR_AG_FAILURE);
+        else
+		telephony_key_press_rsp(telephony_device, CME_ERROR_NONE);

Shouldn't it be an error if cmd is NULL? In general doing initializations upon declaration, especially for error variables, should be avoided. Would e.g. having a final "else err = -EINVAL" at the end of the else/else if statement make sense (which would allow removing the initialzation of err to 0?

Also the indentation of the second if-statments else line is with spaces instead of tabs.

Quoted 1 line

+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)

Looks like a possibly grater than 80 columns line.

audio/telephony-dummy.c void telephony_key_press_req(void *telephony_device, const char *keys) · line 238

Quoted 6 lines
audio/telephony-dummy.c +6 -0
+void telephony_phonebook_storage_req(void *telephony_device, const char *readindex, int ATtype)
+{
+	telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+

Extra empty line.

audio/telephony-fso.c line 582

Quoted 4 lines
audio/telephony-fso.c new file +4 -0
+	if (!dbus_message_get_args(reply, NULL,
+				DBUS_TYPE_STRING, &name,
+				DBUS_TYPE_STRING, &number,
+				DBUS_TYPE_INVALID))

Extra space at the end of the last line.

audio/telephony-fso.c line 594

Quoted 10 lines
audio/telephony-fso.c new file +10 -0
+	if (number_type == &subscriber_number) {
+		g_free(subscriber_number);
+		subscriber_number = g_strdup(number);
+	}
+
+done:
+	dbus_message_unref(reply);
+}
+
+

Extra empty line.

audio/telephony-fso.c line 600

Quoted 6 lines
audio/telephony-fso.c new file +6 -0
+static void retrieve_phonebook_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusError err;
+	DBusMessage *reply;
+	DBusMessageIter iter, array;
+	int ret = 0;

Instead of initializing ret upon declaration (and btw, we use "err" instead of "ret" usually) you could set it to 0 right before the done label.
point:

audio/telephony-fso.c line 637

Quoted 3 lines
audio/telephony-fso.c new file +3 -0
+		if ((index >= phonebook_info.first) && (index <= phonebook_info.last)) {
+			info = g_strdup_printf("%d,\"%s\",,\"%s\"", index, number, name);
+			telephony_phonebook_read_ind(phonebook_info.telephony_device, info);

Looks like possibly grater than 80 columns lines.

audio/telephony-fso.c line 648

Quoted 4 lines
audio/telephony-fso.c new file +4 -0
+	if (ret)
+		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_AG_FAILURE);
+	else
+		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_NONE);

Same here.

audio/headset.c static int voice_dial(struct audio_device *device, const char *buf) · line 1181

Quoted 4 lines
audio/headset.c +4 -0
+}
+
+
+

Two extra empty lines.

audio/telephony-fso.c line 658

Quoted 6 lines
audio/telephony-fso.c new file +6 -0
+static void get_phonebook_info_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusError err;
+	DBusMessage *reply;
+	DBusMessageIter iter, iter_entry, array;
+	int ret = 0;

s/ret/err/ and try to rethink how the initialization upon declaration could be avoided.

audio/telephony-fso.c line 659

Quoted 1 line
audio/telephony-fso.c new file +1 -0
+	int min_index=-1, max_index =-1, number_length = -1, name_length = -1;

Missing spaces before and after '='.

audio/telephony-fso.c line 713

Quoted 9 lines
audio/telephony-fso.c new file +9 -0
+		if (g_str_equal(property, "min_index")) {
+			dbus_message_iter_get_basic(&sub, &min_index);
+		} else if (g_str_equal(property, "max_index")) {
+			dbus_message_iter_get_basic(&sub, &max_index);
+		} else if (g_str_equal(property, "number_length")) {
+			dbus_message_iter_get_basic(&sub, &number_length);
+		} else if (g_str_equal(property, "name_length")) {
+			dbus_message_iter_get_basic(&sub, &name_length);
+		}

No braces for one-line scopes.

audio/telephony-fso.c line 648

Quoted 4 lines
audio/telephony-fso.c new file +4 -0
+	if (ret)
+		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_AG_FAILURE);
+	else
+		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_NONE);

Possibly longer than 80-column lines.

+ g_free(str);

audio/telephony-fso.c line 766

Quoted 9 lines
audio/telephony-fso.c new file +9 -0
+	telephony_phonebook_storage_rsp(phonebook_info.telephony_device, CME_ERROR_NONE);
+
+done:
+	dbus_message_unref(reply);
+
+}
+
+
+

Two unnecessary extra empty lines.

Review

Quoted 1 line
(quoted diff) +1 -0
+	int ret = 0;

Same thing as already mentioned.

audio/telephony-fso.c line 799

Quoted 2 lines
audio/telephony-fso.c new file +2 -0
+	gstr = g_string_new("(");
+	for (i=0; i< n_s; i++) {

Missing spaces before and after '=', before '<'. Can you come up with a more descriptive name for the n_s variable please?

audio/telephony-fso.c line 808

Quoted 9 lines
audio/telephony-fso.c new file +9 -0
+		index = find_category(fso_categories, s[i]);
+		if (index != -1) {
+			debug("GSM %d: %s", index, gsm_categories[index]);
+			if (i == 0)
+				g_string_append_printf(gstr, "%s", gsm_categories[index]);
+			else
+				g_string_append_printf(gstr, ",%s", gsm_categories[index]);
+		}
+	}

Looks like some lines go beyone 80-colums. You can avoid this by doing

if (index == -1)

	continue;

which also (imho) makes the code more readable.

audio/telephony-fso.c line 648

Quoted 6 lines
audio/telephony-fso.c new file +6 -0
+done:
+	dbus_message_unref(reply);
+	if (ret)
+		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_AG_FAILURE);
+	else
+		telephony_phonebook_read_rsp(phonebook_info.telephony_device, CME_ERROR_NONE);

Looks like greater than 80-column lines again.

audio/headset.c static int voice_dial(struct audio_device *device, const char *buf) · line 1181

Quoted 4 lines
audio/headset.c +4 -0
+}
+
+
+

Two extra empty lines.

Quoted 1 line

+void telephony_phonebook_storage_req(void *telephony_device, const char *storage, int ATtype)

Greater than 80-column line. No capital letters in variable names.

audio/telephony-fso.c line 827

Quoted 1 line
audio/telephony-fso.c new file +1 -0
+	int ret=0, index;

s/ret/err/, rethink how the initialization upon declaration could be avoided (not to mention the missing spaces before and after '=').

audio/telephony-fso.c line 839

Quoted 7 lines
audio/telephony-fso.c new file +7 -0
+	switch (ATtype) {
+	case ATQUERY:      // =?
+		ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+			FSO_SIM_INTERFACE,
+			"ListPhonebooks",
+			list_phonebooks_reply, NULL,
+			DBUS_TYPE_INVALID);

No C++ comments, the spit continuation lines need more indentation (at least two more than the original line).

audio/telephony-fso.c line 852

Quoted 4 lines
audio/telephony-fso.c new file +4 -0
+	case ATSET:         // =
+		debug("Phonebook request to be %s", storage);
+		index = find_category(gsm_categories, storage);
+		debug ("Phonebook found at %d", index);

No space between debug and (

audio/telephony-fso.c line 857

Quoted 5 lines
audio/telephony-fso.c new file +5 -0
+		if (index == -1)
+			ret = -1;
+		else {
+			phonebook_info.category = index;
+			telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NONE);

Looks like greater than 80-column line.

audio/telephony-fso.c line 863

Quoted 6 lines
audio/telephony-fso.c new file +6 -0
+		}
+		break;
+	default:
+		ret = -1;
+	}
+

Redundant tab-character on this empty line.

audio/telephony-fso.c line 865

Quoted 2 lines
audio/telephony-fso.c new file +2 -0
+	if (ret < 0)
+		telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_AG_FAILURE);

Greater than 80-column line.

audio/headset.c static int voice_dial(struct audio_device *device, const char *buf) · line 1176

Quoted 3 lines
audio/headset.c +3 -0
+}
+
+

Unnecessary empty line.

Quoted 1 line

+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)

Greater than 80 column line.

audio/telephony-fso.c line 872

Quoted 3 lines
audio/telephony-fso.c new file +3 -0
+{
+	int size, ret=0;
+	debug("telephony-fso: got pbook read request: %s", readindex);

Empty line after variable declarations. s/ret/err/. Rethink initialization upon declaration. Missing space before and after '='

Review

Quoted 2 lines
(quoted diff) +2 -0
+	switch (ATtype) {
+	case ATQUERY:          // =?

No C++ comments.

audio/telephony-fso.c line 883

Quoted 6 lines
audio/telephony-fso.c new file +6 -0
+		ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+		FSO_SIM_INTERFACE,
+			"GetPhonebookInfo",
+			get_phonebook_info_reply, NULL,
+			DBUS_TYPE_STRING, &fso_categories[phonebook_info.category],
+			DBUS_TYPE_INVALID);

Incorrect indentation (e.g. first continuation line seems not to be indented at all from the original line)

audio/telephony-fso.c line 891

Quoted 1 line
audio/telephony-fso.c new file +1 -0
+		phonebook_info.first=-1, phonebook_info.last=-1;

Missing spaces before and after '='.

audio/telephony-fso.c line 894

Quoted 3 lines
audio/telephony-fso.c new file +3 -0
+		sscanf(readindex, "%d,%d", &phonebook_info.first, &phonebook_info.last);
+		if (phonebook_info.first == -1)
+			break;

Probably you'd also want to check for sscanf return value (i.e. == 2). Maybe that's the only thing you should check for and not try to initialize these variables here since you already do that in the beginning of telephony-fso.c where you define the phonebook_info struct?

audio/telephony-fso.c line 896

Quoted 2 lines
audio/telephony-fso.c new file +2 -0
+
+		if (phonebook_info.last == -1) phonebook_info.last = phonebook_info.first;

Break this into two lines.

Quoted 12 lines
audio/telephony-fso.c new file +12 -0
+
+		ret = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+			FSO_SIM_INTERFACE,
+			"RetrievePhonebook",
+			retrieve_phonebook_reply, NULL,
+			DBUS_TYPE_STRING, &fso_categories[phonebook_info.category],
+			DBUS_TYPE_INT32, &phonebook_info.first,
+			DBUS_TYPE_INT32, &phonebook_info.last,
+			DBUS_TYPE_INVALID);
+		break;
+
+	default:

audio/telephony-fso.c line 918

Quoted 10 lines
audio/telephony-fso.c new file +10 -0
+		ret = -1;
+	}
+
+	if (ret < 0)
+		telephony_phonebook_read_rsp(telephony_device, CME_ERROR_AG_FAILURE);
+
+}
+
+
+

Unnecessary empty line at the end of the function and two unnecessary empty lines after it.

Quoted 1 line

+static void parse_gsmcall_property(const char *property, DBusMessageIter sub, struct voice_call *vc)

Too long line.

audio/telephony-fso.c line 934

Quoted 12 lines
audio/telephony-fso.c new file +12 -0
+	if (g_str_equal(property, "direction")) {
+		dbus_message_iter_get_basic(&sub, &direction);
+	} else if (g_str_equal(property, "peer")) {
+		dbus_message_iter_get_basic(&sub, &peer);
+		vc->number = g_strdup(peer);
+	} else if (g_str_equal(property, "reason")) {
+		dbus_message_iter_get_basic(&sub, &reason);
+	} else if (g_str_equal(property, "auxstatus")) {
+		dbus_message_iter_get_basic(&sub, &auxstatus);
+	} else if (g_str_equal(property, "line")) {
+		dbus_message_iter_get_basic(&sub, &line);
+	}

No braces for one-line scopes.

audio/headset.c static int voice_dial(struct audio_device *device, const char *buf) · line 1176

Quoted 3 lines
audio/headset.c +3 -0
+}
+
+

Unnecessary empty line.

audio/telephony-fso.c line 959

Quoted 4 lines
audio/telephony-fso.c new file +4 -0
+	if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING) {
+	error("Unexpected signature in gsmcall PropertyChanged signal");
+		return TRUE;
+	}

Incorrect indentation and no braces needed for one-line scope.

audio/telephony-fso.c line 965

Quoted 5 lines
audio/telephony-fso.c new file +5 -0
+	dbus_message_iter_get_basic(&iter, &status);
+
+	debug("**** gsmProp Changed id:%d status: %s", call_index, status);
+
+

Unnecessary empty line.

audio/telephony-fso.c line 970

Quoted 5 lines
audio/telephony-fso.c new file +5 -0
+	vc = find_vc(call_index);
+	if (!vc) {
+		vc = g_new0(struct voice_call, 1);
+	if (!vc)
+		return TRUE;

g_new0 is guaranteed to succeed or else it will call abort() so the NULL check is redundant.

audio/telephony-fso.c line 973

Quoted 3 lines
audio/telephony-fso.c new file +3 -0
+	vc->call_index = call_index;
+	calls = g_slist_append(calls, vc);
+	}

Looks like some incorrect indentation is going on here (since the ending brace has the same indentation as the preceeding lines.

audio/telephony-fso.c line 1001

Quoted 5 lines
audio/telephony-fso.c new file +5 -0
+		if (dbus_message_iter_get_arg_type(&iter_property)
+			!= DBUS_TYPE_STRING) {
+			error("Unexpected signature in gsmcallPropertyChanged signal");
+			return TRUE;
+		}

Incorrect indentation for the split if-line. The continuation line should be indented by at least two tabs to clearly separate it from the actual branch code.

audio/telephony-fso.c line 962

Quoted 6 lines
audio/telephony-fso.c new file +6 -0
+	return TRUE;
+}
+
+
+
+

Three unnecessary empty lines.

audio/headset.c static int voice_dial(struct audio_device *device, const char *buf) · line 1176

Quoted 3 lines
audio/headset.c +3 -0
+}
+
+

Unnecessary empty line.

Review

Quoted 1 line
(quoted diff) +1 -0
+	/* ARRAY -> ENTRY -> VARIANT*/

Space before */

audio/telephony-fso.c line 1142

Quoted 5 lines
audio/telephony-fso.c new file +5 -0
+		dbus_message_iter_recurse(&iter_property, &sub);
+
+		parse_registration_property(property, sub);
+
+                dbus_message_iter_next(&iter_entry);

Incorrect indentation for the last line.

audio/telephony-fso.c line 590

Quoted 3 lines
audio/telephony-fso.c new file +3 -0
+	}
+	
+done:

Redundant tab character on the empty line.

audio/telephony-fso.c line 955

Quoted 4 lines
audio/telephony-fso.c new file +4 -0
+	return TRUE;
+}
+
+

Unnecessary empty line.

audio/telephony-fso.c line 594

Quoted 5 lines
audio/telephony-fso.c new file +5 -0
+done:
+	dbus_message_unref(reply);
+}
+
+

Unnecessary empty line.

audio/telephony-fso.c line 1180

Quoted 5 lines
audio/telephony-fso.c new file +5 -0
+static gboolean handle_registration_property_changed(DBusConnection *conn,
+						DBusMessage *msg, void *data)
+{
+	return (handle_registration_property(msg));
+}

No () needed around the value given to return.

audio/telephony-fso.c line 608

Quoted 3 lines
audio/telephony-fso.c new file +3 -0
+	if (reply_check_error(&err, reply)) {
+		goto done;
+	}

No braces for one-line scope (though I realize this one is probably inherited from telephony-maemo.c which I'm responsible for :)

audio/telephony.h typedef enum { · line 134

Quoted 11 lines
audio/telephony.h +5 -0
@@ -127,6 +127,12 @@ typedef enum {
 	CME_ERROR_NETWORK_NOT_ALLOWED	= 32,
 } cme_error_t;
 
+/* AT command types */
+#define ATNONE  0
+#define ATQUERY 1
+#define ATCHECK 2
+#define ATSET   3

These should probably be an enum and have some namespacing (e.g. AT_TYPE_). To be consistent with the AT command spec terminology (I've been looking at 3GPP TS 07.07) it should be s/QUERY/TEST/ and s/CHECK/READ/.

There were probably other issues in the patch that I missed but it'd be nice if you could fix the already mentioned problems first and then I'll take a second look.

Johan

author
#2 lore raw

Hi Johan,

attached is the beautified version of the patch. I have deleted all the comments referring to space, tabs, extra parenthesis and empty lines since there are silenty fixed. I have a script that should take care of that (at least I thought I had). The rest of the remarks are those where I need feedback, discussion etc...

One more word of warning: Some People seem to have started to use this patch on the OpenMoko. In order to use it, you MUST update a few files of the FSO framework. Look at my commits at the framework git.

Am Montag, den 01.03.2010, 16:20 -0300 schrieb Johan Hedberg:

Quoted 17 lines

Hi Felix,

Quoted 9 lines · Felix Huber

I have analyzed the data traffic of my car handsfree set and implemented the functions for the phonebook retrieval via the CPBR and CPBS commands. In addition, I added a telephony driver for the FSO middleware with which I tested the new functions using a Parrot CK3100 car kit and a Jabra ear plug using version 4.61 of BlueZ. The other telephony drivers just have empty functions that reject the new commands in order not to break the APIs.

Attached is the patch file for the git repository.

Thanks for this contribution! In general the patch looks quite ok, however before pushing this upstream there are a some coding style issues that'd need to be fixed:

Quoted 7 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+int get_ATtype(const char *buf, int *offset)
+{
+	const char *ATquery = "=?", *ATcheck ="?", *ATset = "=";

We usually don't use capital letters in any variable or function names. Pre-processor defines are an exception. So use something like get_at_type, at_query, etc.

Well usually, but since AT in this case is a proper name, I found it very confusing to read it like the prepostion "at" or even a spelled-out @-sign. I added an underscore for better indication of what it is.

Quoted 2 lines

Also, since get_ATtype is only used within
headset.c you have to declare it static.

done

Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+char *fso_categories[NUM_CATEGORIES] = {"contacts", "emergency", "fixed", "dialed", "received", "missed", "own"};
+char *gsm_categories[NUM_CATEGORIES] = {"\"SM\"",   "\"EN\"",    "\"FD\"","\"DC\"", "\"RC\"",   "\"MC\"", "\"ON\""};

Probably you could split these to fit within 80 columns.

Done, but now we loose the one-to-one correspondance.

Quoted 5 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+		if (!strcmp(phonebooks[i], category))

The convention has usually been to use == 0 in the case of strcmp for readability.

Not quite, as it seems: I looked at the other files and they also used the ! (including one commited by you :) ). So I chose the logical not, which also frees one from having to handle 0 vs. NULL.

Quoted 8 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+static int send_method_call(const char *dest, const char *path,
+				const char *interface, const char *method,
+				DBusPendingCallNotifyFunction cb,
+				void *user_data, int type, ...)

Since this and many other functions are shared with (or actually copied from) telephony-maemo.c would it make sense to put them to some shared c-file (it's fine if this is a separate commit later).

Agreed, but since not all telefony-* files use it, it will create dead code unless we put those functions into a lib.

Quoted 10 lines

Review

Quoted 5 lines
(quoted diff) +4 -0
+	if ((vc = find_vc_with_status(CALL_STATUS_ACTIVE))) {
+	} else if ((vc = find_vc_with_status(CALL_STATUS_DIALING))) {
+	} else if ((vc = find_vc_with_status(CALL_STATUS_INCOMING))) {
+	}

The purpose of this construction isn't imediately clear imo. Wouldn't something like doing specific NULL checks after each find() call be more readable? I.e.

Well, to me it was clear that these are nested calls until a valid vc is found. But anyway, I copied this from telephony-ofono and tried to stay close to the original code for better re-recognition. So either both should be changed or none but not be strict only on new code, since this makes copy-and-paste ineffective.

Quoted 6 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+void telephony_dial_number_req(void *telephony_device, const char *number)
+{
+	const char *clir, *callType = "voice";

No capital letters in variable names.

Done

Quoted 5 lines
Quoted 5 lines
(quoted diff) +5 -0
+#if 0
+	if (!strncmp(number, "*31#", 4)) {
+		number += 4;
+		clir = "enabled";
+	} else if (!strncmp(number, "#31#", 4)) {
Quoted 9 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+		number += 4;
+		clir =  "disabled";
+	} else
+		clir = "default";
+#endif

Is this really code that you think can be enabled later? If not I'd just remove it instead of having it commented out.

Yes, it is needed. My car kit (and maybe others) have a menu to activate this, but the current FSO API cannot handle it yet. Since this driver needs to be update anyhow once the opimd is final, I left it in so it cannot get forgotten to be reenabled.

Quoted 9 lines
Quoted 9 lines
(quoted diff) +9 -0
+	if (cmd) {
+		err = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+					FSO_GSMC_INTERFACE,
+					cmd, NULL, NULL,
+					DBUS_TYPE_INT32, &vc->call_index,
+					DBUS_TYPE_INVALID);
+	}
+	if (err < 0)
+		telephony_key_press_rsp(telephony_device, CME_ERROR_AG_FAILURE);
Quoted 8 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+        else
+		telephony_key_press_rsp(telephony_device, CME_ERROR_NONE);

Shouldn't it be an error if cmd is NULL? In general doing initializations upon declaration, especially for error variables, should be avoided. Would e.g. having a final "else err = -EINVAL" at the end of the else/else if statement make sense (which would allow removing the initialzation of err to 0?

No no, beware! It can happen that the other side of the phone call just hung up before the key press or that a nasty user presses a key when nothing is going on. In this case, the press is silenty ignored instead of confusing some headsets with an unexpected failure.

Quoted 10 lines

Review

Quoted 6 lines
(quoted diff) +6 -0
+static void retrieve_phonebook_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusError err;
+	DBusMessage *reply;
+	DBusMessageIter iter, array;
+	int ret = 0;

Instead of initializing ret upon declaration (and btw, we use "err" instead of "ret" usually) you could set it to 0 right before the done label.

Again, I checked with the other telephony files: they use ret for their codes and err for DBus error. So I stayed consistent with the names.

Quoted 5 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+	gstr = g_string_new("(");
+	for (i=0; i< n_s; i++) {

Missing spaces before and after '=', before '<'. Can you come up with a more descriptive name for the n_s variable please?

What about num_strings? This is my feeling only, since I copied this from the DBus tutorial.

Quoted 6 lines

Looks like some lines go beyone 80-colums. You can avoid this by doing

if (index == -1)

	continue;

which also (imho) makes the code more readable.

Done

Quoted 8 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+		sscanf(readindex, "%d,%d", &phonebook_info.first, &phonebook_info.last);
+		if (phonebook_info.first == -1)
+			break;

Probably you'd also want to check for sscanf return value (i.e. == 2). Maybe that's the only thing you should check for and not try to initialize these variables here since you already do that in the beginning of telephony-fso.c where you define the phonebook_info struct?

Nope, the arguments are optional and both 0,1 or two conversions can happen, so the return of sscanf serves nothing in this case.

Quoted 3 lines

Break this into two lines.

Done

Quoted 6 lines
Quoted 6 lines
(quoted diff) +6 -0
+	if (g_str_equal(property, "direction")) {
+		dbus_message_iter_get_basic(&sub, &direction);
+	} else if (g_str_equal(property, "peer")) {
+		dbus_message_iter_get_basic(&sub, &peer);
+		vc->number = g_strdup(peer);
+	} else if (g_str_equal(property, "reason")) {
Quoted 8 lines

Review

Quoted 6 lines
(quoted diff) +6 -0
+		dbus_message_iter_get_basic(&sub, &reason);
+	} else if (g_str_equal(property, "auxstatus")) {
+		dbus_message_iter_get_basic(&sub, &auxstatus);
+	} else if (g_str_equal(property, "line")) {
+		dbus_message_iter_get_basic(&sub, &line);
+	}

No braces for one-line scopes.

Well, here we have a conflict: The kernel style guide says that if one of the blocks has braces the other one should also have, even if it is a single line.

Quoted 8 lines

Review

Quoted 5 lines
(quoted diff) +5 -0
+	vc = find_vc(call_index);
+	if (!vc) {
+		vc = g_new0(struct voice_call, 1);
+	if (!vc)
+		return TRUE;

g_new0 is guaranteed to succeed or else it will call abort() so the NULL check is redundant.

What a nice feature, done.

Quoted 9 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+}
+
+

Unnecessary empty line.

Review

Quoted 1 line
(quoted diff) +1 -0
+	/* ARRAY -> ENTRY -> VARIANT*/

Space before */

Done, copy and paste telephony-ofono.c -> should be fixed there too.

Quoted 7 lines

Review

Quoted 4 lines
(quoted diff) +3 -0
+	if (reply_check_error(&err, reply)) {
+		goto done;
+	}

No braces for one-line scope (though I realize this one is probably inherited from telephony-maemo.c which I'm responsible for :)

Nope, unfortunately, you are not guilty. My code (and done).

Quoted 7 lines
Quoted 7 lines
audio/telephony.h +0 -0
@@ -127,6 +127,12 @@ typedef enum {
 	CME_ERROR_NETWORK_NOT_ALLOWED	= 32,
 } cme_error_t;
Quoted 8 lines

audio/telephony.h typedef enum { · line 134

Quoted 5 lines
audio/telephony.h +5 -0
+/* AT command types */
+#define ATNONE  0
+#define ATQUERY 1
+#define ATCHECK 2
+#define ATSET   3

These should probably be an enum and have some namespacing (e.g. AT_TYPE_).

Again, in order to be consistent with the existing codes, I looked at what the other files do. I adopted the use of the Call parameters, like CALL_STATUS_ACTIVE. These are all #defines

Quoted 3 lines

To be consistent with the AT command spec terminology (I've
been looking at 3GPP TS 07.07) it should be s/QUERY/TEST/ and s/CHECK/READ/.

Done (I don't have a copy of the spec)

Felix

Quoted 1 line

From 8c30b90811b307dc42d5003347902b121d4fa7d9 Mon Sep 17 00:00:00 2001

From: Felix Huber <[email protected]>
Date: Sun, 7 Mar 2010 18:07:47 +0100
Subject: [PATCH] Added support for HFP phonebook functions and FSO middleware

---
 audio/headset.c         |  265 ++++++----
 audio/telephony-dummy.c |   11 +
 audio/telephony-fso.c   | 1412 +++++++++++++++++++++++++++++++++++++++++++++++
 audio/telephony-maemo.c |   10 +
 audio/telephony-ofono.c |   11 +
 audio/telephony.h       |   12 +
 6 files changed, 1615 insertions(+), 106 deletions(-)
 create mode 100644 audio/telephony-fso.c
audio/headset.c +159 -106
index 15d3672..0360f26 100644
@@ -128,25 +128,6 @@ struct pending_connect {
 	uint16_t svclass;
 };
 
-struct headset_slc {
-	char buf[BUF_SIZE];
-	int data_start;
-	int data_length;
-
-	gboolean cli_active;
-	gboolean cme_enabled;
-	gboolean cwa_enabled;
-	gboolean pending_ring;
-	gboolean inband_ring;
-	gboolean nrec;
-	gboolean nrec_req;
-
-	int sp_gain;
-	int mic_gain;
-
-	unsigned int hf_features;
-};
-
 struct headset {
 	uint32_t hsp_handle;
 	uint32_t hfp_handle;

@@ -163,14 +144,28 @@ struct headset {
 
 	guint dc_timer;
 
+	char buf[BUF_SIZE];
+	int data_start;
+	int data_length;
+
 	gboolean hfp_active;
 	gboolean search_hfp;
+	gboolean cli_active;
+	gboolean cme_enabled;
+	gboolean cwa_enabled;
+	gboolean pending_ring;
+	gboolean inband_ring;
+	gboolean nrec;
+	gboolean nrec_req;
 
 	headset_state_t state;
 	struct pending_connect *pending;
 
+	int sp_gain;
+	int mic_gain;
+
+	unsigned int hf_features;
 	headset_lock_t lock;
-	struct headset_slc *slc;
 };
 
 struct event {

@@ -343,15 +338,14 @@ static int headset_send(struct headset *hs, char *format, ...)
 static int supported_features(struct audio_device *device, const char *buf)
 {
 	struct headset *hs = device->headset;
-	struct headset_slc *slc = hs->slc;
 	int err;
 
 	if (strlen(buf) < 9)
 		return -EINVAL;
 
-	slc->hf_features = strtoul(&buf[8], NULL, 10);
+	hs->hf_features = strtoul(&buf[8], NULL, 10);
 
-	print_hf_features(slc->hf_features);
+	print_hf_features(hs->hf_features);
 
 	err = headset_send(hs, "\r\n+BRSF: %u\r\n", ag.features);
 	if (err < 0)

@@ -540,12 +534,10 @@ static void send_foreach_headset(GSList *devices,
 
 static int cli_cmp(struct headset *hs)
 {
-	struct headset_slc *slc = hs->slc;
-
 	if (!hs->hfp_active)
 		return -1;
 
-	if (slc->cli_active)
+	if (hs->cli_active)
 		return 0;
 	else
 		return -1;

@@ -568,7 +560,6 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
 	int sk;
 	struct audio_device *dev = user_data;
 	struct headset *hs = dev->headset;
-	struct headset_slc *slc = hs->slc;
 	struct pending_connect *p = hs->pending;
 
 	if (err) {

@@ -608,12 +599,12 @@ static void sco_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
 
 	headset_set_state(dev, HEADSET_STATE_PLAYING);
 
-	if (slc->pending_ring) {
+	if (hs->pending_ring) {
 		ring_timer_cb(NULL);
 		ag.ring_timer = g_timeout_add_seconds(RING_INTERVAL,
 						ring_timer_cb,
 						NULL);
-		slc->pending_ring = FALSE;
+		hs->pending_ring = FALSE;
 	}
 }
 

@@ -693,10 +684,9 @@ static void hfp_slc_complete(struct audio_device *dev)
 static int telephony_generic_rsp(struct audio_device *device, cme_error_t err)
 {
 	struct headset *hs = device->headset;
-	struct headset_slc *slc = hs->slc;
 
 	if (err != CME_ERROR_NONE) {
-		if (slc->cme_enabled)
+		if (hs->cme_enabled)
 			return headset_send(hs, "\r\n+CME ERROR: %d\r\n", err);
 		else
 			return headset_send(hs, "\r\nERROR\r\n");

@@ -709,7 +699,6 @@ int telephony_event_reporting_rsp(void *telephony_device, cme_error_t err)
 {
 	struct audio_device *device = telephony_device;
 	struct headset *hs = device->headset;
-	struct headset_slc *slc = hs->slc;
 	int ret;
 
 	if (err != CME_ERROR_NONE)

@@ -722,7 +711,7 @@ int telephony_event_reporting_rsp(void *telephony_device, cme_error_t err)
 	if (hs->state != HEADSET_STATE_CONNECTING)
 		return 0;
 
-	if (slc->hf_features & HF_FEATURE_CALL_WAITING_AND_3WAY &&
+	if (hs->hf_features & HF_FEATURE_CALL_WAITING_AND_3WAY &&
 			ag.features & AG_FEATURE_THREE_WAY_CALLING)
 		return 0;
 

@@ -876,12 +865,11 @@ static int terminate_call(struct audio_device *device, const char *buf)
 static int cli_notification(struct audio_device *device, const char *buf)
 {
 	struct headset *hs = device->headset;
-	struct headset_slc *slc = hs->slc;
 
 	if (strlen(buf) < 9)
 		return -EINVAL;
 
-	slc->cli_active = buf[8] == '1' ? TRUE : FALSE;
+	hs->cli_active = buf[8] == '1' ? TRUE : FALSE;
 
 	return headset_send(hs, "\r\nOK\r\n");
 }

@@ -949,7 +937,6 @@ static int dial_number(struct audio_device *device, const char *buf)
 static int signal_gain_setting(struct audio_device *device, const char *buf)
 {
 	struct headset *hs = device->headset;
-	struct headset_slc *slc = hs->slc;
 	const char *property;
 	const char *name;
 	dbus_uint16_t gain;

@@ -968,18 +955,18 @@ static int signal_gain_setting(struct audio_device *device, const char *buf)
 
 	switch (buf[5]) {
 	case HEADSET_GAIN_SPEAKER:
-		if (slc->sp_gain == gain)
+		if (hs->sp_gain == gain)
 			goto ok;
 		name = "SpeakerGainChanged";
 		property = "SpeakerGain";
-		slc->sp_gain = gain;
+		hs->sp_gain = gain;
 		break;
 	case HEADSET_GAIN_MICROPHONE:
-		if (slc->mic_gain == gain)
+		if (hs->mic_gain == gain)
 			goto ok;
 		name = "MicrophoneGainChanged";
 		property = "MicrophoneGain";
-		slc->mic_gain = gain;
+		hs->mic_gain = gain;
 		break;
 	default:
 		error("Unknown gain setting");

@@ -1043,16 +1030,15 @@ static int list_current_calls(struct audio_device *device, const char *buf)
 static int extended_errors(struct audio_device *device, const char *buf)
 {
 	struct headset *hs = device->headset;
-	struct headset_slc *slc = hs->slc;
 
 	if (strlen(buf) < 9)
 		return -EINVAL;
 
 	if (buf[8] == '1') {
-		slc->cme_enabled = TRUE;
+		hs->cme_enabled = TRUE;
 		debug("CME errors enabled for headset %p", hs);
 	} else {
-		slc->cme_enabled = FALSE;
+		hs->cme_enabled = FALSE;
 		debug("CME errors disabled for headset %p", hs);
 	}
 

@@ -1062,16 +1048,15 @@ static int extended_errors(struct audio_device *device, const char *buf)
 static int call_waiting_notify(struct audio_device *device, const char *buf)
 {
 	struct headset *hs = device->headset;
-	struct headset_slc *slc = hs->slc;
 
 	if (strlen(buf) < 9)
 		return -EINVAL;
 
 	if (buf[8] == '1') {
-		slc->cwa_enabled = TRUE;
+		hs->cwa_enabled = TRUE;
 		debug("Call waiting notification enabled for headset %p", hs);
 	} else {
-		slc->cwa_enabled = FALSE;
+		hs->cwa_enabled = FALSE;
 		debug("Call waiting notification disabled for headset %p", hs);
 	}
 

@@ -1092,10 +1077,9 @@ int telephony_nr_and_ec_rsp(void *telephony_device, cme_error_t err)
 {
 	struct audio_device *device = telephony_device;
 	struct headset *hs = device->headset;
-	struct headset_slc *slc = hs->slc;
 
 	if (err == CME_ERROR_NONE)
-		slc->nrec = hs->slc->nrec_req;
+		hs->nrec = hs->nrec_req;
 
 	return telephony_generic_rsp(telephony_device, err);
 }

@@ -1139,17 +1123,16 @@ static int operator_selection(struct audio_device *device, const char *buf)
 static int nr_and_ec(struct audio_device *device, const char *buf)
 {
 	struct headset *hs = device->headset;
-	struct headset_slc *slc = hs->slc;
 
 	if (strlen(buf) < 9)
 		return -EINVAL;
 
 	if (buf[8] == '0')
-		slc->nrec_req = FALSE;
+		hs->nrec_req = FALSE;
 	else
-		slc->nrec_req = TRUE;
+		hs->nrec_req = TRUE;
 
-	telephony_nr_and_ec_req(device, slc->nrec_req);
+	telephony_nr_and_ec_req(device, hs->nrec_req);
 
 	return 0;
 }

@@ -1171,6 +1154,82 @@ static int voice_dial(struct audio_device *device, const char *buf)
 	return 0;
 }
 
+static int get_AT_type(const char *buf, int *offset)
+{
+	const char *AT_test = "=?", *AT_read ="?", *AT_set = "=";
+
+	if (!strncmp(buf, AT_test, 2)) {
+		*offset = 2;
+		return AT_TEST;
+	} else if (!strncmp(buf, AT_read, 1)) {
+		*offset = 1;
+		return AT_READ;
+	} else if (!strncmp(buf, AT_set, 1)) {
+		*offset = 1;
+		return AT_SET;
+	} else {
+		*offset = 0;
+		return AT_NONE;
+	}
+}
+
+int telephony_phonebook_storage_rsp(void *telephony_device, cme_error_t err)
+{
+	return telephony_generic_rsp(telephony_device, err);
+}
+
+int telephony_phonebook_storage_ind(void *telephony_device,
+						const char *storagelist)
+{
+	struct audio_device *device = telephony_device;
+	struct headset *hs = device->headset;
+
+	headset_send(hs, "\r\n+CPBS: %s\r\n", storagelist);
+
+	return 0;
+}
+
+static int phonebook_storage(struct audio_device *device, const char *buf)
+{
+	int AT_type, offset;
+
+	if (strlen(buf) < 8)
+		return -EINVAL;
+
+	AT_type = get_AT_type(&buf[7], &offset);
+	telephony_phonebook_storage_req(device, &buf[7+offset], AT_type);
+
+	return 0;
+}
+
+int telephony_phonebook_read_rsp(void *telephony_device, cme_error_t err)
+{
+	return telephony_generic_rsp(telephony_device, err);
+}
+
+int telephony_phonebook_read_ind(void *telephony_device, const char *entrylist)
+{
+	struct audio_device *device = telephony_device;
+	struct headset *hs = device->headset;
+
+	headset_send(hs, "\r\n+CPBR: %s\r\n", entrylist);
+
+	return 0;
+}
+
+static int phonebook_read(struct audio_device *device, const char *buf)
+{
+	int AT_type, offset;
+
+	if (strlen(buf) < 8) 
+		return -EINVAL;
+
+	AT_type = get_AT_type(&buf[7], &offset);
+	telephony_phonebook_read_req(device, &buf[7+offset], AT_type);
+
+	return 0;
+}
+
 static struct event event_callbacks[] = {
 	{ "ATA", answer_call },
 	{ "ATD", dial_number },

@@ -1192,6 +1251,8 @@ static struct event event_callbacks[] = {
 	{ "AT+COPS", operator_selection },
 	{ "AT+NREC", nr_and_ec },
 	{ "AT+BVRA", voice_dial },
+	{ "AT+CPBS", phonebook_storage },
+	{ "AT+CPBR", phonebook_read },
 	{ 0 }
 };
 

@@ -1231,7 +1292,6 @@ static gboolean rfcomm_io_cb(GIOChannel *chan, GIOCondition cond,
 				struct audio_device *device)
 {
 	struct headset *hs;
-	struct headset_slc *slc;
 	unsigned char buf[BUF_SIZE];
 	gsize bytes_read = 0;
 	gsize free_space;

@@ -1240,7 +1300,6 @@ static gboolean rfcomm_io_cb(GIOChannel *chan, GIOCondition cond,
 		return FALSE;
 
 	hs = device->headset;
-	slc = hs->slc;
 
 	if (cond & (G_IO_ERR | G_IO_HUP)) {
 		debug("ERR or HUP on RFCOMM socket");

@@ -1251,8 +1310,7 @@ static gboolean rfcomm_io_cb(GIOChannel *chan, GIOCondition cond,
 				&bytes_read) != G_IO_ERROR_NONE)
 		return TRUE;
 
-	free_space = sizeof(slc->buf) - slc->data_start -
-			slc->data_length - 1;
+	free_space = sizeof(hs->buf) - hs->data_start - hs->data_length - 1;
 
 	if (free_space < bytes_read) {
 		/* Very likely that the HS is sending us garbage so

@@ -1261,45 +1319,45 @@ static gboolean rfcomm_io_cb(GIOChannel *chan, GIOCondition cond,
 		goto failed;
 	}
 
-	memcpy(&slc->buf[slc->data_start], buf, bytes_read);
-	slc->data_length += bytes_read;
+	memcpy(&hs->buf[hs->data_start], buf, bytes_read);
+	hs->data_length += bytes_read;
 
 	/* Make sure the data is null terminated so we can use string
 	 * functions */
-	slc->buf[slc->data_start + slc->data_length] = '\0';
+	hs->buf[hs->data_start + hs->data_length] = '\0';
 
-	while (slc->data_length > 0) {
+	while (hs->data_length > 0) {
 		char *cr;
 		int err;
 		off_t cmd_len;
 
-		cr = strchr(&slc->buf[slc->data_start], '\r');
+		cr = strchr(&hs->buf[hs->data_start], '\r');
 		if (!cr)
 			break;
 
-		cmd_len = 1 + (off_t) cr - (off_t) &slc->buf[slc->data_start];
+		cmd_len = 1 + (off_t) cr - (off_t) &hs->buf[hs->data_start];
 		*cr = '\0';
 
 		if (cmd_len > 1)
-			err = handle_event(device, &slc->buf[slc->data_start]);
+			err = handle_event(device, &hs->buf[hs->data_start]);
 		else
 			/* Silently skip empty commands */
 			err = 0;
 
 		if (err == -EINVAL) {
 			error("Badly formated or unrecognized command: %s",
-					&slc->buf[slc->data_start]);
+					&hs->buf[hs->data_start]);
 			err = headset_send(hs, "\r\nERROR\r\n");
 		} else if (err < 0)
 			error("Error handling command %s: %s (%d)",
-						&slc->buf[slc->data_start],
+						&hs->buf[hs->data_start],
 						strerror(-err), -err);
 
-		slc->data_start += cmd_len;
-		slc->data_length -= cmd_len;
+		hs->data_start += cmd_len;
+		hs->data_length -= cmd_len;
 
-		if (!slc->data_length)
-			slc->data_start = 0;
+		if (!hs->data_length)
+			hs->data_start = 0;
 	}
 
 	return TRUE;

@@ -1357,9 +1415,6 @@ void headset_connect_cb(GIOChannel *chan, GError *err, gpointer user_data)
 
 	debug("%s: Connected to %s", dev->path, hs_address);
 
-	hs->slc = g_new0(struct headset_slc, 1);
-	hs->slc->nrec = TRUE;
-
 	/* In HFP mode wait for Service Level Connection */
 	if (hs->hfp_active)
 		return;

@@ -1834,11 +1889,10 @@ static DBusMessage *hs_get_speaker_gain(DBusConnection *conn,
 {
 	struct audio_device *device = data;
 	struct headset *hs = device->headset;
-	struct headset_slc *slc = hs->slc;
 	DBusMessage *reply;
 	dbus_uint16_t gain;
 
-	if (hs->state < HEADSET_STATE_CONNECTED)
+	if (hs->state < HEADSET_STATE_CONNECTED || hs->sp_gain < 0)
 		return g_dbus_create_error(msg, ERROR_INTERFACE ".NotAvailable",
 						"Operation not Available");
 

@@ -1846,7 +1900,7 @@ static DBusMessage *hs_get_speaker_gain(DBusConnection *conn,
 	if (!reply)
 		return NULL;
 
-	gain = (dbus_uint16_t) slc->sp_gain;
+	gain = (dbus_uint16_t) hs->sp_gain;
 
 	dbus_message_append_args(reply, DBUS_TYPE_UINT16, &gain,
 					DBUS_TYPE_INVALID);

@@ -1860,11 +1914,10 @@ static DBusMessage *hs_get_mic_gain(DBusConnection *conn,
 {
 	struct audio_device *device = data;
 	struct headset *hs = device->headset;
-	struct headset_slc *slc = hs->slc;
 	DBusMessage *reply;
 	dbus_uint16_t gain;
 
-	if (hs->state < HEADSET_STATE_CONNECTED || slc == NULL)
+	if (hs->state < HEADSET_STATE_CONNECTED || hs->mic_gain < 0)
 		return g_dbus_create_error(msg, ERROR_INTERFACE ".NotAvailable",
 						"Operation not Available");
 

@@ -1872,7 +1925,7 @@ static DBusMessage *hs_get_mic_gain(DBusConnection *conn,
 	if (!reply)
 		return NULL;
 
-	gain = (dbus_uint16_t) slc->mic_gain;
+	gain = (dbus_uint16_t) hs->mic_gain;
 
 	dbus_message_append_args(reply, DBUS_TYPE_UINT16, &gain,
 					DBUS_TYPE_INVALID);

@@ -1887,7 +1940,6 @@ static DBusMessage *hs_set_gain(DBusConnection *conn,
 {
 	struct audio_device *device = data;
 	struct headset *hs = device->headset;
-	struct headset_slc *slc = hs->slc;
 	DBusMessage *reply;
 	int err;
 

@@ -1917,14 +1969,14 @@ static DBusMessage *hs_set_gain(DBusConnection *conn,
 
 done:
 	if (type == HEADSET_GAIN_SPEAKER) {
-		slc->sp_gain = gain;
+		hs->sp_gain = gain;
 		g_dbus_emit_signal(conn, device->path,
 					AUDIO_HEADSET_INTERFACE,
 					"SpeakerGainChanged",
 					DBUS_TYPE_UINT16, &gain,
 					DBUS_TYPE_INVALID);
 	} else {
-		slc->mic_gain = gain;
+		hs->mic_gain = gain;
 		g_dbus_emit_signal(conn, device->path,
 					AUDIO_HEADSET_INTERFACE,
 					"MicrophoneGainChanged",

@@ -2001,13 +2053,11 @@ static DBusMessage *hs_get_properties(DBusConnection *conn,
 
 	/* SpeakerGain */
 	dict_append_entry(&dict, "SpeakerGain",
-				DBUS_TYPE_UINT16,
-				&device->headset->slc->sp_gain);
+				DBUS_TYPE_UINT16, &device->headset->sp_gain);
 
 	/* MicrophoneGain */
 	dict_append_entry(&dict, "MicrophoneGain",
-				DBUS_TYPE_UINT16,
-				&device->headset->slc->mic_gain);
+				DBUS_TYPE_UINT16, &device->headset->mic_gain);
 
 done:
 	dbus_message_iter_close_container(&iter, &dict);

@@ -2146,8 +2196,10 @@ static int headset_close_rfcomm(struct audio_device *dev)
 		hs->rfcomm = NULL;
 	}
 
-	g_free(hs->slc);
-	hs->slc = NULL;
+	hs->data_start = 0;
+	hs->data_length = 0;
+
+	hs->nrec = TRUE;
 
 	return 0;
 }

@@ -2202,7 +2254,12 @@ struct headset *headset_init(struct audio_device *dev, uint16_t svc,
 
 	hs = g_new0(struct headset, 1);
 	hs->rfcomm_ch = -1;
+	hs->sp_gain = -1;
+	hs->mic_gain = -1;
 	hs->search_hfp = server_is_enabled(&dev->src, HANDSFREE_SVCLASS_ID);
+	hs->hfp_active = FALSE;
+	hs->cli_active = FALSE;
+	hs->nrec = TRUE;
 
 	record = btd_device_get_record(dev->btd_dev, uuidstr);
 	if (!record)

@@ -2445,19 +2502,18 @@ int headset_connect_rfcomm(struct audio_device *dev, GIOChannel *io)
 int headset_connect_sco(struct audio_device *dev, GIOChannel *io)
 {
 	struct headset *hs = dev->headset;
-	struct headset_slc *slc = hs->slc;
 
 	if (hs->sco)
 		return -EISCONN;
 
 	hs->sco = g_io_channel_ref(io);
 
-	if (slc->pending_ring) {
+	if (hs->pending_ring) {
 		ring_timer_cb(NULL);
 		ag.ring_timer = g_timeout_add_seconds(RING_INTERVAL,
 						ring_timer_cb,
 						NULL);
-		slc->pending_ring = FALSE;
+		hs->pending_ring = FALSE;
 	}
 
 	return 0;

@@ -2476,7 +2532,6 @@ static void disconnect_cb(struct btd_device *btd_dev, gboolean removal,
 void headset_set_state(struct audio_device *dev, headset_state_t state)
 {
 	struct headset *hs = dev->headset;
-	struct headset_slc *slc = hs->slc;
 	gboolean value;
 	const char *state_str;
 	headset_state_t old_state = hs->state;

@@ -2522,9 +2577,9 @@ void headset_set_state(struct audio_device *dev, headset_state_t state)
 					DBUS_TYPE_STRING, &state_str);
 		if (hs->state < state) {
 			if (ag.features & AG_FEATURE_INBAND_RINGTONE)
-				slc->inband_ring = TRUE;
+				hs->inband_ring = TRUE;
 			else
-				slc->inband_ring = FALSE;
+				hs->inband_ring = FALSE;
 			g_dbus_emit_signal(dev->conn, dev->path,
 						AUDIO_HEADSET_INTERFACE,
 						"Connected",

@@ -2569,10 +2624,10 @@ void headset_set_state(struct audio_device *dev, headset_state_t state)
 					AUDIO_HEADSET_INTERFACE, "Playing",
 					DBUS_TYPE_BOOLEAN, &value);
 
-		if (slc->sp_gain >= 0)
-			headset_send(hs, "\r\n+VGS=%u\r\n", slc->sp_gain);
-		if (slc->mic_gain >= 0)
-			headset_send(hs, "\r\n+VGM=%u\r\n", slc->mic_gain);
+		if (hs->sp_gain >= 0)
+			headset_send(hs, "\r\n+VGS=%u\r\n", hs->sp_gain);
+		if (hs->mic_gain >= 0)
+			headset_send(hs, "\r\n+VGM=%u\r\n", hs->mic_gain);
 		break;
 	}
 

@@ -2681,7 +2736,7 @@ gboolean headset_get_nrec(struct audio_device *dev)
 {
 	struct headset *hs = dev->headset;
 
-	return hs->slc->nrec;
+	return hs->nrec;
 }
 
 gboolean headset_get_sco_hci(struct audio_device *dev)

@@ -2738,7 +2793,6 @@ int telephony_incoming_call_ind(const char *number, int type)
 {
 	struct audio_device *dev;
 	struct headset *hs;
-	struct headset_slc *slc;
 
 	if (!active_devices)
 		return -ENODEV;

@@ -2746,7 +2800,6 @@ int telephony_incoming_call_ind(const char *number, int type)
 	/* Get the latest connected device */
 	dev = active_devices->data;
 	hs = dev->headset;
-	slc = hs->slc;
 
 	if (ag.ring_timer) {
 		debug("telephony_incoming_call_ind: already calling");

@@ -2755,16 +2808,16 @@ int telephony_incoming_call_ind(const char *number, int type)
 
 	/* With HSP 1.2 the RING messages should *not* be sent if inband
 	 * ringtone is being used */
-	if (!hs->hfp_active && slc->inband_ring)
+	if (!hs->hfp_active && hs->inband_ring)
 		return 0;
 
 	g_free(ag.number);
 	ag.number = g_strdup(number);
 	ag.number_type = type;
 
-	if (slc->inband_ring && hs->hfp_active &&
+	if (hs->inband_ring && hs->hfp_active &&
 					hs->state != HEADSET_STATE_PLAYING) {
-		slc->pending_ring = TRUE;
+		hs->pending_ring = TRUE;
 		return 0;
 	}
 

@@ -2790,10 +2843,10 @@ int telephony_calling_stopped_ind(void)
 	/* In case SCO isn't fully up yet */
 	dev = active_devices->data;
 
-	if (!dev->headset->slc->pending_ring && !ag.ring_timer)
+	if (!dev->headset->pending_ring && !ag.ring_timer)
 		return -EINVAL;
 
-	dev->headset->slc->pending_ring = FALSE;
+	dev->headset->pending_ring = FALSE;
 
 	return 0;
 }

@@ -2851,7 +2904,7 @@ static int cwa_cmp(struct headset *hs)
 	if (!hs->hfp_active)
 		return -1;
 
-	if (hs->slc->cwa_enabled)
+	if (hs->cwa_enabled)
 		return 0;
 	else
 		return -1;
audio/telephony-dummy.c +11 -0
index 2409a49..71d1d2b 100644
@@ -225,6 +225,17 @@ void telephony_key_press_req(void *telephony_device, const char *keys)
 	telephony_key_press_rsp(telephony_device, CME_ERROR_NONE);
 }
 
+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)
+{
+	telephony_phonebook_read_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+void telephony_phonebook_storage_req(void *telephony_device, const char *readindex, int ATtype)
+{
+	telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+
 /* D-Bus method handlers */
 static DBusMessage *outgoing_call(DBusConnection *conn, DBusMessage *msg,
 					void *data)
audio/telephony-fso.c new file +1412 -0

1,412 lines not on this page · raw message

audio/telephony-maemo.c +10 -0
index f7531f3..0d990d1 100644
@@ -922,6 +922,16 @@ void telephony_voice_dial_req(void *telephony_device, gboolean enable)
 	telephony_voice_dial_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
 }
 
+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)
+{
+        telephony_phonebook_read_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+void telephony_phonebook_storage_req(void *telephony_device, const char *readindex, int ATtype)
+{
+        telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
 static void handle_incoming_call(DBusMessage *msg)
 {
 	const char *number, *call_path;
audio/telephony-ofono.c +11 -0
index 3df76c3..6aedf12 100644
@@ -387,6 +387,17 @@ void telephony_key_press_req(void *telephony_device, const char *keys)
 	telephony_key_press_rsp(telephony_device, CME_ERROR_NONE);
 }
 
+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)
+{
+	telephony_phonebook_read_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+void telephony_phonebook_storage_req(void *telephony_device, const char *readindex, int ATtype)
+{
+	telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
+}
+
+
 void telephony_voice_dial_req(void *telephony_device, gboolean enable)
 {
 	debug("telephony-ofono: got %s voice dial request",
audio/telephony.h +12 -0
index 0bc4769..64d58b5 100644
@@ -127,6 +127,12 @@ typedef enum {
 	CME_ERROR_NETWORK_NOT_ALLOWED	= 32,
 } cme_error_t;
 
+/* AT command types */
+#define AT_NONE  0
+#define AT_TEST 1
+#define AT_READ 2
+#define AT_SET   3
+
 struct indicator {
 	const char *desc;
 	const char *range;

@@ -157,6 +163,8 @@ void telephony_call_hold_req(void *telephony_device, const char *cmd);
 void telephony_nr_and_ec_req(void *telephony_device, gboolean enable);
 void telephony_voice_dial_req(void *telephony_device, gboolean enable);
 void telephony_key_press_req(void *telephony_device, const char *keys);
+void telephony_phonebook_storage_req(void *telephony_device, const char *storage, int AT_type);
+void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int AT_type);
 
 /* AG responses to HF requests. These are implemented by headset.c */
 int telephony_event_reporting_rsp(void *telephony_device, cme_error_t err);

@@ -173,6 +181,8 @@ int telephony_call_hold_rsp(void *telephony_device, cme_error_t err);
 int telephony_nr_and_ec_rsp(void *telephony_device, cme_error_t err);
 int telephony_voice_dial_rsp(void *telephony_device, cme_error_t err);
 int telephony_key_press_rsp(void *telephony_device, cme_error_t err);
+int telephony_phonebook_storage_rsp(void *telephony_device, cme_error_t err);
+int telephony_phonebook_read_rsp(void *telephony_device, cme_error_t err);
 
 /* Event indications by AG. These are implemented by headset.c */
 int telephony_event_ind(int index);

@@ -188,6 +198,8 @@ int telephony_subscriber_number_ind(const char *number, int type,
 					int service);
 int telephony_call_waiting_ind(const char *number, int type);
 int telephony_operator_selection_ind(int mode, const char *oper);
+int telephony_phonebook_storage_ind(void *device, const char *storagelist);
+int telephony_phonebook_read_ind(void *device, const char *entrylist);
 
 /* Helper function for quick indicator updates */
 static inline int telephony_update_indicator(struct indicator *indicators,
Signature · 1 line
1.6.4.2
#3 lore raw

Hi Felix,

Quoted 10 lines · Felix Huber
Quoted 7 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+int get_ATtype(const char *buf, int *offset)
+{
+	const char *ATquery = "=?", *ATcheck ="?", *ATset = "=";

We usually don't use capital letters in any variable or function names. Pre-processor defines are an exception. So use something like get_at_type, at_query, etc.

Well usually, but since AT in this case is a proper name, I found it very confusing to read it like the prepostion "at" or even a spelled-out @-sign. I added an underscore for better indication of what it is.

I understand your point, but this is simply the way the coding style is. Even if I would accept it (which I wont) it'd ultimately get rejected by Marcel.

Quoted 5 lines
Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+char *fso_categories[NUM_CATEGORIES] = {"contacts", "emergency", "fixed", "dialed", "received", "missed", "own"};
+char *gsm_categories[NUM_CATEGORIES] = {"\"SM\"",   "\"EN\"",    "\"FD\"","\"DC\"", "\"RC\"",   "\"MC\"", "\"ON\""};

Probably you could split these to fit within 80 columns.

Done, but now we loose the one-to-one correspondance.

Ah, I missed that relationship between the two lists. If this one-to-one correspondance is so important, why not create a simple struct with two const char* members and have just one list? Then you'd have the definition as something like:

struct category categories[] = {
	{ "contacts",	"\"SM\"" },
	{ "emergency",	"\"EN\"" },
	...
	{ NULL, NULL }
};
Quoted 8 lines
Quoted 5 lines

audio/telephony-fso.c line 268

Quoted 1 line
audio/telephony-fso.c new file +1 -0
+		if (!strcmp(phonebooks[i], category))

The convention has usually been to use == 0 in the case of strcmp for readability.

Not quite, as it seems: I looked at the other files and they also used the ! (including one commited by you :) ). So I chose the logical not, which also frees one from having to handle 0 vs. NULL.

BlueZ is unfortunately full of many such inconsistencies in the code:

jh@jh-x301~/src/bluez{master}$ git grep '!.*cmp('|wc -l
183
jh@jh-x301~/src/bluez{master}$ git grep 'cmp(.*== 0'|wc -l
111

But you're right in that the '!' form seems to be more frequent (those regexps might have some false positives though). IMHO since the test in question is a positive one ("if A is equal to B, then...") the negation sign in the statement is at least initially counterintuitive. You might also want to consider using g_str_equal which is quite popular througout the code base (I got 102 hits) or g_strcmp0 in the case that there is risk that one of the inputs could be NULL.

Quoted 9 lines
Quoted 9 lines

audio/telephony-fso.c line 363

Quoted 4 lines
audio/telephony-fso.c new file +4 -0
+	if ((vc = find_vc_with_status(CALL_STATUS_ACTIVE))) {
+	} else if ((vc = find_vc_with_status(CALL_STATUS_DIALING))) {
+	} else if ((vc = find_vc_with_status(CALL_STATUS_INCOMING))) {
+	}

The purpose of this construction isn't imediately clear imo. Wouldn't something like doing specific NULL checks after each find() call be more readable? I.e.

Quoted 5 lines

Well, to me it was clear that these are nested calls until a valid vc is found. But anyway, I copied this from telephony-ofono and tried to stay close to the original code for better re-recognition. So either both should be changed or none but not be strict only on new code, since this makes copy-and-paste ineffective.

Ok, so let's just leave it as it is for now.

Quoted 14 lines
Quoted 5 lines
Quoted 5 lines
audio/telephony-fso.c new file +5 -0
+#if 0
+	if (!strncmp(number, "*31#", 4)) {
+		number += 4;
+		clir = "enabled";
+	} else if (!strncmp(number, "#31#", 4)) {
Quoted 9 lines

audio/telephony-fso.c line 425

Quoted 5 lines
audio/telephony-fso.c new file +5 -0
+		number += 4;
+		clir =  "disabled";
+	} else
+		clir = "default";
+#endif

Is this really code that you think can be enabled later? If not I'd just remove it instead of having it commented out.

Quoted 4 lines

Yes, it is needed. My car kit (and maybe others) have a menu to activate this, but the current FSO API cannot handle it yet. Since this driver needs to be update anyhow once the opimd is final, I left it in so it cannot get forgotten to be reenabled.

Alright, so it's fine then.

Quoted 17 lines
Quoted 9 lines
Quoted 9 lines
audio/telephony-fso.c new file +9 -0
+	if (cmd) {
+		err = send_method_call(FSO_BUS_NAME, FSO_MODEM_OBJ_PATH,
+					FSO_GSMC_INTERFACE,
+					cmd, NULL, NULL,
+					DBUS_TYPE_INT32, &vc->call_index,
+					DBUS_TYPE_INVALID);
+	}
+	if (err < 0)
+		telephony_key_press_rsp(telephony_device, CME_ERROR_AG_FAILURE);
Quoted 8 lines

audio/telephony-fso.c line 542

Quoted 2 lines
audio/telephony-fso.c new file +2 -0
+        else
+		telephony_key_press_rsp(telephony_device, CME_ERROR_NONE);

Shouldn't it be an error if cmd is NULL? In general doing initializations upon declaration, especially for error variables, should be avoided. Would e.g. having a final "else err = -EINVAL" at the end of the else/else if statement make sense (which would allow removing the initialzation of err to 0?

Quoted 4 lines

No no, beware! It can happen that the other side of the phone call just hung up before the key press or that a nasty user presses a key when nothing is going on. In this case, the press is silenty ignored instead of confusing some headsets with an unexpected failure.

Right. Do fix the braces usage for the single-line though. You might want to consider getting rid of the act and rel variables completely and just assign directly the string to cmd (or if it bothers you to have the same string twice for ACTIVE and DIALING create #defines for them.

Quoted 10 lines
Quoted 10 lines

audio/telephony-fso.c

Quoted 6 lines
audio/telephony-fso.c new file +6 -0
+static void retrieve_phonebook_reply(DBusPendingCall *call, void *user_data)
+{
+	DBusError err;
+	DBusMessage *reply;
+	DBusMessageIter iter, array;
+	int ret = 0;

Instead of initializing ret upon declaration (and btw, we use "err" instead of "ret" usually) you could set it to 0 right before the done label.

Quoted 2 lines

Again, I checked with the other telephony files: they use ret for their codes and err for DBus error. So I stayed consistent with the names.

I guess this is yet another example of BlueZ internal inconsistency then. However, I know that Marcel prefers err and we should strive to use it in all new code.

Quoted 7 lines
Quoted 5 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+	gstr = g_string_new("(");
+	for (i=0; i< n_s; i++) {

Missing spaces before and after '=', before '<'. Can you come up with a more descriptive name for the n_s variable please?

What about num_strings? This is my feeling only, since I copied this from the DBus tutorial.

num_strings is certianly more descriptive than the current name and I can't come up with anything better right now, so let's just go with it.

Quoted 10 lines
Quoted 8 lines

audio/telephony-fso.c line 899

Quoted 3 lines
audio/telephony-fso.c new file +3 -0
+		sscanf(readindex, "%d,%d", &phonebook_info.first, &phonebook_info.last);
+		if (phonebook_info.first == -1)
+			break;

Probably you'd also want to check for sscanf return value (i.e. == 2). Maybe that's the only thing you should check for and not try to initialize these variables here since you already do that in the beginning of telephony-fso.c where you define the phonebook_info struct?

Nope, the arguments are optional and both 0,1 or two conversions can happen, so the return of sscanf serves nothing in this case.

Ok.

Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
audio/telephony-fso.c new file +6 -0
+	if (g_str_equal(property, "direction")) {
+		dbus_message_iter_get_basic(&sub, &direction);
+	} else if (g_str_equal(property, "peer")) {
+		dbus_message_iter_get_basic(&sub, &peer);
+		vc->number = g_strdup(peer);
+	} else if (g_str_equal(property, "reason")) {
Quoted 8 lines

audio/telephony-fso.c line 940

Quoted 6 lines
audio/telephony-fso.c new file +6 -0
+		dbus_message_iter_get_basic(&sub, &reason);
+	} else if (g_str_equal(property, "auxstatus")) {
+		dbus_message_iter_get_basic(&sub, &auxstatus);
+	} else if (g_str_equal(property, "line")) {
+		dbus_message_iter_get_basic(&sub, &line);
+	}

No braces for one-line scopes.

Quoted 3 lines

Well, here we have a conflict: The kernel style guide says that if one of the blocks has braces the other one should also have, even if it is a single line.

Yesh, I noticed the same thing when checking the kernel coding style guidelines. Most of BlueZ code is in conflict with the kernel coding style in this respect, so I think we'd need some comment from Marcel on what exactly he wants the BlueZ style to be (might be that I've already discussed this a long time ago with him but I can't remember the outcome right now).

Quoted 4 lines
Quoted 3 lines

audio/telephony-fso.c line 665

Quoted 1 line
audio/telephony-fso.c new file +1 -0
+	/* ARRAY -> ENTRY -> VARIANT*/

Space before */

Done, copy and paste telephony-ofono.c -> should be fixed there too.

Alright. I just pushed a fix for telephony-ofono.c.

Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
audio/telephony.h +0 -0
@@ -127,6 +127,12 @@ typedef enum {
 	CME_ERROR_NETWORK_NOT_ALLOWED	= 32,
 } cme_error_t;
Quoted 8 lines

audio/telephony.h typedef enum { · line 134

Quoted 5 lines
audio/telephony.h +5 -0
+/* AT command types */
+#define ATNONE  0
+#define ATQUERY 1
+#define ATCHECK 2
+#define ATSET   3

These should probably be an enum and have some namespacing (e.g. AT_TYPE_).

Quoted 3 lines

Again, in order to be consistent with the existing codes, I looked at what the other files do. I adopted the use of the Call parameters, like CALL_STATUS_ACTIVE. These are all #defines

Right. So AT_* should be enough then (as you've done).

Johan

maintainer
#4 lore raw

Hi Johan,

Quoted 18 lines
Quoted 14 lines
Quoted 6 lines
Quoted 6 lines
audio/telephony-fso.c new file +6 -0
+	if (g_str_equal(property, "direction")) {
+		dbus_message_iter_get_basic(&sub, &direction);
+	} else if (g_str_equal(property, "peer")) {
+		dbus_message_iter_get_basic(&sub, &peer);
+		vc->number = g_strdup(peer);
+	} else if (g_str_equal(property, "reason")) {
Quoted 8 lines

audio/telephony-fso.c line 940

Quoted 6 lines
audio/telephony-fso.c new file +6 -0
+		dbus_message_iter_get_basic(&sub, &reason);
+	} else if (g_str_equal(property, "auxstatus")) {
+		dbus_message_iter_get_basic(&sub, &auxstatus);
+	} else if (g_str_equal(property, "line")) {
+		dbus_message_iter_get_basic(&sub, &line);
+	}

No braces for one-line scopes.

Quoted 3 lines

Well, here we have a conflict: The kernel style guide says that if one of the blocks has braces the other one should also have, even if it is a single line.

Quoted 6 lines

Yesh, I noticed the same thing when checking the kernel coding style guidelines. Most of BlueZ code is in conflict with the kernel coding style in this respect, so I think we'd need some comment from Marcel on what exactly he wants the BlueZ style to be (might be that I've already discussed this a long time ago with him but I can't remember the outcome right now).

there is not real rule here that can be followed and will be true in all cases. As long as the code is easy to read and understand it is fine. It goes more like this: If the else statement is by itself then don't bother with braces around it. Even if the if needs braces. If you have multiple else if then the braces are actually a good idea. I would almost go that far for complex ones like the one above using braces would make it less error prone if you change something later. Even if the compiler actually does warn you these days.

Use your own personal judgment here. However if the reviewing the code make my brain hurt, then you did it wrong ;)

Regards

Marcel

#5 lore raw
Quoted 2 lines · On Mon, 08 Mar 2010 21:27:21 -0800 Marcel Holtmann <[email protected]> wrote:

Use your own personal judgment here. However if the reviewing the code make my brain hurt, then you did it wrong ;)

Now all one needs to invent is a "Marcel's-brain-hurt-O-meter" :-P

Have fun,

seife

Signature · 4 lines
Stefan Seyfried

"Any ideas, John?"
"Well, surrounding them's out."
maintainer
#6 lore raw

Hi Stefan,

Quoted 4 lines
Quoted 2 lines

Use your own personal judgment here. However if the reviewing the code make my brain hurt, then you did it wrong ;)

Now all one needs to invent is a "Marcel's-brain-hurt-O-meter" :-P

if you get one of these, send me one ;)

Regards

Marcel