Patch Set 1: Code-Review-1 (9 comments)
https://gerrit.osmocom.org/#/c/2692/1/example/defaults.conf File example/defaults.conf: Line 13: cell_identity: 0 i.e. for now all BTS have a cell_identity == 0? Main Q: How do you intend to populate the cell_identity? (BTW same goes for the arfcn below --> OS#2230 https://osmocom.org/issues/2230 and possibly other values; so we could agree to resolve this later, but would be nice to resolve it now if we can without too much effort) a) We could require each BTS configured in the resources config to be given a manual cell identity. That could be the easiest solution, I think I would go there for now. (and would be good to change the arfcn to that, too) b) We could give each BTS a cell identity according to the [index] in the nitb's list of BTSes (...see other comment). c) We could give each new BTS an ascending cell identity much like the phone numbers for modems. Then we are sure that for each run we're using otherwise unknown cell identities. In none of the above cases do we want this in defaults.conf. We never want all cells to have the same id. (And we never want all to use the same arfcn, either, which is my fault / missing implementation.) https://gerrit.osmocom.org/#/c/2692/1/selftest/template_test/osmo-nitb.cfg.tmpl File selftest/template_test/osmo-nitb.cfg.tmpl: Line 62: cell_identity ${bts.cell_identity} here we could use loop.index *if* we wanted the NITB's [index] https://gerrit.osmocom.org/#/c/2692/1/src/osmo_gsm_tester/bts_sysmo.py File src/osmo_gsm_tester/bts_sysmo.py: Line 40: self._conf_for_ntib = None typo: 'nitb'. Note that I am renaming 'nitb' to 'bsc' in most places (to accomodate separate BSC+MSC processes, and because we're talking about the BSC side of the NITB anyway). That code is still on a branch until tested properly. Line 135: self._conf_for_ntib = config.get_defaults('nitb_bts') > I think it may be a good idea to do all the overlay stuff inside configure( configure() is intended for the configuration to run the BTS (osmo-bts-sysmo.cfg) -- but conf_for_nitb() is passed over to the OsmoNitb to end up in osmo-nitb.cfg / osmo-bsc.cfg. So these are called at distinct times and should be independent: we call configure() when we start the BTS. But we call conf_for_nitb() when we start the NITB, which is before that. (btw, this will be conf_for_bsc() soon) I guess I should add code doc to indicate this difference. Line 139: return self._conf_for_ntib ntib: at least you're consistent ;) Line 145: return int(self.conf_for_nitb()['cell_identity']) Let's not store the for_nitb config in the BTS, this would duplicate values / break scoping. If there are values that the OsmoBts instance must know, it should be part of the 'bts' config (and copied into the for_nitb conf); conf_for_nitb() should be called exactly once for when the NITB generates its config file. * if we derive the BTS cell identity from the [index] of the nitb's list, then the NITB could figure out which index the given BTS is at and use that. So it would not need to store in the BTS struct. Arguably that's not very beautiful code but would work. * if we deal out new cell ids incrementally (like phone numbers), then we should store it in the 'bts' resource (like we store the MSISDN in the 'modem' resource). Above overlay(conf_for_nitb, self.conf) would then put the individual cell ID into the struct communicated to the NITB. Later on we fetch the cell id from self.conf, not conf_for_nitb. * if we have manual cell ids, then same: cell id in BTS conf, just without the automatic cell id generation. (I guess I would go for this first and in a second step add incremental cell id creation, i.e. just create a low-prio 'Feature' issue for it. And have a check in the nitb that warns if the user gave two BTS the same cell id maybe.) https://gerrit.osmocom.org/#/c/2692/1/src/osmo_gsm_tester/osmo_nitb.py File src/osmo_gsm_tester/osmo_nitb.py: Line 155 correct but unrelated. rather submit in a separate patch. Line 196: ctrl.do_get(OsmoNitbCtrl.BTS_CELL_IDENTITY_VAR % i) hmm, what if the number of connected BTS changes between count_bts_connected() and this? This CTRL back-and-forth looks rather unholy, looks like grown spaghetti. I think we may need a tailored race-less CTRL command, to provide all info we need in a single call (i.e. fix the BSC side so this code can be simpler + safer). Line 219: (drop some blank lines) -- To view, visit https://gerrit.osmocom.org/2692 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1bbf9ac2b05d95a113ed4ad5e93093431496d44e 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-HasComments: Yes
