Patch Set 3: (13 comments)
About blocking/non-blocking in general: in this patch review I am assuming that we want register() and connect() to remain blocking. If we however would wait for N modems to Scan(), a test run could take quite a long time. It would indeed be an improvement to Scan() in parallel. However, the DBus call to Scan() is so far blocking anyway. So let us keep the entire thing blocking until we maybe introduce proper non-blocking connect behavior later. (You could argue that connect() was non-blocking before, but that was actually just a side effect of us merely switching on the modem. The intention is to keep the test API blocking. We will still query the NITB/MSC whether the modems are reported as subscribed after the blocking registration.) https://gerrit.osmocom.org/#/c/2779/3/src/osmo_gsm_tester/ofono_client.py File src/osmo_gsm_tester/ofono_client.py: Line 39: MAX_REGISTER_ATTEMPTS = 3 maybe also have the NETREG_ prefix for this value? Line 346: self.dbg('%r.PropertyChanged() -> %s=%s' % (I_NETREG, name, value)) it's IMHO nice to have this, but maybe in a separate patch because it is technically unrelated from the rest (same goes for the PropertyChanged signal registration above) Line 351: except Exception: Rather catch the specific exception you are expecting here. KeyError? Line 353: return self.netreg_status why store netreg_status in self? Line 355: def is_roaming(self): do we need this? Line 365: raise RuntimeError('Failed to find Network Operator', self.mcc, self.mnc) self.raise_exn('Failed to find Network Operator', mcc=self.mcc, mnc=self.mnc) Line 367: self.dbg('Failed to find Network Operator', self.mcc, self.mnc, 'attempt', attempts) also use kwargs like above mcc=self.mcc, mnc=self.mnc, attempt=attempts Line 368: defer(self.register_timeout, attempts + 1) you are waiting for the duration of a timeout? sounds like the wrong constant name. Line 376: nr.Scan() I don't think this needs a scan, does it? Line 405: def power_reset(self): 'power_cycle()'? ... power reset sounds like you're powering it down and it will be left off. Line 422: self.mnc = mnc Add input validation: * ensure that either both MCC and MNC or none of them are passed. * ensure MCC and MNC are numbers [1..999] (or check the range in the specs, or in our vty code) Maybe it would generally make sense to pass MCC+MNC in a tuple, because we are always using them together. Like in osmo_nitb and osmo_msc, have a function mcc_mnc() returning the tuple, in this class receive an mcc_mnc arg. Then we can also, like in Modem with msisdn, have an mccmnc_or_bsc arg, which calls mccmnc_or_bsc.mcc_mnc() in case it is not a tuple? In util.py we could have a valid_mcc_mnc() function to validate such a tuple, to do input validation here. Line 424: self.register_timeout(1) the name is unclear: "set the register timeout to 1 second"? For general code simplicity/readability: I would prefer to call register() here. In register(), I would prefer to have a loop trying N times instead of recursion. The loop guts could be a try_register() function returning True/False. Secondly: at first you are registering in a blocking way, but on the second attempt you defer(), i.e. this function here would return without the modem being registered. Avoid that with a loop (that remembers to poll the event_loop). I assume we would like registration to be blocking. Thirdly, why store MCC and MNC in self -- simply pass it to the register() function, which registers to that, and then there is no need to store them. I suggest: a register() call without args meaning register_default() and a register(mcc, mnc) call meaning to scan and pick a specific operator. https://gerrit.osmocom.org/#/c/2779/3/suites/aoip_debug/interactive.py File suites/aoip_debug/interactive.py: Line 20: m.connect(msc.mcc(), msc.mnc()) Do we really want this to be nonblocking? -- To view, visit https://gerrit.osmocom.org/2779 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I8d9eb47eac1044550d3885adb55105c304b0c15c Gerrit-PatchSet: 3 Gerrit-Project: osmo-gsm-tester Gerrit-Branch: master Gerrit-Owner: Pau Espin Pedrol <pes...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-HasComments: Yes
