[PATCH] Add call-volume interface to allow user adjust speaker and mic volume
Marcel Holtmann
marcel at holtmann.org
Fri Sep 25 06:41:35 PDT 2009
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 0000000..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 <ofono/types.h>
> +#include <ofono/dbus.h>
> +
> +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 0000000..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. 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
> + *
> + */
> +
> +#ifdef HAVE_CONFIG_H
> +#include <config.h>
> +#endif
> +
> +#define _GNU_SOURCE
> +#include <string.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <errno.h>
> +#include <unistd.h>
> +
> +#include <glib.h>
> +
> +#include <ofono/log.h>
> +#include <ofono/modem.h>
> +#include <ofono/call-volume.h>
> +
> +#include <gdbus.h>
> +#include "ofono.h"
> +#include "common.h"
> +
> +#define CALL_VOLUME_INTERFACE "org.ofono.CallVolume"
we have to stop doing this. Start using this:
#define FOO_INTERFACE OFONO_SERVICE ".FooInterface"
> +#define CALL_VOLUME_FLAG_PENDING 0x1
> +
> +static GSList *g_drivers = NULL;
> +
> +struct ofono_call_volume {
> + DBusMessage *pending;
> + const struct ofono_call_volume_driver *driver;
> + int flags;
> + void *driver_data;
> + struct ofono_atom *atom;
> +
> + unsigned int speaker_volume;
> + unsigned int microphone_volume;
> +
> + unsigned int temp_volume; /* temp volume before it is accepted by remote */
> +};
> +
> +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.
> +void ofono_call_volume_notify(struct ofono_call_volume *cv,
> + const char *property,
> + unsigned int percent)
> +{
> + DBusConnection *conn = ofono_dbus_get_connection();
> + const char *path = __ofono_atom_get_path(cv->atom);
> +
> + if(!strcmp(property, "SpeakerVolume")) {
> + cv->speaker_volume = percent;
> + }
Coding style. Actually twice. No space behind if and single if
statements need no { }.
> +
> + if (!strcmp(property, "MicrophoneVolume")) {
> + cv->microphone_volume = percent;
> + }
No { } needed.
> + ofono_dbus_signal_property_changed(conn, path, CALL_VOLUME_INTERFACE,
> + property, DBUS_TYPE_UINT32,
> + &percent);
> +}
> +
> +static DBusMessage *cv_get_properties(DBusConnection *conn,
> + DBusMessage *msg, void *data)
> +{
> + struct ofono_call_volume *cv = data;
> + DBusMessage *reply;
> + DBusMessageIter iter, dict;
> + unsigned int speaker_volume = cv->speaker_volume;
> + unsigned int microphone_volume = cv->microphone_volume;
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.
> +
> + dbus_message_iter_close_container(&iter, &dict);
> +
> + __ofono_dbus_pending_reply(&cv->pending, reply);
> +
> + return NULL;
> +}
> +
> +static void generic_callback(const struct ofono_error *error, void *data)
> +{
> + struct ofono_call_volume *cv = data;
> + DBusConnection *conn = ofono_dbus_get_connection();
> + DBusMessage *reply;
> +
> + cv->flags &= ~CALL_VOLUME_FLAG_PENDING;
> +
> + if (!cv->pending)
> + return;
> +
> + if (error->type == OFONO_ERROR_TYPE_NO_ERROR)
> + reply = dbus_message_new_method_return(cv->pending);
> + else
> + reply = __ofono_error_failed(cv->pending);
> +
> + g_dbus_send_message(conn, reply);
> +
> + dbus_message_unref(cv->pending);
> + cv->pending = NULL;
> +}
> +
> +static void sv_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 Speaker Volume set: %s",
> + telephony_error_to_str(error));
> + } else {
> + cv->speaker_volume = cv->temp_volume;
> + }
> +
> + generic_callback(error, data);
> +}
> +
> +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.
> +
> + if (!strcmp(property, "MicrophoneVolume")) {
> + if (!cv->driver->microphone_volume)
> + return (msg != NULL)?__ofono_error_not_implemented(msg):NULL;
> + cv->driver->microphone_volume(cv, percent, mv_set_callback, cv);
> + }
See comment above.
> +
> + return NULL;
> +}
> +
> +static DBusMessage *cv_set_property(DBusConnection *conn, DBusMessage *msg,
> + void *data)
> +{
> + struct ofono_call_volume *cv = data;
> + DBusMessageIter iter;
> + DBusMessageIter var;
> + const char *property;
> + unsigned int percent;
> +
> + if (cv->pending)
> + return __ofono_error_busy(msg);
> +
> + if (!dbus_message_iter_init(msg, &iter))
> + return __ofono_error_invalid_args(msg);
> +
> + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_STRING)
> + return __ofono_error_invalid_args(msg);
> +
> + dbus_message_iter_get_basic(&iter, &property);
> + dbus_message_iter_next(&iter);
> +
> + if (dbus_message_iter_get_arg_type(&iter) != DBUS_TYPE_VARIANT)
> + return __ofono_error_invalid_args(msg);
> +
> + dbus_message_iter_recurse(&iter, &var);
> +
> + if (dbus_message_iter_get_arg_type(&var) != DBUS_TYPE_UINT32)
> + return __ofono_error_invalid_args(msg);
> +
> + dbus_message_iter_get_basic(&var, &percent);
> +
> + return cv_set_call_volume(msg, cv, property, percent);
> +}
> +
> +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.
> +
> +static GDBusSignalTable cv_signals[] = {
> + { "PropertyChanged", "sv" },
> + { }
> +};
> +
> +static void call_volume_remove(struct ofono_atom *atom)
> +{
> + struct ofono_call_volume *cv = __ofono_atom_get_data(atom);
> +
> + DBG("atom: %p", atom);
> +
> + if (cv== NULL)
> + return;
> +
> + if (cv->driver && cv->driver->remove)
> + cv->driver->remove(cv);
> +
> + g_free(cv);
> +}
> +
> +struct ofono_call_volume *ofono_call_volume_create(struct ofono_modem *modem,
> + unsigned int vendor,
> + const char *driver,
> + void *data)
> +{
> + struct ofono_call_volume *cv;
> + GSList *l;
> +
> + 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.
> +
> + cv->atom = __ofono_modem_add_atom(modem,
> + OFONO_ATOM_TYPES_CALL_VOLUME,
> + call_volume_remove, cv);
> +
> + for (l = g_drivers; l; l = l->next) {
> + const struct ofono_call_volume_driver *drv = l->data;
> +
> + if (g_strcmp0(drv->name, driver))
> + continue;
> +
> + if (drv->probe(cv, vendor, data) < 0)
> + continue;
> +
> + cv->driver = drv;
> + break;
> + }
> +
> + if (cv->driver) {
> + cv_set_call_volume(NULL, cv, "SpeakerVolume", cv->speaker_volume);
> + cv_set_call_volume(NULL, cv, "MicrophoneVolume", cv->microphone_volume);
> + }
> +
> + return cv;
> +}
> +
> +void ofono_call_volume_register(struct ofono_call_volume *cv)
> +{
> + DBusConnection *conn = ofono_dbus_get_connection();
> + struct ofono_modem *modem = __ofono_atom_get_modem(cv->atom);
> + const char *path = __ofono_atom_get_path(cv->atom);
> +
> + if (!g_dbus_register_interface(conn, path,
> + CALL_VOLUME_INTERFACE,
> + cv_methods, cv_signals, NULL,
> + cv, NULL)) {
> + ofono_error("Could not create %s interface",
> + CALL_VOLUME_INTERFACE);
> +
> + return;
> + }
> +
> + ofono_modem_add_interface(modem, CALL_VOLUME_INTERFACE);
> +
> + __ofono_atom_register(cv->atom, call_volume_unregister);
> +}
> +
> +void call_volume_unregister(struct ofono_atom *atom)
> +{
> + DBusConnection *conn = ofono_dbus_get_connection();
> + struct ofono_call_volume *cv = __ofono_atom_get_data(atom);
> + struct ofono_modem *modem = __ofono_atom_get_modem(atom);
> + const char *path = __ofono_atom_get_path(atom);
> + GSList *l;
> +
> + ofono_modem_remove_interface(modem, CALL_VOLUME_INTERFACE);
> + g_dbus_unregister_interface(conn, path,
> + CALL_VOLUME_INTERFACE);
> +}
> +
> +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 these casts only because of const driver declaration.
Personally I am against it, but I see some value behind having this
const. However there has to be a space between the cast and the
variable.
> +
> + return 0;
> +}
> +
> +void ofono_call_volume_driver_unregister(const struct ofono_call_volume_driver *d)
> +{
> + DBG("driver: %p, name: %s", d, d->name);
> +
> + g_drivers = g_slist_remove(g_drivers, (void *)d);
> +}
See comment above.
> +
> +void ofono_call_volume_remove(struct ofono_call_volume *cv)
> +{
> + __ofono_atom_free(cv->atom);
> +}
> +
> +void ofono_call_volume_set_data(struct ofono_call_volume *cv, void *data)
> +{
> + cv->driver_data = data;
> +}
> +
> +void *ofono_call_volume_get_data(struct ofono_call_volume *cv)
> +{
> + return cv->driver_data;
> +}
> +
> diff --git a/src/ofono.h b/src/ofono.h
> index 0659989..409a9e2 100644
> --- a/src/ofono.h
> +++ b/src/ofono.h
> @@ -105,6 +105,7 @@ enum ofono_atom_type {
> OFONO_ATOM_TYPE_SSN = 12,
> OFONO_ATOM_TYPE_MESSAGE_WAITING = 13,
> OFONO_ATOM_TYPE_CBS = 14,
> + OFONO_ATOM_TYPES_CALL_VOLUME = 15,
> };
>
> enum ofono_atom_watch_condition {
Regards
Marcel
More information about the ofono
mailing list