Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/9666 )
Change subject: inter-BSC HO: add neighbor_ident API to manage neighbor-BSS-cells ...................................................................... Patch Set 3: (5 comments) https://gerrit.osmocom.org/#/c/9666/3/include/osmocom/bsc/gsm_data.h File include/osmocom/bsc/gsm_data.h: https://gerrit.osmocom.org/#/c/9666/3/include/osmocom/bsc/gsm_data.h@755 PS3, Line 755: struct gsm_bts_ref { not a request to change, just a general reminder: We also have the "vector" of libosmovty, which is sort of a dynamically-sized array of pointers to other objects which one might have used rather than a linked list. https://gerrit.osmocom.org/#/c/9666/3/include/osmocom/bsc/neighbor_ident.h File include/osmocom/bsc/neighbor_ident.h: https://gerrit.osmocom.org/#/c/9666/3/include/osmocom/bsc/neighbor_ident.h@18 PS3, Line 18: BSIC_9BIT, huh, where's that 9 bit BSIC coming from? Do you have a spec reference for me? https://gerrit.osmocom.org/#/c/9666/3/src/osmo-bsc/gsm_data.c File src/osmo-bsc/gsm_data.c: https://gerrit.osmocom.org/#/c/9666/3/src/osmo-bsc/gsm_data.c@567 PS3, Line 567: bool gsm_bts_matches_cell_id(struct gsm_bts *bts, const struct gsm0808_cell_id *ci) not important, but I'd expect 'bts' could also be a const pointer, as we're just reading/comparing/matching? https://gerrit.osmocom.org/#/c/9666/3/src/osmo-bsc/neighbor_ident_vty.c File src/osmo-bsc/neighbor_ident_vty.c: https://gerrit.osmocom.org/#/c/9666/3/src/osmo-bsc/neighbor_ident_vty.c@560 PS3, Line 560: install_element(BTS_NODE, &cfg_neighbor_resolve_cmd); the BTS_NODE is a sub-node of the CONFIG NODE. Inside "configure terminal" we should only permit configuration commands, but not any query-type commands. I think the proper place for a query command is the VIEW+ENABLE node. So you can do something like "bts 0 neighbor resolve ...". If you still want to do it while in the CONFIG/BTS node, you can execute any VIEW/ENABLE command by prefixing it with "do". https://gerrit.osmocom.org/#/c/9666/3/tests/handover/neighbor_ident_test.c File tests/handover/neighbor_ident_test.c: https://gerrit.osmocom.org/#/c/9666/3/tests/handover/neighbor_ident_test.c@33 PS3, Line 33: arfcn it might be interesting to have test cases for ARFCNs in different bands. Particularly take into consideration PCS band neighbor cells, where the PCS/DCS is indicated as 0x8000 bit of the uint16_t arfcn. This gets easily missed in a lot of code. I don't understand your current changes sufficiently to state if it's needed exactly here. Just in general we need to consider neighbor cells in different bands, including PCS neigbors. -- To view, visit https://gerrit.osmocom.org/9666 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I0153d7069817fba9146ddc11214de2757d7d37bf Gerrit-Change-Number: 9666 Gerrit-PatchSet: 3 Gerrit-Owner: Neels Hofmeyr <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: Harald Welte <[email protected]> Gerrit-Comment-Date: Wed, 20 Jun 2018 08:09:08 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
