Hi Holger!
I'm currently reviewing changeset 2d501ea26a219176b1c556449e45ebd90d4accfb and just noticed that you introduce a new 'int rf_locked' member to struct gsm_bts_trx, which I believe is not needed.
We are already tracking the administrative state of all objects in the BTS and mirror their state.
The specific state you are looking for should be in gsm_bts_trx.nm_state.administrative
I would appreciate if you could fix this.
On Wednesday 25 November 2009 20:47:28 Harald Welte wrote:
Hi Holger!
I'm currently reviewing changeset 2d501ea26a219176b1c556449e45ebd90d4accfb and just noticed that you introduce a new 'int rf_locked' member to struct gsm_bts_trx, which I believe is not needed.
We are already tracking the administrative state of all objects in the BTS and mirror their state.
The specific state you are looking for should be in gsm_bts_trx.nm_state.administrative
I would appreciate if you could fix this.
I will, honestly I was just too lazy to figure out if the state is updated, how it should be set on startup. I will try to fix it before FOSS.IN.
z.
On Thursday 26 November 2009 01:44:45 Holger Freyther wrote:
On Wednesday 25 November 2009 20:47:28 Harald Welte wrote:
I would appreciate if you could fix this.
I will, honestly I was just too lazy to figure out if the state is updated, how it should be set on startup. I will try to fix it before FOSS.IN.
Hi, I did not forget this. Today I was starting to set the nm_state.administrative of the trx and it is working for the _LOCKED start up. For the _UNLOCKED startup we seem to copy a new state to the nm_state which includes changing the administrative back to _LOCKED so the TRX is getting locked during startup. I tried using hardware watchpoints on the memory address in gdb but this made things quite slow (in the Xen instance I was testing).
I think I will have something working by tomorrow.
regards holger
On Sunday 20 December 2009 14:16:31 Holger Freyther wrote:
I think I will have something working by tomorrow.
Using administrative will not make us happy. From what I see in the 12.21 it has two different meanings. In one way it should be controlled by the BSC and in another way it can be used by the BTS to send information (MMI whatever that is...)
So when we set the administrative state of the trx to unlocked it will be changed in abis_nm_rx_statechg_rep to whatever we got from the BTS (mostly being locked).
One approach would be to never set administrative in abis_nm but then we would lose the state of the BTS... and we would not update it when we get acks..
So I was introducing administrative_bsc and administrative_bts to the nm_state struct and keep the bsc state in the bsc one and the bts field in the bts one. Now I'm kind of struggling with bringing up the BTS (I make it reboot...)
Harald do you have an idea of how it should look like?
On Monday 21 December 2009 10:35:20 Holger Freyther wrote:
On Sunday 20 December 2009 14:16:31 Holger Freyther wrote:
I think I will have something working by tomorrow.
Harald do you have an idea of how it should look like?
Okay, there is a heisenbug somewhere... (when compiled with -O0..)
This is the patch I would like to commit. The biggest change is in the abis_nm_rx_statechg_rep function to not use memcmp or assignment of the structs.
The semantic to the administrative state is that it will be assigned if it is 0 (default by the BSC) and ignored once it is set. It will still be updated in the ACK messages...
diff --git a/openbsc/include/openbsc/gsm_data.h b/openbsc/include/openbsc/gsm_data.h index f3ec9eb..da085fc 100644 --- a/openbsc/include/openbsc/gsm_data.h +++ b/openbsc/include/openbsc/gsm_data.h @@ -273,9 +273,6 @@ struct gsm_bts_trx { } bs11; }; struct gsm_bts_trx_ts ts[TRX_NR_TS]; - - /* NM state */ - int rf_locked; };
enum gsm_bts_type { diff --git a/openbsc/src/abis_nm.c b/openbsc/src/abis_nm.c index e7e3bf0..e7def1a 100644 --- a/openbsc/src/abis_nm.c +++ b/openbsc/src/abis_nm.c @@ -812,12 +812,17 @@ static int abis_nm_rx_statechg_rep(struct msgb *mb) } DEBUGPC(DNM, "\n");
- if (memcmp(&new_state, nm_state, sizeof(new_state))) { + if ((new_state.administrative != 0 && nm_state->administrative == 0) || + new_state.operational != nm_state->operational || + new_state.availability != nm_state->availability) { /* Update the operational state of a given object in our in-memory data * structures and send an event to the higher layer */ void *obj = objclass2obj(bts, foh->obj_class, &foh->obj_inst); rc = nm_state_event(EVT_STATECHG_OPER, foh->obj_class, obj, nm_state, &new_state); - *nm_state = new_state; + nm_state->operational = new_state.operational; + nm_state->availability = new_state.availability; + if (nm_state->administrative == 0) + nm_state->administrative = new_state.administrative; } #if 0 if (op_state == 1) { @@ -2781,7 +2786,7 @@ void gsm_trx_lock_rf(struct gsm_bts_trx *trx, int locked) { int new_state = locked ? NM_STATE_LOCKED : NM_STATE_UNLOCKED;
- trx->rf_locked = locked; + trx->nm_state.administrative = new_state; if (!trx->bts || !trx->bts->oml_link) return;
diff --git a/openbsc/src/bsc_init.c b/openbsc/src/bsc_init.c index 8450af6..d4e0616 100644 --- a/openbsc/src/bsc_init.c +++ b/openbsc/src/bsc_init.c @@ -429,8 +429,7 @@ static int sw_activ_rep(struct msgb *mb) * This code is here to make sure that on start * a TRX remains locked. */ - int rc_state = trx->rf_locked ? - NM_STATE_LOCKED : NM_STATE_UNLOCKED; + int rc_state = trx->nm_state.administrative; /* Patch ARFCN into radio attribute */ nanobts_attr_radio[5] &= 0xf0; nanobts_attr_radio[5] |= trx->arfcn >> 8; diff --git a/openbsc/src/gsm_data.c b/openbsc/src/gsm_data.c index 94ed91b..62576bd 100644 --- a/openbsc/src/gsm_data.c +++ b/openbsc/src/gsm_data.c @@ -126,6 +126,7 @@ struct gsm_bts_trx *gsm_bts_trx_alloc(struct gsm_bts *bts)
trx->bts = bts; trx->nr = bts->num_trx++; + trx->nm_state.administrative = NM_STATE_UNLOCKED;
for (k = 0; k < TRX_NR_TS; k++) { struct gsm_bts_trx_ts *ts = &trx->ts[k];
On Mon, Dec 21, 2009 at 10:35:20AM +0100, Holger Freyther wrote:
On Sunday 20 December 2009 14:16:31 Holger Freyther wrote:
I think I will have something working by tomorrow.
Using administrative will not make us happy. From what I see in the 12.21 it has two different meanings. In one way it should be controlled by the BSC and in another way it can be used by the BTS to send information (MMI whatever that is...)
MMI == man machine interface.
[...] Harald do you have an idea of how it should look like?
I really don't understand the problem, sorry.
-
Harald Welte -
Holger Freyther