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