Attention is currently required from: arehbein, fixeria, laforge, neels.

Patch set 16:Code-Review -1

View Change

16 comments:

• File include/osmocom/bsc/gsm_data.h:

  • Patch Set #16, Line 720: enum gprs_rlc_par {

    I bet this is not needed anymore and can be dropped?

• File src/osmo-bsc/bsc_init.c:

  • Patch Set #16, Line 51: extern void bts_gprs_timer_groups_init(struct gsm_bts *bts);

    why don't you add this to a header file?

  • Patch Set #16, Line 240: bts_gprs_timer_groups_init(bts);

    why is this put here and not in gsm_bts_alloc_register() ?

• File src/osmo-bsc/bsc_vty.c:

  • Patch Set #16, Line 235:

    You could submit this a a preparation patch.

  • Patch Set #16, Line 336: int rc = CMD_WARNING, bts_nr = atoi(argv[0]);

    please use separate lines.

  • Patch Set #16, Line 346: if (bts->gprs.mode == BTS_GPRS_NONE) {

    why can't I configure gprs timers if gprs is not currently enabled? I don't see a problem with it, and several usability problems, like having to comment all timer lines if I decide to start the app with "gprs mode none".

  • Patch Set #16, Line 351: ++group;

    we tend to use group++ unless there's a good reason to use preinc.

  • Patch Set #16, Line 354: if (bts_write_group_timers(vty, "", bts_nr, group, T_arg, false) == CMD_WARNING)

    I don't really understand what are you doing here comparing against == CMD_WARNING and then doing rc = CMD_SUCCESS.

• File src/osmo-bsc/bts_init.c:

  • Patch Set #12, Line 60: 3101

    I actually find this problematic. […]

    Simply use 3101 for T3101, and -3101 (X3101) for N3101.

• File src/osmo-bsc/bts_init.c:

  • Patch Set #16, Line 88: for (gbtg = 0; gbtg < ARRAY_SIZE(bts_gprs_timer_template_groups); gbtg++)

    memcpy(&bts->timer_groups[0], bts_gprs_timer_template_groups[0], ARRAY_SIZE(bts_gprs_timer_template_groups));

• File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c:

  • Patch Set #12, Line 253: struct abis_nm_ipacc_att_rlc_cfg

    cosmetic: it's not necessary to specify the struct type here.

    Ack

• File src/osmo-bsc/bts_vty.c:

  • Patch Set #3, Line 1706: /* TODO (BSSGP timer patch): If argument is one of BSSGP Timers strings, then translate to

    My understanding so far has been, that we have three timer groups that relate to OS#5335 (e.g. […]

    So if this patch is for RLC; why are you touching BSSGP lines here?

• File src/osmo-bsc/bts_vty.c:

  • Patch Set #12, Line 56: limits.h

    why including thise header? I cannot see `_MIN` or `_MAX` in new code...

    Ack

• File tests/nanobts_omlattr/nanobts_omlattr_test.c:

  • Patch Set #16, Line 37: extern void bts_gprs_timer_groups_init(struct gsm_bts *bts);

    don't we have headers for this?

  • Patch Set #16, Line 38: extern void bts_grprs_tdef_groups_init(void);

    typo: grprs.

  • Patch Set #16, Line 158: bts_grprs_tdef_groups_init();

    typo: grprs.

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