pseudomodem: Implement Enable. [chromiumos/third_party/autotest : master]

[Arman Uguray (Code Review)]

Arman Uguray has uploaded a new change for review.

Change subject: pseudomodem: Implement Enable.
......................................................................

pseudomodem: Implement Enable.

This CL adds partial functionality that allows the pseudomodem to be
enabled without registering to a network. The state machine is
implemented as a step process that uses the modem state as its counter.

A minor change to the dbus_std_ifaces.DBusProperties constructor has
also been included, such that user provided properties are handled more
properly. The class also allows a dbus connection to be set at a later
time when this is not possible during initialization.

BUG=chromium-os:34605
TEST=I tested that the enable state changes function properly by sending
dbus messages to the pseudomodem using dbus-send.

Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
---
M client/cros/cellular/pseudomodem/dbus_std_ifaces.py
M client/cros/cellular/pseudomodem/modem.py
M client/cros/cellular/pseudomodem/modem_3gpp.py
3 files changed, 196 insertions(+), 27 deletions(-)


git pull ssh://gerrit.chromium.org:29418/chromiumos/third_party/autotest refs/changes/89/37589/1
--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 1
Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

[Thieu Le (Code Review)]

Thieu Le has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 2: (5 inline comments)

....................................................
File client/cros/cellular/pseudomodem/dbus_std_ifaces.py
Line 44: This serves as the abstract base class for all objects that expose
Delete 'abstract' since this base class has some implementation

Line 60: dbus.service.Object.__init__(self, bus, None)
Do you mean to default bus to something? Ie. dbus.SessionBus(), etc?

....................................................
File client/cros/cellular/pseudomodem/modem.py
Line 22: def ASSERT(condition):
We should put the asserts in some place that's common. I think we'll reuse this in other places.

Not sure about the naming convention, though...

Line 71: ', ignoring.')
Fix indent? Can't tell if it's off or if Chrome is messing up.

Line 73: mm1.MMCoreError.IN_PROGRESS, message)
Fix indent?

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 2

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

Gerrit-Reviewer: Arman Uguray <arma...@chromium.org>
Gerrit-Reviewer: Ben Chan <ben...@chromium.org>
Gerrit-Reviewer: Gerrit <chrom...@google.com>
Gerrit-Reviewer: Thieu Le <thi...@chromium.org>

[Arman Uguray (Code Review)]

Arman Uguray has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 2: (4 inline comments)

....................................................
File client/cros/cellular/pseudomodem/dbus_std_ifaces.py
Line 44: This serves as the abstract base class for all objects that expose

I thought having at least one abstract method qualifies a class as abstract.

Line 60: dbus.service.Object.__init__(self, bus, None)

No. The bus can either be passed to the constructor or set at a later time via SetBus but path has to be provided in the constructor. Also, dbus.service.Object.__ini__ requires that BOTH bus and path be present OR BOTH be None. The first if statement basically calls dbus.service.Object.__init__(self, None, None)

....................................................
File client/cros/cellular/pseudomodem/modem.py
Line 22: def ASSERT(condition):

Yeah, I also wonder whether there is an assertion mechanism in autotest already.

Line 71: ', ignoring.')

Yeah, I made a bad habit out of indenting a line 8 spaces if it's a continuation...

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 2

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

[Thieu Le (Code Review)]

Thieu Le has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 2: (2 inline comments)

....................................................
File client/cros/cellular/pseudomodem/dbus_std_ifaces.py
Line 60: dbus.service.Object.__init__(self, bus, None)
Ok, so you did mean to pass in (None, None). I saw (bus, None) and wasn't sure if you meant to default it to a bus. Probably better to just explicitly pass in (None, None).

....................................................
File client/cros/cellular/pseudomodem/modem.py
Line 22: def ASSERT(condition):

Looks like python has a built-in assert.
http://docs.python.org/2/reference/simple_stmts.html

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 2

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

[Arman Uguray (Code Review)]

Arman Uguray has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 2: (3 inline comments)

....................................................
File client/cros/cellular/pseudomodem/dbus_std_ifaces.py
Line 60: dbus.service.Object.__init__(self, bus, None)

Done

