[review v2] Voice implementation fixes (with audio channel setup handlers)

Aleksander Morgado aleksander at aleksander.es
Sat Sep 22 09:40:00 UTC 2018


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

 * From your "FIXME: How do we ensure these are true?" question
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.



More information about the ModemManager-devel mailing list