Patch Set 2: Code-Review-1 (5 comments)
https://gerrit.osmocom.org/#/c/4704/2/src/osmo_gsm_tester/bts_octphy.py File src/osmo_gsm_tester/bts_octphy.py: Line 162: 'location_area_code': self.suite_run.lac(), -1: see sysmo https://gerrit.osmocom.org/#/c/4704/2/src/osmo_gsm_tester/bts_osmotrx.py File src/osmo_gsm_tester/bts_osmotrx.py: Line 140: 'location_area_code': self.suite_run.lac(), -1: see sysmo https://gerrit.osmocom.org/#/c/4704/2/src/osmo_gsm_tester/bts_sysmo.py File src/osmo_gsm_tester/bts_sysmo.py: Line 147: 'location_area_code': self.suite_run.lac(), -1: Each BTS object must get a LAC *once*, conf_for_bsc() should be a "const" function. With this code, if a test case were to call conf_for_bsc() twice, then the same BTS object would exhibit mismatching LACs. I guess we should have a set_lac() function like we have set_msisdn() for the modems. We can call set_lac(suite_run.lac()) in bts_obj() in suite.py. Then overlay self.lac here. (either implement a set_lac() for each BTS class, or we could start having a BtsBase class which the others inherit from, so we have set_lac() implemented exactly once.) https://gerrit.osmocom.org/#/c/4704/2/src/osmo_gsm_tester/resource.py File src/osmo_gsm_tester/resource.py: Line 215: return self.next_persistent_value('lac', '1', schema.uint16, lambda x: str((int(x)+1) % pow(2,16)), origin) -1: What if x starts out as -23? maybe str(max(0, ... )); 0: two spaces before pow; 0: In C I'd use (int(x)+1) & 0xffff but in python, ok; 0: The magic number "pow(2,16)" is used a lot, Does it make sense to have a LAC_MAX constant defined once somewhere? Is this the only one outside of the uint16() function? https://gerrit.osmocom.org/#/c/4704/2/src/osmo_gsm_tester/schema.py File src/osmo_gsm_tester/schema.py: Line 78: if n >= pow(2,16): 0: lol, why not just > 65535 ... python goes on to calculate 2 ** 16 every time. -- To view, visit https://gerrit.osmocom.org/4704 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9f864bac05e39ec2fc305f774194799c3d8fe1b0 Gerrit-PatchSet: 2 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