....................................................
File client/cros/cellular/pseudomodem/modem.py

Line 71: ', ignoring.')

Done

Line 73: mm1.MMCoreError.IN_PROGRESS, message)
Done

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 2

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

[Alex Miller (Code Review)]

Alex Miller has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 5: (6 inline comments)

....................................................
File client/cros/cellular/pseudomodem/autotest_common.py
Line 1: # Copyright (c) 2012 The Chromium OS Authors. All rights reserved.
This file should be an exact copy of the common.py that you can find elsewhere in autotest, with only the |client_dir =| line changed to point to the client folder via a relative path.

Please keep the name as common.py.

....................................................
File client/cros/cellular/pseudomodem/dbus_std_ifaces.py
Line 54: path -- The DBus object path of this object.
config?

Line 58: raise TypeError('A value for "path" has to be provided.')
If you have to provide a value, why are you providing a default argument of |None|?

Line 59: if not bus:
This would make more sense if you flipped this whole if block to do |if bus|.

....................................................
File client/cros/cellular/pseudomodem/modem.py
Line 19: from autotest_lib.client.common_lib import error
I can't seem to find what in your code needs this import?

Line 273: band_list.append(dbus.types.UInt32(band))
band_list = [dbus.types.UInt32(band) for band in bands]

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 5

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

Gerrit-Reviewer: Alex Miller <mill...@chromium.org>

[Arman Uguray (Code Review)]

Arman Uguray has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 5: (6 inline comments)

....................................................
File client/cros/cellular/pseudomodem/autotest_common.py
Line 1: # Copyright (c) 2012 The Chromium OS Authors. All rights reserved.

Done

....................................................
File client/cros/cellular/pseudomodem/dbus_std_ifaces.py
Line 54: path -- The DBus object path of this object.

Done

Line 58: raise TypeError('A value for "path" has to be provided.')

Done

Line 59: if not bus:

Done

....................................................
File client/cros/cellular/pseudomodem/modem.py
Line 19: from autotest_lib.client.common_lib import error

Done

Line 273: band_list.append(dbus.types.UInt32(band))
Done

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 5

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

[Alex Miller (Code Review)]

Alex Miller has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 6: Looks good to me, but someone else must approve

(1 inline comment)

....................................................
File client/cros/cellular/pseudomodem/dbus_std_ifaces.py
Line 68: 'not "None".'))
You can drop this check entirely. If you start passing None into functions that expect an object, I find crashing horribly a perfectly acceptable outcome.

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

[Thieu Le (Code Review)]

Thieu Le has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 6: (5 inline comments)

....................................................
File client/cros/cellular/pseudomodem/modem.py
Line 41: if self.modem.register_step:
The EnableStep.Cancel() should only cancel items related to EnableStep, like removing any pending tasks in the main loop that EnableStep queued to it. A higher level Modem.Cancel() should be responsible for cancelling the active steps.

Line 60: ', ignoring.')
Fix indent

Line 99: self.modem.RegisterWithNetwork()
You'll also need to handle other states here (ie. registered, connected, etc).

Line 187: val = None
Don't need to explicitly initialize |val| here since you end up setting |val| in the following if/else block

Line 196: def Enable(self, enable):
We're going to need a way to delay the completion of this call for some tests. A TODO here is sufficient.

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

[Arman Uguray (Code Review)]

Arman Uguray has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 6: (5 inline comments)

....................................................

File client/cros/cellular/pseudomodem/dbus_std_ifaces.py
Line 68: 'not "None".'))

I need path to be not None, because I need to hold on to it if a bus is provided later on. If path is None AND bus is None, dbus.service.Object.__init__ will not fail. But when SetBus is called later, then I have to pass the path to dbus.service.Object.add_to_connection. I much prefer objects to set their own paths during initialization as opposed to someone else providing the path while calling SetBus. So, my choice here is not to crash horribly. I could, on the other hand, add an assertion here that checks that path is not None, but I think the error message makes debugging faster.

....................................................
File client/cros/cellular/pseudomodem/modem.py
Line 41: if self.modem.register_step:

Done

Line 60: ', ignoring.')

Done

