Attention is currently required from: pespin.
fixeria has posted comments on this change by pespin. (
https://gerrit.osmocom.org/c/osmo-bts/+/41807?usp=email )
Change subject: {bs,ms}_power_control: Move params inside lchan_power_ctrl_state
......................................................................
Patch Set 3: Code-Review-1
(4 comments)
Patchset:
PS2:
> > So far I had a quick look and I don't really understand/see what do we
> > benefit from this refactori […]
>> From what I remember, I intentionally wanted to have the power control
>> parameters separated from the power control state. And using the pointer
>> allows to avoid memcpy()ing parameters which are identical for all lchans in
>> 99% cases.
> You are still holding a copy on each lchan. All I'm doing is getting rid of
> one extra pointer field.
I guess my original intention was to let the `*params` point to either per-TRX
or per-lchan power control parameters structure. However, this idea has a flow:
changing the global per-TRX parameters would immediately affect all logical
channels, which is not what we want. This is why each lchan always holds its
own copy of the power control parameters. And you're right, we always do
`memcpy()` anyway.
With that, I am no longer against merging this change.
However, there are some issues (see my other comments), thus CR-1.
File src/common/power_control.c:
https://gerrit.osmocom.org/c/osmo-bts/+/41807/comment/c0559abe_158c5091?usp=email
:
PS3, Line 151: memcpy(params, trx->ms_dpc_params, sizeof(*params));
What's the point of moving this `memcpy()` here? Why not keeping it where it
was (in `rsl_rx_chan_activ()`). This `memcpy()` is redundant when static power
control is used. This specific change also creates inconsistency with
`rsl_rx_ms_pwr_ctrl()`, where this `memcpy()` is preserved.
https://gerrit.osmocom.org/c/osmo-bts/+/41807/comment/886c80c9_a6842b2e?usp=email
:
PS3, Line 343: memcpy(params, trx->ms_dpc_params, sizeof(*params));
And you say pointers are the source of bugs...
The real source of bugs is the copy-paste function in text editors :D
```suggestion
memcpy(params, trx->bs_dpc_params, sizeof(*params));
```
But likewise here, I believe this `memcpy()` should be kept where it was.
File src/common/rsl.c:
https://gerrit.osmocom.org/c/osmo-bts/+/41807/comment/d30596b7_5173aae4?usp=email
:
PS3, Line 2082: lchan->ms_power_ctrl.dpc_enabled = true;
```suggestion
lchan->bs_power_ctrl.dpc_enabled = true;
```
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/41807?usp=email
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I0e2a6c2c33e0ac9a0bc1734d83eaeafc27f2f4df
Gerrit-Change-Number: 41807
Gerrit-PatchSet: 3
Gerrit-Owner: pespin <[email protected]>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <[email protected]>
Gerrit-Attention: pespin <[email protected]>
Gerrit-Comment-Date: Sun, 11 Jan 2026 23:51:30 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: pespin <[email protected]>
Comment-In-Reply-To: fixeria <[email protected]>
