osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-pcu/+/22309 )
Change subject: Allow multiple bts objects in PCU ...................................................................... Patch Set 4: Code-Review-1 (11 comments) https://gerrit.osmocom.org/c/osmo-pcu/+/22309/3/src/bts.cpp File src/bts.cpp: https://gerrit.osmocom.org/c/osmo-pcu/+/22309/3/src/bts.cpp@1132 PS3, Line 1132: void bts_update_tbf_ta(struct gprs_rlcmac_bts *bts, const char *p, uint32_t fn, uint8_t trx_no, uint8_t ts, int8_t ta, bool is_rach) (line too long) https://gerrit.osmocom.org/c/osmo-pcu/+/22309/3/src/gprs_bssgp_pcu.c File src/gprs_bssgp_pcu.c: https://gerrit.osmocom.org/c/osmo-pcu/+/22309/3/src/gprs_bssgp_pcu.c@213 PS3, Line 213: /* FIXME: look if MS is attached a specific BTS and then only page on that one? */ Sounds like looping over the attached BTS and their MS would solve it. Is this out of scope for this patch? (same with _ps below) https://gerrit.osmocom.org/c/osmo-pcu/+/22309/3/src/gprs_bssgp_pcu.c@823 PS3, Line 823: bts = llist_first_entry_or_null(&the_pcu->bts_list, struct gprs_rlcmac_bts, list); Why not add bts as parameter to gprs_bssgp_pcu_rx_ptp()? https://gerrit.osmocom.org/c/osmo-pcu/+/22309/3/src/gprs_rlcmac.h File src/gprs_rlcmac.h: https://gerrit.osmocom.org/c/osmo-pcu/+/22309/3/src/gprs_rlcmac.h@116 PS3, Line 116: (space) https://gerrit.osmocom.org/c/osmo-pcu/+/22309/3/src/osmo-bts-litecell15/lc15_l1_if.c File src/osmo-bts-litecell15/lc15_l1_if.c: https://gerrit.osmocom.org/c/osmo-pcu/+/22309/3/src/osmo-bts-litecell15/lc15_l1_if.c@160 PS3, Line 160: bts = llist_first_entry_or_null(&the_pcu->bts_list, struct gprs_rlcmac_bts, list); add bts parameter instead, or at least a FIXME comment like above? Same with all other "bts = llist_first_entry_or_null" lines below / in other files. Alternatively / additionally to FIXME everywhere, maybe add a note in README under "Current limitations". https://gerrit.osmocom.org/c/osmo-pcu/+/22309/4/src/osmobts_sock.c File src/osmobts_sock.c: https://gerrit.osmocom.org/c/osmo-pcu/+/22309/4/src/osmobts_sock.c@64 PS4, Line 64: bool retry = !llist_empty(&the_pcu->bts_list); I think the ! is wrong, shouldn't retry be true if the list is empty? https://gerrit.osmocom.org/c/osmo-pcu/+/22309/4/src/osmobts_sock.c@114 PS4, Line 114: llist_for_each_entry(bts, &the_pcu->bts_list, list) { My understanding is, that if one bts closes the socket to the pcu, the pcu will give up completely, drop all bts and exit. So this could use another FIXME. https://gerrit.osmocom.org/c/osmo-pcu/+/22309/4/src/pcu_l1_if.h File src/pcu_l1_if.h: https://gerrit.osmocom.org/c/osmo-pcu/+/22309/4/src/pcu_l1_if.h@159 PS4, Line 159: struct gprs_rlcmac_bts; its declared above already in the "ifdef __cplusplus" section. is that on purpose? https://gerrit.osmocom.org/c/osmo-pcu/+/22309/4/src/pcu_l1_if.cpp File src/pcu_l1_if.cpp: https://gerrit.osmocom.org/c/osmo-pcu/+/22309/4/src/pcu_l1_if.cpp@845 PS4, Line 845: bts = gprs_pcu_get_bts_by_nr(the_pcu, pcu_prim->bts_nr); this is called twice: gprs_pcu_get_bts_by_nr(the_pcu, pcu_prim->bts_nr); https://gerrit.osmocom.org/c/osmo-pcu/+/22309/4/src/pcu_vty.c File src/pcu_vty.c: https://gerrit.osmocom.org/c/osmo-pcu/+/22309/4/src/pcu_vty.c@1073 PS4, Line 1073: pcu_vty_show_tbf_all(vty, bts, flags); Print out the BTS number too? Otherwise it will just be "UL TBFs", "DL TBFs", "UL TBFs", ... https://gerrit.osmocom.org/c/osmo-pcu/+/22309/4/src/pcu_vty.c@1085 PS4, Line 1085: pcu_vty_show_ms_all(vty, bts); How about extending show_ms() to mention the BTS number? -- To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/22309 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Change-Id: I6b10913f46c19d438c4e250a436a7446694b725a Gerrit-Change-Number: 22309 Gerrit-PatchSet: 4 Gerrit-Owner: pespin <pes...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: osmith <osm...@sysmocom.de> Gerrit-Comment-Date: Tue, 19 Jan 2021 16:12:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
