Attention is currently required from: fixeria, dexter.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31497 )
Change subject: pcu_sock: activate/deactivate PDCH on pcu reconnect ......................................................................
Patch Set 2:
(19 comments)
Commit Message:
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/ed4cd07a_aeea2adf PS2, Line 10: po typo
Patchset:
PS2: looks good in general! let's just polish some details...
File doc/timeslot.msc:
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/9c435994_9ac70549 PS2, Line 95: bsc_ts abox bsc_ts [label="UNUSED"]; a timeslot FSM cannot transition from ST_PDCH directly to ST_UNUSED. Please insert here the actual transition as i see in the implementation:
bsc_ts abox bsc_ts [label="WAIT_PDCH_DEACT (4s, T23001)"]; bts <= bsc_ts [label="RSL RF Chan Release of PDCH",ID="Osmocom style"]; ...; bts => bsc_ts [label="RSL RF Chan Release ACK",ID="Osmocom style"]; bsc_ts abox bsc_ts [label="UNUSED"];
Same below for PDCH activation...
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/8c881ef0_8db1a260 PS2, Line 98: " \nfor all timeslots in state UNUSED:"
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/f329b054_31e0657c PS2, Line 99: PDCH UNUSED
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/63d8ad9a_d24aca23 PS2, Line 100: bsc_ts <- pcu_sock [label="TS_EV_PDCH_ACT"]; bsc_ts abox bsc_ts [label="WAIT_PDCH_ACT (4s, T23001)"]; bts <= bsc_ts [label="RSL Chan Activ of PDCH",ID="Osmocom style"]; ...; bts => bsc_ts [label="RSL RF Chan Activ ACK",ID="Osmocom style"]; bsc_ts abox bsc_ts [label="PDCH"];
File include/osmocom/bsc/timeslot_fsm.h:
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/9ecfa52e_5e39de9d PS2, Line 38: TS_EV_LCHAN_UNUSED, Would be nice to add comments to clarify how the "TS_EV_PDCH_*" are fundamentally different:
/* RSL responses received from the BTS: */
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/43ad28b0_6c1a3956 PS2, Line 42: TS_EV_PDCH_DEACT_NACK, /* An Ericsson PCU has disconnected from OsmoBSC, deactivate PDCH: */
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/895197c7_ad141738 PS2, Line 43: TS_EV_PDCH_DEACT, /* An Ericsson PCU has reconnected to OsmoBSC, re-activate PDCH: */
File src/osmo-bsc/pcu_sock.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/20e5fa50_64da4632 PS2, Line 801: printf("l1sap_chan_rel(trx,gsm_lchan2chan_nr(ts->lchan));\n"); either keep this printf, or if it should be dropped, drop it in a separate patch?
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/18d0ddda_bea86340 PS2, Line 794: for (i = 0; i < PCU_IF_NUM_TRX; i++) { this is a fix of a magic number, very good but rather in a separate patch
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/a5e5a216_bf4db00c PS2, Line 801: && ts->pchan_on_init == GSM_PCHAN_OSMO_DYN Why does this apply only to dynamic timeslots? I think I understand why, but I'd like to see a code comment saying so explicitly.
How about GSM_PCHAN_TCH_F_PDCH dynamic timeslots? I guess they don't apply to Ericsson RBS -- also good to clarify that in a comment.
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/013eb5ea_4a1592c5 PS2, Line 802: ts->fi->state == TS_ST_PDCH
Could you clarify why checking the FSM state is necessary? It's a bit weird to see code checking `fi […]
generally true, but here i agree with checking the state explicitly:
Receiving EV_PDCH_DEACT only makes sense when in ST_PDCH. We could add an allstate-event handling in timeslot_fsm.c that does this same check, but emitting EV_PDCH_DEACT logging for every single timeslot, in ST_PDCH or not, would be spammy.
I think my favorite would be exactly this code, but in a function implemented in timeslot_fsm.c and called from here:
void ts_pdch_deact(...) { if (ts->fi->state != TS_ST_PDCH) return; osmo_fsm_inst_dispatch(ts->fi, TS_EV_PDCH_DEACT); }
so exactly this code, but kept closer to the ts fsm implementation instead of spreading ts fsm behavior details into pcu_sock.c.
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/39790d92_377bf241 PS2, Line 940: /* FIXME: allow multiple BTS */ what makes it hard to implement this now instead of adding a FIXME comment?
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/55eb9537_4ea93938 PS2, Line 941: bts = llist_entry(state->net->bts_list.next, struct gsm_bts, list); rather use llist_first_entry_or_null()
but ... instead of always acting on the first BTS, shouldn't this find the exact BTS that the specific PCU is responsible for? .. I think I understand that this is a special case for RBS where the PCU is running next to the BSC instead of the usual next-to-BTS. Right? Add a code comment to clarify?
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/f16b4e18_e7f5e3c3 PS2, Line 973: gsm_bts_trx_num
As I already commented on one of your previous patches, using `gsm_bts_trx_num()` in a loop is not a […]
agree
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/b2c9d9ed_dc64dc7a PS2, Line 976: 8
ARRAY_SIZE(trx->ts)
could also use TRX_NR_TS, but ARRAY_SIZE is also my favorite; TRX_NR_TS is our legacy way, these days osmo code uses ARRAY_SIZE
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/cc3a9e76_beaeb054 PS2, Line 979: && ts->pchan_on_init == GSM_PCHAN_OSMO_DYN && ts->fi->state == TS_ST_UNUSED) { Also for GSM_PCHAN_TCH_F_PDCH ?
...complementary to above, here i would love to see a function ts_pdch_act() called, function implementation in timeslot_fsm.c, so that we don't add ts_fsm state logic outside of timeslot_fsm.c
File src/osmo-bsc/timeslot_fsm.c:
https://gerrit.osmocom.org/c/osmo-bsc/+/31497/comment/d0917b6a_27105e03 PS2, Line 326: static void ts_fsm_unused_pdch_act(struct osmo_fsm_inst *fi) it would be cool to add this function in a separate patch before this patch, so that in this patch the reader can easily see that the "if is_ericsson" part is being added and the rest remains unchanged (also if someone from the future reviews the git log)