Patch Set 1: (7 comments)
looks good in principle, please don't be discouraged from the number of comments :) https://gerrit.osmocom.org/#/c/2449/1/src/osmo_gsm_tester/ofono_client.py File src/osmo_gsm_tester/ofono_client.py: Line 116: if interface_name == I_NETREG: elif? Line 129: nr = self.dbus_obj()[I_NETREG] this being called from is_connected() would raise a KeyError if the I_NETREG was not in the list. Instead, is_connected() should probably return False? So maybe return None in case there is no I_NETREG. Maybe this works: nr = self.dbus_obj().get(I_NETREG) if nr is None: return None What do you think? Line 138: return status == 'registered' or status == 'roaming' are 'registered' and 'roaming' fixed definitions from ofono? I hope so. Especially when we're using the same one several times ('roaming'), I would prefer using constants defined above, sort of like the I_NETREG. Line 140: def connect(self, nitb): hmm, I see a name duality arising: register and connect. Maybe we should rename all of them to 'register'? Line 148: self.log('Already registered with the network') what if the nitb arg reflects another network than the one we're connected to? I assume we usually want to disconnect and re-connect. Line 154: self.dbg('Registered with network successfully: current status is %s', self.get_netreg_status()) could call get_netreg_status() less often... Line 156: raise Exception('Failed to register with the network, current status is %s' % self.get_netreg_status()) so far I've been using RuntimeError (but we should also probably use more fine grained exceptions, like introduce an OfonoExn or ModemExn...) -- 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: neels <[email protected]> Gerrit-HasComments: Yes
