Attention is currently required from: dexter. pespin 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 1:

(12 comments)

File src/pcu_l1_if.h:

https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/56eee3e7_3f6363d3 PS1, Line 34: struct gsm_e1_subslot; where does this come from? #include <osmocom/abis/e1_input.h>? better include it if its coming from a dependency library.

https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/f989a22e_7686b91f PS1, Line 182: int pcu_l1if_get_ccu_conn_pars(struct gsm_e1_subslot *e1_link, uint8_t pdch_ts, uint8_t pdch_trx_no); please keep same param names other functions. None has the pdch_ prefix.

File src/pcu_l1_if.cpp:

https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/0d56e7dc_23dd37dc PS1, Line 77: uint8_t trx_no; "trx_no" but "ts", let's better standarize variable names please.

https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/2dad1c76_dedbcaa0 PS1, Line 81: struct gsm_e1_subslot e1_link; type is subslot but name is link? this looks weird tbh.

https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/d0b2c0bc_bcacf08d PS1, Line 84: static LLIST_HEAD(e1_ccu_table); some comment describing this wuld be welcome.

https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/28440884_e5c689cb 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.

https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/5de5c980_a029a09d 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? pass "const struct gsm_e1_subslot **e1_link" or similar?

https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/d44bc411_ea85d64d PS1, Line 992: LOGP(DL1IF, LOGL_ERROR, "(PDCH-TS:%u, TRX:%u) cannot find E1 connection parameters for CCU\n", pdch_ts, This logging prefixes are not following any existing format in any osmocom code afaict.

https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/9dc8268d_e9fbe199 PS1, Line 1003: if (e1_ccu_ind->e1_ts_ss == 255) { don't we have defined for all these numbers?

https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/4c680978_887e3a38 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.

https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/6d63fbb1_e3f49758 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_conn_pars and don't need to use a loop here.

https://gerrit.osmocom.org/c/osmo-pcu/+/31174/comment/f30b4a36_2d20e748 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.