Re: ModemManager API review

["Tambet Ingo" <tambet gmail com>]

On Tue, Nov 4, 2008 at 3:54 AM, Marcel Holtmann <marcel holtmann org> wrote:
> So the first thing that draw me off is that we are stupidly mapping the HAL
> devices 1:1 to our devices. That is wrong. We should not do this. So for
> example my Option card has three TTYs and one network device. This all is
> one device. Currently it shows up as three devices. The number of TTY
> (control, data or whatever) is an implementation and should not be exposed
> via the API. So we have to be smart with this.

With the generic implementation, MM maps a HAL device with
"modem.command_sets" property as a single device. So if you got 3, it
means your HAL .fdi file is incorrect.

> The second thing is that the Manager interface talks about devices, while
> the main interface to the hardware is called Modem. So that should be
> consistent. Either we call them devices or modems.

Agreed. I initially had modems everywhere, but then thought it would
be more consistent in the big picture if it resembled
org.freedesktop.DeviceKit.Disks
(http://hal.freedesktop.org/docs/DeviceKit-disks/) interface. So
EnumerateModems was renamed to EnumerateDevices while the modem
interfaces didn't change. Just the reason behind it.

> The Modem interface has a Connect method call that takes a parameter number.
> This makes no sense whatsoever. Connect should not take any arguments it
> should connect with whatever has been configured or be smart and
> auto-configure it. Especially since you don't know if you are using a real
> number or actually an APN or something else.

Modem interface is for all modems. Landline, GSM, CDMA, ... It is up
to the specific implementation to validate and use the number. All the
modems I've ever seen (and the dial command 'D' in the spec) take a
number, so it makes perfect sense to me.

> And then we have Enable with a parameter. Don't do that. Just add Enable and
> Disable methods. Otherwise the API looks weird. Also signals like Connected,
> Enabled etc. are missing.

It doesn't look weird to me. In real life you don't have two switches
to turn things on/off, and to me it would look weird if my modem has
two physical buttons: one to turn it on, and another to turn it off.
In short it's a personal opinion and doesn't make much sense to argue
about.

There is no need for Connected and Enabled signals because the method
to Enable/Connect doesn't return before it's done. That does not mean
MM is not async, it accepts other commands and is responsive while
Enable/Connect/any other method is pending.

> So the split between Modem interface and Gsm.Card make no real sense to me.
> I would just convert everything into properties or create a GetProperties
> method to retrieve one dictionary with all the information. All the GetImei,
> GetImsi calls only create round-trips to D-Bus that can be avoided. If one
> technology doesn't have IMSI, then this property is just missing.

Again, it's your opinion. In my opinion, when I need IMEI, I don't
want the modem to issue 50 AT commands to get all the properties.

> And for setting things like the APN etc, you can use writable properties or
> a SetProperty method. So you could just set all properties and then call
> Connect. To make this fully async, a signal PropertyChanged would be needed,
> too.

So Enable(bool) looks weird to you and then you suggest the whole API
to consist of 2 methods (SetStuff() and GetStuff()). Again, your
personal opinion I don't agree with.

> And on that matter, please don't use enums since higher level languages
> don't really have the concept of includes from a C definition. So if you
> wanna give the band information you can just say "gsm900", "gsm1800" etc.
> Also for the mode having things like "connect", "connecting" etc. make it a
> lot easier to develop and debug. And when using dbus-monitor is shows up in
> clear text.

Every UI needs to translate enums to localized strings and back so all
the possible values need to be defined anyway. It's easier to do it
with integers than strings.

> Some things like GetRegistrationInfo are just better separated into
> properties or key/value pairs in a dictionary. That keeps the API small and
> also flexible for future changes.

This is something I finally agree on! :)

> So the network details on GSM are not really that interesting at all. I
> would leave them out for now. However I do think that representing every
> network as object path would be a better approach here.

Network details and the signal quality are the biggest reason I
started ModemManager, the most often asked features in this list. Why
would they need to be DBus objects with paths? What methods do you
think a network would have?

Tambet