Hi Felix,
On Sun, Feb 28, 2010, Felix Huber wrote:
> 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:
> +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.
> + if (!strncmp(buf, ATquery, 2)) {
> + *offset = 2;
> + return (ATQUERY);
No () around the value given to return;
> + } 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.
> +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.
> + headset_send(hs, "\r\n+CPBS: %s \r\n", storagelist);
Why is there a space before the "\r\n"?
> + int ATtype, offset;
Again the capital letter thing.
> + if (strlen(buf) < 8) {
> + return -EINVAL;
> + }
No braces for single-line scopes.
> + ATtype = get_ATtype(&buf[7], &offset);
No capital letters.
> + if (strlen(buf) < 8) {
> + return -EINVAL;
> + }
No braces.
> + ATtype = get_ATtype(&buf[7], &offset);
> + telephony_phonebook_read_req(device, &buf[7+offset], ATtype);
> +
> + return 0;
> +}
> +
> +
Unnecessary extra empty line.
> +void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)
Looks like a > 80 column line.
> +void telephony_phonebook_storage_req(void *telephony_device, const char *readindex, int ATtype)
Same issue.
> +{
> + telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_NOT_SUPPORTED);
> +}
> +
> +
Extra empty line again.
> +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.
> +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.
> +} 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.
> +static void vc_free(struct voice_call *vc)
> +{
> + if (!vc)
> + return;
> + g_free(vc->number);
> + g_free(vc);
> +}
> +
> +
Extra empty line.
> +static int find_category(char **phonebooks, const char *category)
> +{
> + int i;
> + for (i=0; i < NUM_CATEGORIES; i++) {
Space before and after '='.
> + if (!strcmp(phonebooks[i], category))
The convention has usually been to use == 0 in the case of strcmp for
readability.
> + 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.
> +
> +
Extra empty line.
> +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).
> + 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.
> + 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.
> +void telephony_dial_number_req(void *telephony_device, const char *number)
> +{
> + const char *clir, *callType = "voice";
No capital letters in variable names.
> +#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.
> + 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.
> + 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.
> + 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.
> +void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)
Looks like a possibly grater than 80 columns line.
> +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.
> + 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.
> + if (number_type == &subscriber_number) {
> + g_free(subscriber_number);
> + subscriber_number = g_strdup(number);
> + }
> +
> +done:
> + dbus_message_unref(reply);
> +}
> +
> +
Extra empty line.
> +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:
> + 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.
> + 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.
> +}
> +
> +
> +
Two extra empty lines.
> +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.
> + int min_index=-1, max_index =-1, number_length = -1, name_length = -1;
Missing spaces before and after '='.
> + 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.
> + 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);
> + telephony_phonebook_storage_rsp(phonebook_info.telephony_device, CME_ERROR_NONE);
> +
> +done:
> + dbus_message_unref(reply);
> +
> +}
> +
> +
> +
Two unnecessary extra empty lines.
> + int ret = 0;
Same thing as already mentioned.
> + 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?
> + 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.
> +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.
> +}
> +
> +
> +
Two extra empty lines.
> +void telephony_phonebook_storage_req(void *telephony_device, const char *storage, int ATtype)
Greater than 80-column line. No capital letters in variable names.
> + 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 '=').
> + 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).
> + 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 (
> + 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.
> + }
> + break;
> + default:
> + ret = -1;
> + }
> +
Redundant tab-character on this empty line.
> + if (ret < 0)
> + telephony_phonebook_storage_rsp(telephony_device, CME_ERROR_AG_FAILURE);
Greater than 80-column line.
> +}
> +
> +
Unnecessary empty line.
> +void telephony_phonebook_read_req(void *telephony_device, const char *readindex, int ATtype)
Greater than 80 column line.
> +{
> + 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 '='
> + switch (ATtype) {
> + case ATQUERY: // =?
No C++ comments.
> + 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)
> + phonebook_info.first=-1, phonebook_info.last=-1;
Missing spaces before and after '='.
> + 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?
> +
> + if (phonebook_info.last == -1) phonebook_info.last = phonebook_info.first;
Break this into two lines.
> +
> + 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:
> + 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.
> +static void parse_gsmcall_property(const char *property, DBusMessageIter sub, struct voice_call *vc)
Too long line.
> + 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.
> +}
> +
> +
Unnecessary empty line.
> + 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.
> + dbus_message_iter_get_basic(&iter, &status);
> +
> + debug("**** gsmProp Changed id:%d status: %s", call_index, status);
> +
> +
Unnecessary empty line.
> + 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.
> + 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.
> + 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.
> + return TRUE;
> +}
> +
> +
> +
> +
Three unnecessary empty lines.
> +}
> +
> +
Unnecessary empty line.
> + /* ARRAY -> ENTRY -> VARIANT*/
Space before */
> + dbus_message_iter_recurse(&iter_property, &sub);
> +
> + parse_registration_property(property, sub);
> +
> + dbus_message_iter_next(&iter_entry);
Incorrect indentation for the last line.
> + }
> +
> +done:
Redundant tab character on the empty line.
> + return TRUE;
> +}
> +
> +
Unnecessary empty line.
> +done:
> + dbus_message_unref(reply);
> +}
> +
> +
Unnecessary empty line.
> +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.
> + 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 :)
> --- a/audio/telephony.h
> +++ b/audio/telephony.h
> @@ -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
--
To unsubscribe from this list: send the line "unsubscribe linux-bluetooth" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
