Re: [PATCH] Add call-volume interface to allow user adjust speaker and mic volume
Hi Zhenhua, Your patch has been applied. I refactored much of it afterwards :) Let me know if I have broken something. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH] Add call-volume interface to allow user adjust speaker and mic volume
Hi Marcel,
Marcel Holtmann wrote:
> Hi Zhenhua,
>
>> +#define CALL_VOLUME_INTERFACE "org.ofono.CallVolume"
>
> we have to stop doing this. Start using this:
>
> #define FOO_INTERFACE OFONO_SERVICE ".FooInterface"
>
>> +static void call_volume_unregister(struct ofono_atom *atom);
>
> That forward declaration seems pointless unless I actually missed
> something. No forward declaration unless they are really needed. Just
> shuffle the code around.
>
> Coding style. Actually twice. No space behind if and single if
> statements need no { }.
>
>> +
>> + if (!strcmp(property, "MicrophoneVolume")) {
>> + cv->microphone_volume = percent;
>> + }
>
> No { } needed.
>
> What is this variable non-sense for. You can use cv->speaker_volume
> directly from D-Bus message append functions. Only for certain strings
> you need this.
>
>> +
>> + if (cv->pending)
>> + return __ofono_error_busy(msg);
>> +
>> + cv->pending = dbus_message_ref(msg);
>> +
>> + reply = dbus_message_new_method_return(cv->pending); +
>> + if (!reply)
>> + return NULL;
>
> What is this empty line for? Just don't do that. The error check
> logically belongs to the function creating reply.
>
> Also who unrefs cv->pending in this case?
>
>> + dbus_message_iter_init_append(reply, &iter); +
>> + dbus_message_iter_open_container(&iter, DBUS_TYPE_ARRAY,
>> +
>> OFONO_PROPERTIES_ARRAY_SIGNATURE, +
>> &dict); +
>> + ofono_dbus_dict_append(&dict, "SpeakerVolume",
>> DBUS_TYPE_UINT32, +
>> &speaker_volume); + + ofono_dbus_dict_append(&dict,
>> "MicrophoneVolume", +
>> DBUS_TYPE_UINT32, µphone_volume);
>
> Why do we use UINT32 for percentage values? A BYTE would be clearly
> enough.
>
>> +static void mv_set_callback(const struct ofono_error *error, void
>> *data) +{ + struct ofono_call_volume *cv = data;
>> +
>> + if (error->type != OFONO_ERROR_TYPE_NO_ERROR) {
>> + ofono_debug("Error occurred during Microphone Volume
>> set: %s", +
>> telephony_error_to_str(error)); + } else { +
>> cv->microphone_volume = cv->temp_volume; + } +
>> + generic_callback(error, data);
>> +}
>
> So personally I think the ofono_debug() call makes these two function
> hard to read and more complicated than needed. If we really want to
> display this error then either use ofono_error() or just don't bother
> at all with any kind of output.
>
>> +static DBusMessage *cv_set_call_volume(DBusMessage *msg, struct
>> ofono_call_volume *cv, +
>> const char *property, unsigned int percent) +{ + if (percent >
>> 100) + return __ofono_error_invalid_format(msg); +
>> + DBG("set %s volume to %d percent", property, percent); +
>> + cv->flags |= CALL_VOLUME_FLAG_PENDING;
>> + cv->temp_volume = percent;
>> +
>> + if (msg)
>> + cv->pending = dbus_message_ref(msg); +
>> + if(!strcmp(property, "SpeakerVolume")) {
>> + if (!cv->driver->speaker_volume) {
>> + return (msg !=
>> NULL)?__ofono_error_not_implemented(msg):NULL; + } +
>> cv->driver->speaker_volume(cv, percent, sv_set_callback, cv); +
>> }
>
> So the space after if is missing again. And this return statement is
> not readable at all. It violates coding style and makes my brain
> hurt. Redo this it becomes readable. Cramping everything in one line
> is just no helpful.
>
>> +static GDBusMethodTable cv_methods[] = {
>> + { "GetProperties", "", "a{sv}",
>> cv_get_properties, +
>> G_DBUS_METHOD_FLAG_ASYNC}, + { "SetProperty","sv",
>> "", cv_set_property, +
>> G_DBUS_METHOD_FLAG_ASYNC }, + { } +};
>
> I am missing the point why GetProperties has to be done async. That
> looks like too much overhead for something just returning stored
> values.
>
>> +
>> + if (driver == NULL)
>> + return NULL;
>> +
>> + cv = g_try_new0(struct ofono_call_volume, 1);
>> + cv->speaker_volume = 100;
>> + cv->microphone_volume = 100;
>> +
>> + if (cv == NULL)
>> + return NULL;
>
> How do you expect this to work actually? If cv == NULL, then your
> assignment are NULL pointer dereferences and the daemon will crash.
> Your error handling is not protecting it.
>
>> +int ofono_call_volume_driver_register(const struct
>> ofono_call_volume_driver *d) +{ + DBG("driver: %p, name: %s",
>> d, d->name); +
>> + if (d->probe == NULL)
>> + return -EINVAL;
>> +
>> + g_drivers = g_slist_prepend(g_drivers, (void *)d);
>
> I really hate t
Re: [PATCH] Add call-volume interface to allow user adjust speaker and mic volume
Hi Zhenhua,
> Makefile.am |4 +-
> include/call-volume.h | 67 +
> src/call-volume.c | 374
> +
> src/ofono.h |1 +
> 4 files changed, 444 insertions(+), 2 deletions(-)
> create mode 100644 include/call-volume.h
> create mode 100644 src/call-volume.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 02f8835..522c1b5 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -10,7 +10,7 @@ include_HEADERS = include/log.h include/plugin.h
> include/history.h \
> include/phonebook.h include/ssn.h include/ussd.h \
> include/sms.h include/sim.h include/message-waiting.h
> \
> include/netreg.h include/voicecall.h
> include/devinfo.h \
> - include/cbs.h
> + include/cbs.h include/call-volume.h
>
> nodist_include_HEADERS = include/version.h
>
> @@ -163,7 +163,7 @@ src_ofonod_SOURCES = $(gdbus_sources) $(builtin_sources) \
> src/ssn.c src/call-barring.c src/sim.c \
> src/phonebook.c src/history.c src/message-waiting.c \
> src/simutil.h src/simutil.c src/storage.h \
> - src/storage.c src/cbs.c src/watch.c
> + src/storage.c src/cbs.c src/watch.c src/call-volume.c
>
> src_ofonod_LDADD = $(builtin_libadd) \
> @GLIB_LIBS@ @GTHREAD_LIBS@ @DBUS_LIBS@ -ldl
> diff --git a/include/call-volume.h b/include/call-volume.h
> new file mode 100644
> index 000..f36d2f4
> --- /dev/null
> +++ b/include/call-volume.h
> @@ -0,0 +1,67 @@
> +/*
> + *
> + * oFono - Open Source Telephony
> + *
> + * Copyright (C) 2008-2009 Intel Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301
> USA
> + *
> + */
> +
> +#ifndef __OFONO_CALL_VOLUME_H
> +#define __OFONO_CALL_VOLUME_H
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +#include
> +#include
> +
> +struct ofono_call_volume;
> +
> +typedef void (*ofono_call_volume_cb_t)(const struct ofono_error *error,
> + void *data);
> +
> +struct ofono_call_volume_driver {
> + const char *name;
> + int (*probe)(struct ofono_call_volume *cv, unsigned int vendor,
> + void *data);
> + void (*remove)(struct ofono_call_volume *cv);
> + void (*speaker_volume)(struct ofono_call_volume *cv, unsigned int
> percent,
> + ofono_call_volume_cb_t cb, void *data);
> + void (*microphone_volume)(struct ofono_call_volume *cv, unsigned int
> percent,
> + ofono_call_volume_cb_t cb, void *data);
> +};
> +
> +void ofono_call_volume_notify(struct ofono_call_volume *cv,
> + const char *property, unsigned int
> percent);
> +
> +int ofono_call_volume_driver_register(const struct ofono_call_volume_driver
> *d);
> +void ofono_call_volume_driver_unregister(const struct
> ofono_call_volume_driver *d);
> +
> +struct ofono_call_volume *ofono_call_volume_create(struct ofono_modem *modem,
> + unsigned int vendor, const char
> *driver, void *data);
> +
> +void ofono_call_volume_register(struct ofono_call_volume *cv);
> +void ofono_call_volume_remove(struct ofono_call_volume *cv);
> +
> +void ofono_call_volume_set_data(struct ofono_call_volume *cv, void *data);
> +void *ofono_call_volume_get_data(struct ofono_call_volume *cv);
> +
> +#ifdef __cplusplus
> +}
> +#endif
> +
> +#endif /* __OFONO_CALL_VOLUME_H */
> diff --git a/src/call-volume.c b/src/call-volume.c
> new file mode 100644
> index 000..aa740c0
> --- /dev/null
> +++ b/src/call-volume.c
> @@ -0,0 +1,374 @@
> +/*
> + *
> + * oFono - Open Source Telephony
> + *
> + * Copyright (C) 2008-2009 Intel Corporation. All rights reserved.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. S
[PATCH] Add call-volume interface to allow user adjust speaker and mic volume
Hi, Attached is the patch to add call-volume interface for Ofono. It exposes two properties: 'SpeakerVolume' and 'MicrophoneVolume' to allow user could control phone volume through lower level driver. The ofono_call_volume_notify() is used to report call volume property change notification to the upper application. The default volume could be stored in the modem.conf or query from remote. Each driver could report value in percentage to call-volume through ofono_call_volume_notify(). And then the call-volume will sync volume with remote side right after probe(). Please review it. Regards, Zhenhua 0001-Add-call-volume-interface-to-allow-user-to-adjust-sp.patch Description: 0001-Add-call-volume-interface-to-allow-user-to-adjust-sp.patch ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
