Change in osmo-pcu[master]: Allow multiple bts objects in PCU

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

osmith gerrit-no-reply at lists.osmocom.org
Tue Jan 19 16:12:52 UTC 2021


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 <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: osmith <osmith at sysmocom.de>
Gerrit-Comment-Date: Tue, 19 Jan 2021 16:12:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210119/b6183459/attachment.htm>


More information about the gerrit-log mailing list