Change in osmo-bsc[master]: Implement MS Uplink Power Control Loop.

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/.

keith gerrit-no-reply at lists.osmocom.org
Sat Oct 2 01:35:39 UTC 2021


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

Change subject: Implement MS Uplink Power Control Loop.
......................................................................


Patch Set 8:

(10 comments)

https://gerrit.osmocom.org/c/osmo-bsc/+/25654/7//COMMIT_MSG 
Commit Message:

https://gerrit.osmocom.org/c/osmo-bsc/+/25654/7//COMMIT_MSG@10 
PS7, Line 10: * Imports power_control.c from osmo-bts project.
> Please add a commit reference here.
OK, Should it be the commit hash of the commit that originally adds power_control.c to osmo-bts? Or the last commit to modify it? (i.e. the version I took?)
Or do you mean a URL reference to the file itself? Thnx


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

https://gerrit.osmocom.org/c/osmo-bsc/+/25654/7/include/osmocom/bsc/gsm_data.h@825 
PS7, Line 825: 	struct gsm_power_ctrl_meas_proc_state rxlev_meas_proc;
> Ack
Are you saying you would you like to see ms_power moved in here to '.current' in a separate patchset? (maybe previous to this one?) 
I've removed unused fields from the state struct, not sure if you would prefer to leave them there? Or put them back them in a subsequent commit that actually makes use of them?


https://gerrit.osmocom.org/c/osmo-bsc/+/25654/7/src/osmo-bsc/bsc_vty.c 
File src/osmo-bsc/bsc_vty.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/25654/7/src/osmo-bsc/bsc_vty.c@1359 
PS7, Line 1359: == GSM_PWR_CTRL_MODE_DYN_BSC
> I would rather do != GSM_PWR_CTRL_MODE_DYN_BTS here. […]
I'm happy to drop it. It's just an advice to the operator that the command won't do anything as it's not in DYN_BTS mode. Let me know what you prefer.


https://gerrit.osmocom.org/c/osmo-bsc/+/25654/7/src/osmo-bsc/bts_vty.c 
File src/osmo-bsc/bts_vty.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/25654/7/src/osmo-bsc/bts_vty.c@3150 
PS7, Line 3150: 	if (params->mode == GSM_PWR_CTRL_MODE_DYN_BSC) {
> These thresholds also does not matter in 'static' mode. Not sure if we really need this check here.. […]
I don't know what to do with "not sure" comments. 😊
We don't really "NEED" it, no, do we really need the following lines I based it on?

I don't mind. I'll follow your lead, if you want me to take it out, please be more explicit.
thanks


https://gerrit.osmocom.org/c/osmo-bsc/+/25654/7/src/osmo-bsc/power_control.c 
File src/osmo-bsc/power_control.c:

https://gerrit.osmocom.org/c/osmo-bsc/+/25654/7/src/osmo-bsc/power_control.c@161 
PS7, Line 161: struct gsm_meas_rep *mr
> Please make this pointer 'const'.
Done


https://gerrit.osmocom.org/c/osmo-bsc/+/25654/7/src/osmo-bsc/power_control.c@170 
PS7, Line 170: 	// int16_t ul_lqual_cb_avg;
> Ack
Missed it..


https://gerrit.osmocom.org/c/osmo-bsc/+/25654/7/src/osmo-bsc/power_control.c@171 
PS7, Line 171: 	uint8_t ms_power_lvl = ms_pwr_ctl_lvl(band, mr->ms_l1.pwr);
> I'd say this is wrong. mr->ms_l1. […]
meas_rep.h:

	struct {
		int8_t pwr;	/* MS power in dBm */
		uint8_t ta;	/* MS timing advance */
	} ms_l1;


abis_rsl.c:1319:
mr->ms_l1.pwr = ms_pwr_dbm(sign_link->trx->bts->band, val[0] >> 3);

Besides, I don't think it would have perfomed correctly if that were wrong 😋


https://gerrit.osmocom.org/c/osmo-bsc/+/25654/7/src/osmo-bsc/power_control.c@177 
PS7, Line 177: 	/* Not doing the power loop here if we are not handling it */
> I would move this check to the calling function, but not critical.
I did question this. My doubt is whether it less optimal or less clear to access
mr->lchan->ts->trx->bts>ms_power_ctrl->mode in the calling function, or do do it here.
I don't really understand what happens with CPU/Memory (if anything) when we call this function with those declarations at the top, only to exit.


https://gerrit.osmocom.org/c/osmo-bsc/+/25654/7/src/osmo-bsc/power_control.c@183 
PS7, Line 183: 	if (bts->dtxu == GSM48_DTX_SHALL_BE_USED)
> Indeed. […]
Yep, thanks I hadn't actually investigated the full report IE, I see it now.


https://gerrit.osmocom.org/c/osmo-bsc/+/25654/7/src/osmo-bsc/power_control.c@258 
PS7, Line 258: 	lchan->ms_power = new_power_lvl;
> Here for sure lchan_update_ms_power_ctrl_level() must be called instead.
Done



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

Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: Ibc307e758697eb5ca3fb86622f35709d6077db9e
Gerrit-Change-Number: 25654
Gerrit-PatchSet: 8
Gerrit-Owner: keith <keith at rhizomatica.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy at sysmocom.de>
Gerrit-CC: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Sat, 02 Oct 2021 01:35:39 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20211002/33046f2d/attachment.htm>


More information about the gerrit-log mailing list