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
Hi Stefan,
> > 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
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
--
Stefan Seyfried
"Any ideas, John?"
"Well, surrounding them's out."
Hi Johan,
> > > > + 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.
> > 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).
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
Hi Felix,
On Sun, Mar 07, 2010, Felix Huber wrote:
> > > +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.
> > > +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 }
};
> > > + 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.
> > > + 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.
Ok, so let's just leave it as it is for now.
> > > +#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.
> >
> 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.
> > > + 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?
> 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.
> > > +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.
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.
> > > + 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.
> > > + 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.
> > > + 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.
> 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).
> > > + /* 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.
> > > --- 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_).
> 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
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:
> 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.
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.
> Also, since get_ATtype is only used within
> headset.c you have to declare it static.
done
> > +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.
> > + 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.
> > +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.
>
> > + 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.
> > +void telephony_dial_number_req(void *telephony_device, const char *number)
> > +{
> > + const char *clir, *callType = "voice";
>
> No capital letters in variable names.
>
Done
> > +#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.
>
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.
> > + 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?
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.
> > +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.
> > + 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.
> 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
> > + 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.
>
> Break this into two lines.
Done
> > + 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.
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.
> > + 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.
> > +}
> > +
> > +
>
> Unnecessary empty line.
>
> > + /* ARRAY -> ENTRY -> VARIANT*/
>
> Space before */
Done, copy and paste telephony-ofono.c -> should be fixed there too.
>
> > + 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).
>
> > --- 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_).
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
> 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
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