GNOME.org
 

Re: ModemManager API review

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 ;)
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
[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]