Patch Set 1: (4 comments)
https://gerrit.osmocom.org/#/c/2449/1/src/osmo_gsm_tester/ofono_client.py File src/osmo_gsm_tester/ofono_client.py: Line 129: nr = self.dbus_obj()[I_NETREG] > Makes sense though I noticed now that dbus_obj() has no get() function >:( In current master I added code to check whether an interface is really present by trying to get via [] and catching an exception (because when I get the signal that an interface was added, sometimes the interface is not present in the dbus object yet -- it's racy). Maybe we can use that function here. (And now that I think of it we could change that function to use the 'in' operator instead, in case that works, probably for a separate patch) Line 140: def connect(self, nitb): > I agree. I can create a 2nd patch to do the connect()->register() yes, thx Line 148: self.log('Already registered with the network') > Yes, this is not really detailed but for now it's enough. Even in connect() In current master I already cycle powered and online to false and then true here ... so I went with the paradigm "connect() means re-connect" and that connect() is the First Step (tm) one takes to start using a modem. We could also move those clear-the-state things to a separate start() or reset() function, but test scripts might easily forget to call it, so I would still call the reset function from here (until a need arises to have them separate, my guess being there will not be any)... bottom line is, when you rebase to master you will see that there isn't really a need for this 'if' anymore. s/connect/register/g Line 154: self.dbg('Registered with network successfully: current status is %s', self.get_netreg_status()) > Do you think is really an issue? it's not like it's taking seconds to do it the issue really is that you obtain the same value twice, meaning you could technically get two different values. Something like this would be more, I don't know, "safe": def is_connected(netreg_status=None): if netreg_status is None: netreg_status = self.get_netreg_status() return netreg_status == ... def foo(): ns = self.get_netreg_status() if self.is_connected(ns): self.dbg('registered', netreg_status=ns) you are right that it's a little bit overboard for this less important place, nevertheless I'd prefer this coding style to be used all the time. s/connect/register/g -- To view, visit https://gerrit.osmocom.org/2449 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1db8c7cba8a83746c16e1ca45f4b8aa0d595caf8 Gerrit-PatchSet: 1 Gerrit-Project: osmo-gsm-tester Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Pau Espin Pedrol <[email protected]> Gerrit-Reviewer: neels <[email protected]> Gerrit-HasComments: Yes
