Branch for review: 'riccardo/voice'

[Riccardo Vangelisti]

Il 27/05/2015 17:58, Dan Williams ha scritto:
> On Sat, 2015-05-23 at 18:31 +0200, Aleksander Morgado wrote:
>> Hey Dan & others,
>>
>> Riccardo & Marco developed an initial draft of the AT-based voice
>> support, including API, libmm-glib, mmcli, generic modem and
>> Huawei-specific implementations; his work is in the "riccardo/voice"
>> branch in upstream git:
>>
>> http://cgit.freedesktop.org/ModemManager/ModemManager/log/?h=riccardo/voice
>>
>> I've pushed some additional fixes on top of their work, mostly
>> leftovers or minor coding style fixes.
>>
>> Riccardo & Marco; I also rewrote a bit the commit messages, not the
>> contents. Also, please review my changes and tell me what you think.
>> And sorry for the late reply...
>>
>> Anyway, the branch looks good enough to me; there are some things that
>> it doesn't yet cover, but I believe it's a good start. Dan, what do
>> you think?
>> libmm-glib: added support for Modem.Voice and Call interfaces
> Should mm_call_properties_set_number() check that the number is not
> zero-length?  I'm not sure how much validation we want to do in
> mm-call-properties.c but maybe a bit more?
Ok, we've pushed this. All other mm_call_properties_set_* functions are 
already checked by enum type.
>
> Also, mm_modem_voice_list_call() -> mm_modem_voice_list_calls() (since
> it should be plural).  The D-Bus API is also "ListCalls" so we should
> keep it consistent.  Same for the _sync() variant, and for
> ListCallContext.
Ok, fixed typos
>
> Also a couple occurrences of some odd parentheses:
>
> dictionary = (mm_call_properties_get_dictionary (properties));
Ok changed
>
> Also some dead code in libmm-glib/mm-modem-voice.c that's been commented
> out with //.  Actually, finalize() can just get removed from this file.
Already modified by Aleksander
>
>> mmcli: added Modem.Voice and Call support
> Could use more g_clear_object() in various places, but I'll bet this is
> a C&P from before glib-2.28 or something.  Anyway, something to keep in
> mind for new code.
Ok, hereafter we will use it
>
> Also, it should be "--voice-list-calls" too like above in libmm-glib.
>
>> broadband-modem: added voice call support
> I'm not sure just checking for ATH is a great indicator of voice
> support; most devices I know of will support ATH, but almost none
> actually support voice.  Possibly we need plugin-specific checks
> instead?  For example I don't think most of the Huawei devices support
> voice unless they return ^CVOICE: 1, but they will return OK from ATH.
Ok nice question, we have discussed a bit me and Marco but there isn't 
an AT command that confirm the voice call support.
Also there isn't a proprietary Huawei ones.
Actually, if someone use a voice call method will receive an error on 
unsupported modules (for example on start method).

Any ideas ?

>
> Should also remove the C&P dtmf_received() implementation that's
> commented out.
You refer to a wip commit, was removed later
>
>>   build: added voice call include in libmm-glib and modified
> Makefile.am
>
> This should probably get squashed with previous commits.
>
>> api,voice: added DTMF interface specification
> How about "SendDtmf" and "DtmfReceived" instead?  Seems clearer to me.
Aleksander what do you think ? If you agree, I will change it.
>
> Dan
>
All changes are pushed on bitbucket.

Thanks,
Riccardo