[PATCH] Add call-volume interface to allow user adjust speaker and mic volume

[Zhang, Zhenhua]

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, &microphone_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 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.
> 

Thank you for your valuable comments. The new patch fixed the code style
problem and I want to highlight some points here:

1. SpeakerVolume and MicrophoneVolume changes from DBUS_TYPE_UINT32 to
DBUS_TYPE_BYTE. The internal presentation is 'unsigned char' now.

2. In ofono_call_volume_create(), the initial speaker_volume and 
microphone_volume set to 50% instead of 100%. Each driver is responsible to 
probe modem and get the volume from mobile. Then it should use 
ofono_call_volume_notify() to overwrite default values.

3. However, in HFP spec v1.5, it's alway use HF side volume value to sync with
AG volume by using +VGS=, +VGM=. I don't see the AT command to query mobile
volume from the HF side. I expose ofono_set_call_volume() to lower driver to allow 
HFP driver sync volume with AG in the probe stage.

Please review it.

> 
> Regards
> 
> Marcel
> 
> 
> _______________________________________________
> ofono mailing list
> ofono at ofono.org
> http://lists.ofono.org/listinfo/ofono

Regards,
Zhenhua
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Add-call-volume-interface-to-adjust-speaker-and-mic.patch
Type: application/octet-stream
Size: 13661 bytes
Desc: 0002-Add-call-volume-interface-to-adjust-speaker-and-mic.patch
URL: <http://lists.ofono.org/pipermail/ofono/attachments/20090927/cd904f56/attachment.obj>