Change in osmo-bsc[master]: inter-BSC HO: add neighbor_ident API to manage neighbor-BSS-cells

[Harald Welte]

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 <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: Harald Welte <laforge at gnumonks.org>
Gerrit-Comment-Date: Wed, 20 Jun 2018 08:09:08 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: No
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20180620/19420f60/attachment.htm>