Re: ModemManager: SMS List API doesn't have an index, buffer isn't big enough

[Dan Williams <dcbw redhat com>]

On Wed, 2011-04-06 at 18:21 -0500, Dan Williams wrote:
> On Wed, 2011-04-06 at 12:16 -0400, Nathan Williams wrote:
> > Hi, all. In the process of implementing some SMS support, I've run
> > into a couple of issues with the
> > org.freedesktop.ModemManager.Modem.Gsm.SMS.List DBus call, one in the
> > API and one in the implementation.
> 
> Excellent :)  We can change the API because for all practical purposes,
> it's not being used yet.

Also on the CDMA side, the way to get at messaging isn't really
standardized.  Huawei CDMA modems (EC168C for example) may try to map it
to GSM-type commands, but in general it's available on CDMA phones or
devices unless you start doing QCDM/Diag and poke around the EFS
folders.  Of course devices like Gobi may provide access to SMS-related
stuff from the helper library.  But if we need to change the API at all
it might be helpful to make it more generic so that CDMA could use it
too.

At least CDMA is fairly simple, without the complexity of multi-part
messages and much of the PDU mode/SMSC stuff.

Dan

> > The API issue is that as specified, the caller gets the contents of
> > all of the messages, but not their index numbers, so there's no way to
> > delete the messages that have been received. Here are three ways I've
> > considered fixing this:
> > 
> > 
> >  1. Change to a List command that just gets the index numbers of the
> > messages and lets the caller Get and Delete from that as needed. This
> > is an OK API but doesn't really match the AT commands and would be a
> > bit inefficient in that regard.
> >  2. Change the return type from "aa{sv}" to something that includes
> > the index distinct from the message, such as "a(ia{sv})". Reflects the
> > low level well, creates a bit of extra assembly and disassembly work.
> >  3. Add an "index" integer element to the dictionary of each message.
> > This is both easy and still pretty closely reflects the low-level
> > operation.
> > 
> > 
> > I'm leaning towards #3.
> 
> Yeah, I agree.  #2 maps more closely to the other methods since they all
> take top-level "u" arguments, but having it in the dict isn't a problem.
> But make sure you make the 'index' item type uint32 ("u") to match the
> rest instead of 'i'.
> 
> > 
> > The implementation issue is the 2kb buffer-size limit imposed in
> > mm-serial-port.c. With a bunch of test messages on my device, I'm
> > already up past 3kb of data returned from a single AT+CMGL command. A
> > 30-message memory could be up to 10kb with maximum-size messages, and
> > I have no reason to think that there aren't devices with larger
> > memories. At the very least, I'll cook up a patch to raise the buffer
> > size limit, perhaps to 64k; ideally, the stack-allocated read buf size
> > wouldn't have to increase as well, since most commands don't need this
> > much space. Alternately, it would be nice to have some other means of
> > detecting out-of-control modem spew besides an arbitrary size limit.
> > Suggestions?
> 
> Just took a look at the code and that's my read of it too.  The
> out-of-control thing is only required for probing, so perhaps we could
> add another GObject property for "spew-control" (boolean) and when
> probing we'd set that property to TRUE, but normally it would be FALSE.
> THen we'd check that in data_available() like so:
> 
>     /* Make sure the response doesn't grow too long */
>     if (priv->response->len > SERIAL_BUF_SIZE && priv->spew_control) {
> 
> We're certainly not going to be grabbing messages when probing the modem
> so the two operations here are mutually exclusive.  Note that
> priv->response will grow automatically in size so I don't think we need
> to adjust the initial 500 byte size at all.
> 
> If you're not entirely familiar with GObject properties yet, here's what
> you do...
> 
> 1) add the property string name in mm-serial-port.h near the top; I like
> to use the #defined names instead of strings to ensure that wrong
> property names are compile errors instead of runtime ones
> 
> 2) add the internal property number at the top of mm-serial-port.c in
> the property enum
> 
> 3) in mm_serial_port_class_init() register the property, you'll be using
> g_param_spec_boolean, and make the default be FALSE.  You'll want
> G_PARAM_CONSTRUCT_ONLY and G_PARAM_READWRITE for flags since we only
> ever want to set the property when we create the port.
> 
> 4) add the 'gboolean spew_control' member to the NMSerialPortPrivate
> struct
> 
> 5) then you add the property to get_property() and set_property() using
> g_value_set_boolean() and g_value_get_boolean() respectively, and hook
> up the code to set or return priv->spew_control accordingly
> 
> 6) then you can use priv->spew_control to turn it off in
> data_available().
> 
> If you knew all that already, my apologies... :)  Sounds good to me!
> 
> Dan
> 
> 
> _______________________________________________
> networkmanager-list mailing list
> networkmanager-list gnome org
> http://mail.gnome.org/mailman/listinfo/networkmanager-list