Hi Andreas, Harald,
the recent "SI generation" fix broke multi trx setups. This is because when the first (c0) TRX is going up bts->si_valid will include SI2 and other SIs scheduled on the BCCH and once the second trx is going up the code decides it only needs to generate the SI5 (and more).
But si_valid still includes SI2 and it will be set on the second trx. I have fixed it locally by setting si_valid to 0 early in the set_system_infos routine and added a warning for this case.
How should this be properly fixed for master? Will the content of a specific SI differ from TRX0 to TRX1 of a BTS? Should the si_valid be moved to the trx data structure?
I have attached my local patch and would like to have a review for it.
comments? ideas?
holger
hi holger,
since all SI (5 and 6) are equal for all TRX of BTS, the si_valid should stay part of BTS structure. resetting it will break the static assignment feature via VTY.
the first part of my patch ensures that only generated SI are set to valid. other static SI may be valid or not, as specified via VTY.
the second part calls rsl_si() with only the required SI.
i think it is good to keep your error check at rsl_si().
regards,
andreas
Holger Hans Peter Freyther wrote:
Hi Andreas, Harald,
the recent "SI generation" fix broke multi trx setups. This is because when the first (c0) TRX is going up bts->si_valid will include SI2 and other SIs scheduled on the BCCH and once the second trx is going up the code decides it only needs to generate the SI5 (and more).
But si_valid still includes SI2 and it will be set on the second trx. I have fixed it locally by setting si_valid to 0 early in the set_system_infos routine and added a warning for this case.
How should this be properly fixed for master? Will the content of a specific SI differ from TRX0 to TRX1 of a BTS? Should the si_valid be moved to the trx data structure?
I have attached my local patch and would like to have a review for it.
comments? ideas?
holger
From bde31667fdd182268fe2172c8c5e21b0c7d085c1 Mon Sep 17 00:00:00 2001
From: Holger Hans Peter Freyther zecke@selfish.org Date: Mon, 8 Oct 2012 22:12:13 +0200 Subject: [PATCH] bsc_init: Forget which SIs are valid for the trx.
The recent commit to improve the SI generation lead to setting the BCCH SIs for all TRX in a multi-trx setup. This is because we create the SIs globally but si_valid appears to be limited to the 'current' trx. Warn if we attempt to set SIs for the BCCH on a trx that does not have a BCCH.
openbsc/src/libbsc/bsc_init.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/openbsc/src/libbsc/bsc_init.c b/openbsc/src/libbsc/bsc_init.c index e05aec7..7a60e04 100644 --- a/openbsc/src/libbsc/bsc_init.c +++ b/openbsc/src/libbsc/bsc_init.c @@ -129,6 +129,11 @@ static int rsl_si(struct gsm_bts_trx *trx, enum osmo_sysinfo_type i, int si_len) GSM_BTS_SI(bts, i), si_len); break; default:
if (bts->c0 != trx)LOGP(DRR, LOGL_ERROR,"Attempting to set BCCH SI%s on wrong BTS/TRX (%d/%d)\n",get_value_string(osmo_sitype_strs, i), rc = rsl_bcch_info(trx, osmo_sitype2rsl(i), GSM_BTS_SI(bts, i), si_len); break;bts->nr, trx->nr);@@ -149,6 +154,9 @@ static int set_system_infos(struct gsm_bts_trx *trx) ms_pwr_ctl_lvl(bts->band, bts->ms_max_power); bts->si_common.cell_sel_par.neci = bts->network->neci;
/* Zero, forget the state of the SIs */
bts->si_valid = 0;
/* First, we determine which of the SI messages we actually need */
if (trx == bts->c0) {
On Tue, Oct 09, 2012 at 12:56:54PM +0200, jolly wrote:
hi holger,
since all SI (5 and 6) are equal for all TRX of BTS, the si_valid should stay part of BTS structure. resetting it will break the static assignment feature via VTY.
what is the reason to re-generate some of the SI when the n-th trx is up? Shouldn't we generate the SI once and then based on the TRX decide which one (gen_si) to send to the BTS?
the first part of my patch ensures that only generated SI are set to valid. other static SI may be valid or not, as specified via VTY.
patch looks fine (didn't review and I don't know much about the static SI generation). Please commit.
On Tue, Oct 09, 2012 at 05:32:23PM +0200, jolly wrote:
what is the reason to re-generate some of the SI when the n-th trx is up? Shouldn't we generate the SI once and then based on the TRX decide which one (gen_si) to send to the BTS?
here is an update of my patch. it generates only once now.
well, I think doing it this way is not really helping. Comments and variables are called 'gen'(erate) and in the end the SIs might not be generated. It is quite easy to miss the consequence of the conditions. Right now a config change will be reflected the next time the BTS (and its TRX) are up, after this change one would need to restart OpenBSC.
My comment was meant as an ecouragement to check if we could separate the SI generation from the place where we select which ones to send to the BTS.
holger
Holger Hans Peter Freyther wrote:
My comment was meant as an ecouragement to check if we could separate the SI generation from the place where we select which ones to send to the BTS.
hi holger,
if we want changes of config to be present in the SI whenever a TRX comes up, it is required to generate it when this happens. (so forget my last patch, because it does not do it and also it does it wrong.) i have no better solution in mind right now. can you test my attached patch before i commit it, because i don't have multi TRX setup?
regards,
andreas
On Wed, Oct 10, 2012 at 09:19:07AM +0200, jolly wrote:
Hi,
if we want changes of config to be present in the SI whenever a TRX comes up, it is required to generate it when this happens. (so forget my last patch, because it does not do it and also it does it wrong.) i have no better solution in mind right now. can you test my attached patch before i commit it, because i don't have multi TRX setup?
do you have time to test this yourself? I have already sent an email how to easily install the FakeBTS code on Debian 6.0 (and later), and now I pushed my dual trx implementation and updated the package.
$ gst st: PackageLoader fileInPackage: #FakeBTS. st: dual := FakeBTS.DualTrxBTS new btsId: '1/0/0'; yourself st: dual connect: 'localhost'; waitForBTSReady.
this will create a DualTRX BTS with unit id 1 and connect it to the localhost. If you want to re-connect just call the last line again.
cheers holger
Holger Hans Peter Freyther wrote:
do you have time to test this yourself? I have already sent an email how to easily install the FakeBTS code on Debian 6.0 (and later), and now I pushed my dual trx implementation and updated the package.
$ gst st: PackageLoader fileInPackage: #FakeBTS. st: dual := FakeBTS.DualTrxBTS new btsId: '1/0/0'; yourself st: dual connect: 'localhost'; waitForBTSReady.
this will create a DualTRX BTS with unit id 1 and connect it to the localhost. If you want to re-connect just call the last line again.
hi holger,
... <0019> input/ipa.c:322 accept()ed new link from 127.0.0.1 to port 3003 <0004> bsc_init.c:284 bootstrapping RSL for BTS/TRX (0/0) on ARFCN 871 using MCC=1 MNC=1 LAC=1 CID=0 BSIC=63 TSC=7 <0003> system_information.c:333 Serving cell: 871 873 <0003> bsc_init.c:104 SI1: 55 06 19 8f b3 a0 00 00 00 00 00 00 00 00 00 00 00 00 00 e5 04 00 2b <0003> bsc_init.c:104 SI2: 59 06 1a 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ff e5 04 00 <0003> bsc_init.c:104 SI3: 49 06 1b 00 00 00 f1 10 00 01 49 03 00 27 47 40 e5 04 00 3b 2b 2b 2b <0003> bsc_init.c:104 SI4: 31 06 1c 00 f1 10 00 01 47 40 e5 04 00 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b <0003> bsc_init.c:104 SI5: 49 06 1d 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 2b 2b 2b 2b <0003> bsc_init.c:104 SI6: 2d 06 1e 00 00 00 f1 10 00 01 27 ff 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b ... <0019> input/ipa.c:322 accept()ed new link from 127.0.0.1 to port 3003 <0004> bsc_init.c:284 bootstrapping RSL for BTS/TRX (0/1) on ARFCN 873 using MCC=1 MNC=1 LAC=1 CID=0 BSIC=63 TSC=7 <0003> bsc_init.c:104 SI5: 49 06 1d 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 2b 2b 2b 2b <0003> bsc_init.c:104 SI6: 2d 06 1e 00 00 00 f1 10 00 01 27 ff 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b 2b ...
as you can see, it works. i have also tested it with BS11 and forced voice traffic to second TRX. it works. you can apply my patch.
regards,
andreas