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=... : 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=... : 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=... : PS3, Line 2082: lchan->ms_power_ctrl.dpc_enabled = true; ```suggestion lchan->bs_power_ctrl.dpc_enabled = true; ```