Attention is currently required from: fixeria, dexter.
19 comments:
Commit Message:
typo
Patchset:
looks good in general! let's just polish some details...
File doc/timeslot.msc:
Patch Set #2, 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...
\nfor all timeslots in state UNUSED:"
UNUSED
Patch Set #2, 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:
Patch Set #2, 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: */
Patch Set #2, Line 42: TS_EV_PDCH_DEACT_NACK,
/* An Ericsson PCU has disconnected from OsmoBSC, deactivate PDCH: */
Patch Set #2, Line 43: TS_EV_PDCH_DEACT,
/* An Ericsson PCU has reconnected to OsmoBSC, re-activate PDCH: */
File src/osmo-bsc/pcu_sock.c:
Patch Set #2, 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?
Patch Set #2, 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
Patch Set #2, 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.
Patch Set #2, 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.
Patch Set #2, Line 940: /* FIXME: allow multiple BTS */
what makes it hard to implement this now instead of adding a FIXME comment?
Patch Set #2, 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?
Patch Set #2, 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
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
Patch Set #2, 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:
Patch Set #2, 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)
To view, visit change 31497. To unsubscribe, or for help writing mail filters, visit settings.