This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Neels Hofmeyr gerrit-no-reply at lists.osmocom.org
Patch Set 5: Code-Review-1
(9 comments)
https://gerrit.osmocom.org/#/c/819/5//COMMIT_MSG
Commit Message:
Line 11: combined capacity of DL and UL for TS allocation,
this would be a good place to end the sentence to make it easier to read and understand
Line 12: with this there is a difference in throughput between
"with this", what "this" exactly?
https://gerrit.osmocom.org/#/c/819/5/src/bts.h
File src/bts.h:
Line 49: NO_MAXIMISE
In enum values we typically use a common prefix.
e.g. MAXIMISE_DIR_DL_ONLY, MAXIMISE_DIR_NONE
Line 198: enum maximise_direction maximise_dir;
did you check whether this struct is used only in osmo-pcu and that having an enum is fine here? The point is that enums don't have an explicitly defined size. Since all other members of this struct are very specific on the int size, I suspect an enum may break some ABI.
https://gerrit.osmocom.org/#/c/819/5/src/gprs_rlcmac_ts_alloc.cpp
File src/gprs_rlcmac_ts_alloc.cpp:
Line 778: if (bts->maximise_dir == DL_ONLY) {
Before there was UL, DL and a third 'else', and that should have been a switch() statement. If maximise_dir has only two states now, it should be a bool and not an enum; will UL be added again at a later stage? In that case I would like a switch() statement here.
https://gerrit.osmocom.org/#/c/819/5/src/pcu_vty.c
File src/pcu_vty.c:
Line 500: #define MAXIMISE_STR "Maximise TS allocation based on configuration\n"
I still don't like "TS allocation based on configuration".
Please write something that is intelligible.
Line 502: DEFUN(cfg_pcu_ts_alloc_maximise_type,
drop the '_type'
Line 504: "maximise-direction dl",
Interesting, now you've dropped the UL completely?
Is UL likely to be added some time in the future?
Otherwise we can just have a boolean flag called 'maximise_dl' everywhere, no need for an enum at all.
Line 517: NO_STR "Maximise direction configuration\n")
Once MAXIMISE_STR has a sensible string, you should use it here for 'maximise-direction', like above.
--
To view, visit https://gerrit.osmocom.org/819
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b4a99194ccae68bb3417bce538d16e944d5ec71
Gerrit-PatchSet: 5
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: arvind.sirsikar <arvind.sirsikar at radisys.com>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Holger Freyther <holger at freyther.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: arvind.sirsikar <arvind.sirsikar at radisys.com>
Gerrit-HasComments: Yes