Line 99: self.modem.RegisterWithNetwork()
Register is handled by RegisterWithNetwork, which I will upload in an upcoming CL.

Line 196: def Enable(self, enable):

Yep, please see the TODO above the StateMachine class declaration.

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

[Thieu Le (Code Review)]

Thieu Le has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 6: (2 inline comments)

....................................................
File client/cros/cellular/pseudomodem/modem.py
Line 99: self.modem.RegisterWithNetwork()
What if someone calls Enable() while in the connected state?

Line 196: def Enable(self, enable):

That'll work, as long as we can delay the DBus completion of Enable().

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

[Thieu Le (Code Review)]

Thieu Le has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 6: (1 inline comment)

....................................................
File client/cros/cellular/pseudomodem/modem.py
Line 99: self.modem.RegisterWithNetwork()

Nevermind :)

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 6

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

[Thieu Le (Code Review)]

Thieu Le has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 7: Looks good to me, but someone else must approve

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 7

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

[Ben Chan (Code Review)]

Ben Chan has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 7: (7 inline comments)

....................................................
File client/cros/cellular/pseudomodem/dbus_std_ifaces.py
Line 78: for k,pr in config:
style nit: space after comma

also, k, pr, v feel a bit too abbreviated

Line 79: for p,v in pr:
style nit: space after comma

Line 201: logging.info(('Properties Changed on interface: ' + interface_name +
nit: be consistent to either use string formatting or string concatenation

....................................................
File client/cros/cellular/pseudomodem/modem.py
Line 53: message = None
why assigning message = None?

Line 60: mm1.MMCoreError.IN_PROGRESS, message)
seems like it can fit in one line, no?

Line 179: old = self.Get(mm1.I_MODEM, 'State')
nit: old -> old_state

Line 185: val = None
why assigning val = None?

perhaps:

val = sim.path if sim else '/'

perhaps also defining '/' as a constant?

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 7

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

[Arman Uguray (Code Review)]

Arman Uguray has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 7: (7 inline comments)

....................................................
File client/cros/cellular/pseudomodem/dbus_std_ifaces.py
Line 78: for k,pr in config:

Done

Line 79: for p,v in pr:

Done

Line 201: logging.info(('Properties Changed on interface: ' + interface_name +

Done

....................................................
File client/cros/cellular/pseudomodem/modem.py
Line 53: message = None

I initially thought that |message| would go out of scope if I didn't declare it outside of the if statement. This is obviously not true in Python, so I'll remove that line.

Line 60: mm1.MMCoreError.IN_PROGRESS, message)
Done

Line 179: old = self.Get(mm1.I_MODEM, 'State')

Done

Line 185: val = None

Done

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 7

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

[Ben Chan (Code Review)]

Ben Chan has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 8: Looks good to me, approved

(1 inline comment)

one nit, no need for extra turnaround

....................................................
File client/cros/cellular/pseudomodem/mm1.py
Line 17: ROOT_PATH = '/'
INVALID_SIM_OBJECT_PATH?

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 8

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

[Arman Uguray (Code Review)]

Arman Uguray has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 8: Verified; Ready

(1 inline comment)

....................................................
File client/cros/cellular/pseudomodem/mm1.py
Line 17: ROOT_PATH = '/'

Well it's not really an invalid path, since you can use it as a wildcard path e.g. Disconnect('/') means "disconnect all bearers".

Just checked http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-objectmanager, and they refer to '/' as the "root path".

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 8

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

[Arman Uguray (Code Review)]

Arman Uguray has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 8: No score; Not Ready

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 8

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>

[Arman Uguray (Code Review)]

Arman Uguray has posted comments on this change.

Change subject: pseudomodem: Implement Enable.
......................................................................

Patch Set 9: Verified; Looks good to me, approved; Ready

--
To view, visit https://gerrit.chromium.org/gerrit/37589
To unsubscribe, visit https://gerrit.chromium.org/gerrit/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I32d57d3664eb4ef3acb0ef46464736b39e8fd955
Gerrit-PatchSet: 9

Gerrit-Project: chromiumos/third_party/autotest
Gerrit-Branch: master
Gerrit-Owner: Arman Uguray <arma...@chromium.org>