Re: ModemManager API review

[Dan Williams <dcbw redhat com>]

On Tue, 2008-11-04 at 10:04 +0100, Marcel Holtmann wrote:
> Hi Tambet,
> 
> >> 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.
> 
> I don't see how that can be. It was the HSO card and it might need  
> more tweaks than usual. Besides kernel patches ;)

hso cards _do_ need more tweaks than usual.  They have either one or two
control ports, but the HAL magic here is somewhat lacking.  We need to
"tag" the various ports with "control" or "data" or "gps" or "control2"
in addition to determining the command sets.  What you're seeing is HAL
not being able to tag the ports correctly, because the only way to know
this is to check hso-specific sysfs files.  We really need a generic way
to do this in the kernel drivers.

What _should_ be happening here is that (once Kay creates the repo) that
a udev prober checks out each serial port's supported command sets, and
stores that info in the udev database.  Then, either MM can read that
information directly, or we create a small HAL callout that reads the
udev database and adds the right modem.command_sets items for backwards
compatibility.  Then we kill 10-modem.fdi and everyone is happy.

In short, 'hso' is special and HAL is missing some stuff.  It's not
really MM's fault.

Dan

> >> 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.
> 
> Then just rename the interface to Device and everything will be fine.
> 
> >> 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.
> 
> Actually it doesn't. You should have some kind of method to setup your  
> device. So for example call Device.Setup(dict). You can give it a key  
> "Number" or key "APN" and then it will do the right calls.
> 
> Actually the magic ATD*99*# crap is just something ETSI invented  
> because every OS was using weird and broken dialer scripts at that  
> time. There are proper commands when attaching a GPRS/UMTS link. Also  
> the HSO based cards don't use numbers.
> 
> If you do the Setup method call once, then you can call Connect/ 
> Disconnect multiple times without modifying your setup or giving it a  
> number all the time. It just no consistent to give the APN one way and  
> the number the other way. And also some device don't use numbers at  
> all. We should really hide this implementation detail.
> 
> >> 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.
> 
> It is weird in conjunction with Connect/Disconnect and then Enable  
> takes a parameter where TRUE means "enable" and FALSE means "disble".
> 
> > 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.
> 
> The async here means if other applications wants to monitor the  
> status. For example if you have an applet just indicating if it is  
> connected or not (but never calling Connect by itself). For that we  
> need the signals.
> 
> >> 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.
> 
> We can cache the properties. Since when doing Enable we can issue the  
> AT commands for the values that are not changing and then just cache  
> them.
> 
> >> 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.
> 
> I don't care about the Enable and how it is done, but you have to see  
> the whole picture here. Doing Connect/Disconnect means for me I do  
> Enable/Disable. Nevermind though.
> 
> Using properties for settings that can change or for others that a  
> pure informational is really a big advantage. I have been down the  
> road with something like 20 Set* and Get* methods and it is just  
> painful for the application programmer using the the API since at some  
> point they need to call these methods anyway. I know it is nice from  
> the command line in Python to just do GetEmei instead of  
> GetProperties()["IMEI"], but when you look at the possible users of  
> ModemManager it is different. For one you have NetworkManager and in  
> some other cases you have a standalone UI that wants to access  
> ModemManager on its own. Using GetProperties, SetProperty and  
> PropertyChanged really worked out nicely and reduced the complexity in  
> the application.
> 
> Remember that doing blocking D-Bus calls is always a bad idea in  
> interactive applications so calling one method and handling the error  
> cases is a lot simpler here.
> 
> >> 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.
> 
> I disagree here. Debugging enums is just a pain. Having clear string  
> values is nice and all high-level languages don't care. And even in C  
> it is simple enough.
> 
> >> 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! :)
> 
> Hah.
> 
> >> 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?
> 
> This goes more in the direction of roaming. You could attach the  
> signal strength just to the Device object since that is on a per  
> device basis and not network basis. Even if it is for that particular  
> network, but as it happens you only care about the current connected  
> one.
> 
> So do we actually care that outside your own country (or sometimes  
> even in the country) you could do roam and should we really bother  
> supporting this in that detail. If yes, then putting the available  
> networks on objects. If not then we might not bother at all. I can  
> make up use cases where you wanna lock yourself to a specific network.  
> There are some spots in Europe where you would roam to a different  
> country, but do we really care or should better do something like the  
> iPhone in allowing/disallowing data roaming.
> 
> Regards
> 
> Marcel
> 
> _______________________________________________
> NetworkManager-list mailing list
> NetworkManager-list gnome org
> http://mail.gnome.org/mailman/listinfo/networkmanager-list