Change in osmo-bsc[master]: WIP: Support set up of C/I parameters for osmo-bts MS Power Control Loop

[fixeria]

fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/25298 )

Change subject: WIP: Support set up of C/I parameters for osmo-bts MS Power Control Loop
......................................................................


Patch Set 2: Code-Review-1

(10 comments)

https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/include/osmocom/bsc/gsm_data.h 
File include/osmocom/bsc/gsm_data.h:

https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/include/osmocom/bsc/gsm_data.h@1346 
PS2, Line 1346: 	/* Measurement averaging parameters for C/I: */
I think using an array with pre-defined indexes would simplify the code:

  enum {
    GSM_PWR_CTRL_MP_CI_FR,
    GSM_PWR_CTRL_MP_CI_HR,
    _GSM_PWR_CTRL_MP_NUM,
  };

  ....

  struct gsm_power_ctrl_meas_params ci[_GSM_PWR_CTRL_MP_NUM];

So you can iterate over 'ci' using a loop:

  for (i = 0; i < ARRAY_SIZE(ci); i++)
    ...

rather than using huge macros in the generating/parsing code. And for the VTY, you would be able to use value_string instead of custom mapping functions.


https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_ipaccess_nanobts.c 
File src/osmo-bsc/bts_ipaccess_nanobts.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_ipaccess_nanobts.c@870 
PS2, Line 870: enc_osmo_meas_proc_params
Why do we add osmo-bts specific code in bts_ipaccess_nanobts.c? I believe it should be in bts_osmobts.c. This would require to make some API non-static in this file, so you can re-use it from bts_osmobts.c, which complicates the task a bit. But logically it should be there, not here.


https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_vty.c 
File src/osmo-bsc/bts_vty.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_vty.c@3063 
PS2, Line 3063: Channel Type
FR/EFR is not really a 'Channel type', but rather 'Codec type'.


https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_vty.c@3068 
PS2, Line 3068: Channel Type (E)GPRS
Can you please explain how is this related to GPRS? Currently all parameters in 'config-{ms,bs}-power-ctrl' nodes are for CS specific power control only. As far as I know, GPRS specific power control is completely different from CS domain. How is this going to be used and where? Are you planning to forward it to osmo-pcu somehow?


https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_vty.c@3137 
PS2, Line 3137: ,
Hmm, maybe worth adding a comment like: , /* nope */, because two comas look odd.


https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_vty.c@3190 
PS2, Line 3190: lower_cmp_p, lower_cmp_n
Ha! I also copy-pasted it here :P


https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_vty.c@3209 
PS2, Line 3209: Upper
Lower?


https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_vty.c@3227 
PS2, Line 3227: lower_cmp_p, lower_cmp_n
Copy-pasted, should be upper (also see comment above).


https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/src/osmo-bsc/bts_vty.c@3354 
PS2, Line 3354: -toI
-to-


https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/tests/power_ctrl.vty 
File tests/power_ctrl.vty:

https://gerrit.osmocom.org/c/osmo-bsc/+/25298/2/tests/power_ctrl.vty@78 
PS2, Line 78:    ci-thresh fr lower 13 upper 17
Why do we print C/I parameters for *BS* power control if they only apply to MS power control?



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/25298
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I7e76ec47b323d469f777624b74b08752d1f5584f
Gerrit-Change-Number: 25298
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-Comment-Date: Wed, 01 Sep 2021 08:47:04 +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/20210901/cc8fad0d/attachment.htm>