Attention is currently required from: dexter.

View Change

12 comments:

• File src/pcu_l1_if.h:

  • Patch Set #1, 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.

  • Patch Set #1, 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:

  • Patch Set #1, Line 77: uint8_t trx_no;

    "trx_no" but "ts", let's better standarize variable names please.

  • Patch Set #1, Line 81: struct gsm_e1_subslot e1_link;

    type is subslot but name is link? this looks weird tbh.

  • Patch Set #1, Line 84: static LLIST_HEAD(e1_ccu_table);

    some comment describing this wuld be welcome.

  • Patch Set #1, 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.

  • Patch Set #1, 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?

  • Patch Set #1, 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.

  • Patch Set #1, Line 1003: if (e1_ccu_ind->e1_ts_ss == 255) {

    don't we have defined for all these numbers?

  • Patch Set #1, 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.

  • Patch Set #1, 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.

  • Patch Set #1, 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 change 31174. To unsubscribe, or for help writing mail filters, visit settings.