Re: ModemManager API review



Tambet Ingo wrote:
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.

  
and this is HAL .fdi problem?:

dmesg:
[82410.648181] usb 2-2: new full speed USB device using uhci_hcd and address 2
[82410.821948] usb 2-2: configuration #1 chosen from 1 choice
[82411.050542] cdc_acm 2-2:1.1: ttyACM0: USB ACM device
[82411.053368] cdc_acm 2-2:1.3: ttyACM1: USB ACM device
[82411.055246] usbcore: registered new interface driver cdc_acm
[82411.055256] cdc_acm: v0.26:USB Abstract Control Model driver for USB modems and ISDN adapters

SE w810i, usb cable

t.

  
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
_______________________________________________
NetworkManager-list mailing list
NetworkManager-list gnome org
http://mail.gnome.org/mailman/listinfo/networkmanager-list
  



[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]