pespin 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: (9 comments) 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. […] Yes it's out of the scope of this patch. This patch is not aiming at properly supporting multibts at runtime, simply adapting the code architecture for allowing it in the future. In the only event contemplated here (1 BTS), looping does 1 iteration so no change in behavior. This patch is already too big to try to do more stuff :) 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()? because caller of this function, bvc_timeout(), is called in lots of places with NULL param where specific BTS object is not available. In any case, I put this here because this function is already super fucked up and should be completely rewritten independently of this patch (see FIXME below). I also remember myself seeing the calculations were wrong for other reasons in the past. So, to keep old behavior, the easiest here is to take the first BTS in list. 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? […] It's not a current limitation. If we are using the direct_phy backend, it means we are attached to the BTS directly, so there's only 1 BTS announced in PCUIF, and hence taking the first one is fine (because it's the only one to be ever available). Same applies for file sysmo_l1_if.c, oc2g_l1_if.c. 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? Indeed, thanks! In general is not a big issue because pcu_tx_txt_ind() is sent during successful port open in pcu_l1if_open(), but indeed you are right. 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, […] There's only 1 PCUIF unix socket, which can be connected to a BTS or a BSC. In the later, BSC sends several info_ind, one for each BTS. But in this patch, when the unix socket is closed, we want to drop all BTS. 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. […] Yes, because I need it on top of the pcu_l1if_tx_* functions, but if a C file is including this header, then I also need to put the struct gprs_rlcmac_bts here since the block above will not be seen by it. I agree some headers may look a bit messy but I expect them to become cleaner as more and more helper classes are moved to C over time. 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: […] Ack 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", ... Which is fine since I don't want to change current behavior in this code. This is printing TBFs, not BTS and its associated TBFs. If at all, one should decide whether it makes sense to print the BTS number inside each TBF block in pcu_vty_show_tbf_all(). But in any case, I'm not willing to change that here. 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? Same reason as above,this lists MS, I don't plan to change behavior here. -- 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:59:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: osmith <osm...@sysmocom.de> Gerrit-MessageType: comment
