[Librem-5-dev] [review v2] Voice implementation fixes (with audio channel setup handlers)

[Aleksander Morgado]

Hey,

On Wed, Sep 19, 2018 at 6:29 PM Bob Ham <bob.ham at puri.sm> wrote:
>
> On 16/08/18 17:30, Bob Ham wrote:
>
> > Possibly, yes.  The SimTech (== SIMCom) plugin is using the
> > MMBroadbandModemQmi class which means I either need to (1) subclass
> > MMBroadbandModemQmi and mix in AT commands, or (2) use the plugin's
> > AT-only MBroadbandModemSimtech class and add to it.  I'd rather stick
> > with QMI but I don't know how well the modem will cope with it so I need
> > to experiment.
>
> I did stick with QMI, or at least the Qmi class.  FYI, here's my patches:
>
>   https://source.puri.sm/Librem5/ModemManager/commits/rah/sim7100-audio
>
> Feedback is welcome.
>

Some comments:

 * You're subclassing the QMI modem, but then implementing AT-based
logic for the Voice operations in the modem. In this case, it's better
to have some "shared" code between the QMI and non-QMI modem objects,
for example by the means of a "MMSharedSimtech" interface (see e.g.
the XMM plugin where we are doing exactly this to share logic between
MBIM and non-MBIM modem objects).

 * The MMCallQmiSimtech object is subclassing the MMBaseCall object,
better call it MMCallSimtech instead, as there is no QMI in there.

 * G_DECLARE_FINAL_TYPE (GLib >= 2.44).... well, it's nice to see
that, but we're currently allowing builds with GLib 2.36, so that new
code would fail to build. I would possibly agree on bumping the
minimum required GLib version to 2.40, which is the latest update
available for Ubuntu 14.04 LTS (I maintain a PPA for 14.04 with the
latest MM), at least until April 2019 which is when 14.04 is no longer
maintained.

 * From your "FIXME: How do we ensure these are true?" question
regarding MM_BASE_CALL_SUPPORTS_DIALING_TO_RINGING and
MM_BASE_CALL_SUPPORTS_RINGING_TO_ACTIVE, see:
https://gitlab.freedesktop.org/mobile-broadband/ModemManager/commit/6d3777fd6942fd91d132f64bfc2c9d9ee87227d1.
You should set those to TRUE only after you have implemented custom
support in your plugin to detect the transitions "from dialing to
ringing" and "from ringing to active". The generic code in MMBaseCall
does NOT support those transitions, as there is no generic URC
reporting those. You should check in the SimTech AT command reference
which AT command they use for these 2 transitions, and provide an
implementation, in the same way as done with UCALLSTAT for u-blox
modems in that patch above.

Cheers!

-- 
Aleksander
https://aleksander.es