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. -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/31174 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I1f926b023205a9ecf78d8a8586d6ac21cc73eb0a Gerrit-Change-Number: 31174 Gerrit-PatchSet: 1 Gerrit-Owner: dexter <pmaier@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: pespin <pespin@sysmocom.de> Gerrit-Attention: dexter <pmaier@sysmocom.de> Gerrit-Comment-Date: Fri, 03 Feb 2023 16:33:53 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment