Attention is currently required from: pespin.

Patch set 3:Code-Review -1

View Change

4 comments:

• Patchset:

  • Patch Set #2:

    > 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:

  • Patch Set #3, 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.

  • Patch Set #3, 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:

  • Patch Set #3, Line 2082: lchan->ms_power_ctrl.dpc_enabled = true;

    ```suggestion
    		lchan->bs_power_ctrl.dpc_enabled = true;
    ```

To view, visit change 41807. To unsubscribe, or for help writing mail filters, visit settings.