Attention is currently required from: pespin, fixeria.
20 comments:
File include/osmocom/bsc/bts.h:
Patch Set #5, Line 839: struct gsm_lchan *gl
> it's the initials of `gsm_lchan`. […]
Done
Patch Set #5, Line 842: if (gl) {
I would actually prefer it as is, at least I find there is a gain in readability that makes up for a […]
Done
Patch Set #5, Line 855: talloc_strdup_append
Ah yeah thanks! I don't know what I was doing there
Done
File src/osmo-bsc/bts.c:
Patch Set #1, Line 535: unsigned long ny1;
It's totally fine to include logging in bts. […]
Done
Patch Set #1, Line 654: unsigned long gsm_bts_check_ny1(struct gsm_bts *bts, unsigned long *ny1_out)
Since this is only needed in this file, set the function to static (move it above where it is used).
Done (considering your approval of having the check called during runtime)
File src/osmo-bsc/bts.c:
Patch Set #9, Line 535: if (!gsm_bts_check_ny1(bts, LOGL_ERROR, "Value of Ny1 too low.")) {
This string can probably go inside the gsm_bts_check_ny1 function.
Done
Patch Set #9, Line 1725: /* Return 'true' iff ny1 satisfies network requirements, otherwise return a suggestion for ny1.
"Ny1" (see initial cap). […]
Done
Patch Set #9, Line 1727: bool gsm_bts_check_ny1(struct gsm_bts *bts, unsigned int log_level, char const *msg)
const always first. Also, "msg" name is usually used for struct msgb, better call it "str".
Done
Patch Set #9, Line 1729: struct gsm_lchan *gl = gsm_bts_get_cbch(bts);
we usually use "lchan" as variable name for a struct gsm_lchan. […]
According to the specs, T3124 is set depending on the channel type (I assume 'lchan' stands for 'link channel' or something like that (?)). So I'm using that information to create the lower bound for Ny1.
Patch Set #9, Line 1738: unsigned long T3105 = osmo_tdef_get_entry(bts->network->T_defs, 3105)->val;
You can use osmo_tdef_get() here. […]
Done
Patch Set #9, Line 1740: unsigned long ny1 = osmo_tdef_get_entry(bts->network->T_defs, -3105)->val;
Same, osmo_tdef_get()
Done
Patch Set #9, Line 1742: bool ny1_satisfies_requirements = T3105 * ny1 > T3124 + GSM_NY1_REQ_DELTA;
We usually declare variables at the start of the function/block.
Is this a hard rule of ours or a preference? If there is any leeway, I'd rather leave it as is.
Patch Set #9, Line 1744: unsigned long ny1_recommended = (T3124 + GSM_NY1_REQ_DELTA)/T3105 + 1;
Why this +1? can you explain?
Without the '+ 1', due to integer arithmetic in C, we would be setting ny1 to the largest integer smaller or equal to the rational number (T3124 + delta)/T3105.
With it we are setting it to the smallest integer larger than that number which from my understanding is what we would want (assuming 'delta' is 'good').
Patch Set #9, Line 1748: return ny1_satisfies_requirements;
You can simplify all this code with early return: […]
I considered this. I put '!(T3105 * ny1 > T3124 + GSM_NY1_REQ_DELTA)' as the condition so as to preserve the original inequality mentioned everywhere else.
File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c:
Patch Set #1, Line 96: unsigned long ny1;
ACK, feel free to leave it there then.
Done
File src/osmo-bsc/bts_ipaccess_nanobts_omlattr.c:
Patch Set #9, Line 96: (void) gsm_bts_check_ny1(bts, LOGL_NOTICE, "Value of Ny1 should be higher.");
In one function is "should be lower" and here "should be higher"? This probably makes no sense.
It was 'Value of Ny1 too low' and here it is 'should be higher'. But I got rid of the message string itself as a parameter, so it doesn't matter anymore (only the log level is a parameter now).
File tests/nanobts_omlattr/nanobts_omlattr_test.c:
Patch Set #9, Line 127: static unsigned long const T3105_TESTVAL = 13;
> Move const after static - use 'static const unsigned long' […]
Done
Patch Set #9, Line 130: static unsigned long const NY1_TESTVAL = (GSM_T3124_MAX + GSM_NY1_REQ_DELTA)/T3105_TESTVAL + 1;
> Move const after static - use 'static const unsigned long' […]
Done
Patch Set #9, Line 133: { .T = -3105, .default_val = GSM_NY1_DEFAULT, .val = NY1_TESTVAL, .unit = OSMO_TDEF_CUSTOM,
Ah I see now that you actually this this T3015 here because it's used when test calls the check_ny1 […]
Done
Patch Set #9, Line 161: trx = talloc_zero(ctx, struct gsm_bts_trx);
I think this test is missing a calling to osmo_tdefs_reset(net->T_defs);
I tried it out but that wasn't actually missing (the test is setting the necessary values)
To view, visit change 30602. To unsubscribe, or for help writing mail filters, visit settings.