Attention is currently required from: neels, laforge, pespin.
7 comments:
Patchset:
Hi all,
Thanks for the reviews so far. Latest patchset uploaded which addresses some concerns.
I think our main disagreement is that of precision. The best of both worlds would be for me to store cumulative active time as seconds instead of milliseconds while still being able to show individual channel ages in milliseconds. This would require carrying remainders as state between activations which I think is ugly.
Perhaps it is indeed enough to only track seconds. In the grand scheme of using this value to extrapolate Erlangs per site, it is still quite precise.
Regards,
-Michael
File src/osmo-bsc/bts_trx_vty.c:
Patch Set #4, Line 575: vty_out(vty, " Activated %llu ms ago%s", duration_ms, VTY_NEWLINE);
Probably more interesting to print it in VTY as seconds? Doesn't look like we can glance millisecond […]
SDCCH channels might not ever reach a full second. This was my thinking in using millisecond precision in the VTY.
File src/osmo-bsc/bts_vty.c:
Patch Set #4, Line 3830: vty_out(vty, " Channel Activations : %"PRIu64" TCH", activations_tch);
I'd rather put it: […]
You don't want to show total activations? Same comment as above about ms precision being needed to display SDCCH info in a meaningful way. I could add logic to simply show "<1s" for them. Sounds clean as well.
File src/osmo-bsc/gsm_data.c:
Patch Set #4, Line 349: long long
wondering if a signed data type makes sense here, but this is not critical.
true...everything is now unsigned long long
File src/osmo-bsc/lchan_fsm.c:
Patch Set #4, Line 1812: long long duration_ms = gsm_lchan_active_duration_ms(lchan);
Shouldn't this be an unsigned long long everywhere? Maybe simply use time_t?
true...everything is now unsigned long long
Patch Set #4, Line 1814: if (lchan->type == GSM_LCHAN_TCH_F || lchan->type == GSM_LCHAN_TCH_H) {
I'd go for a switch statement here, but not critical.
done
Patch Set #4, Line 1816: rate_ctr_add(rate_ctr_group_get_ctr(bts_ctrs, BTS_CTR_LCHAN_TCH_TOTAL_ACTIVE_MILLISECONDS), duration_ms);
my gut feeling also goes more towards a stat_item, but I didn't spend a lot of time contemplating it […]
Agreed. Converted to stat_item.
To view, visit change 27081. To unsubscribe, or for help writing mail filters, visit settings.