Attention is currently required from: fixeria, pespin. dexter has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/31174 )
Change subject: pcu_l1_if: receive E1 connection parameters ......................................................................
Patch Set 2:
(11 comments)
File src/pcu_l1_if.cpp:
https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/a627bb0d_bfcb25a8 PS1, Line 77: uint8_t trx_no;
"trx_no" but "ts", let's better standarize variable names please.
Done
https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/af20f1c2_7cbc329e PS1, Line 81: struct gsm_e1_subslot e1_link;
type is subslot but name is link? this looks weird tbh.
This is inherited from osmo-bsc, there the name e1_link is very common. The struct just contains the e1 line number along with the timeslot number and the subslot number.
https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/c8452857_bae0bfc5 PS1, Line 84: static LLIST_HEAD(e1_ccu_table);
some comment describing this wuld be welcome.
Done
https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/d71415fa_bb4006d3 PS1, Line 981: int pcu_l1if_get_ccu_conn_pars(struct gsm_e1_subslot *e1_link, uint8_t pdch_ts, uint8_t pdch_trx_no)
trx should go before ts.
Done
https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/d5401eb9_1adee3b6 PS1, Line 987: memcpy(e1_link, &e1_ccu_conn_pars->e1_link, sizeof(*e1_link));
this looks a bit weird. Why copying the content if you can provide a const pointer to it? […]
Done
https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/deb3b54d_e6c34919 PS1, Line 992: LOGP(DL1IF, LOGL_ERROR, "(PDCH-TS:%u, TRX:%u) cannot find E1 connection parameters for CCU\n", pdch_ts,
Indeed, would be nice to have the format consistent with LOGPDCH().
Done
https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/f35dd08d_92c31991 PS1, Line 1003: if (e1_ccu_ind->e1_ts_ss == 255) {
don't we have defined for all these numbers?
As far as I know not. The 255 is also used in osmo-bsc:bts_trx_vty.c like this. The two variables are also only used for logging.
https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/c7745883_68c30932 PS1, Line 1012: "(PDCH-TS:%u, TRX:%u) new E1 CCU communication parameters for CCU (E1-line:%u, E1-TS:%u, E1-SS:%u, rate:%ukbps)\n",
same, prefix formatting.
Done
https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/808bd281_9a0b108e PS1, Line 1017: llist_for_each_entry(e1_ccu_conn_pars, &e1_ccu_table, entry) {
If you passed a pointer to the pcu_l1if_get_ccu_conn_pars() above you could get a pointer to e1_ccu_ […]
Done
https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/94b4cb08_875114f9 PS1, Line 1027: e1_ccu_conn_pars = talloc_zero(tall_pcu_ctx, struct e1_ccu_conn_pars);
You can probably move this into an _alloc() function.
I don't think that this is necessary. This is the only location where we need to alloc the connection parameters.
https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/74f73db9_159c7e6c PS1, Line 1271: pcu_prim->u.info_ind
copy-paste, should be 'e1_ccu_ind' instead
Done
-
dexter