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.orgPatch Set 2: Code-Review-1 (14 comments) https://gerrit.osmocom.org/#/c/861/2//COMMIT_MSG Commit Message: Line 12: the specification explains that A bit within the uncompressed bitmap capital "The" , lower case "a bit"? Line 14: ignored. But current PCU implementation ignores EPDAN the sentence ends in mid-air? The problem is not explained. Line 16: fix in a subsequent commit please take care that your writing is readable, which includes ending sentences with a '.' https://gerrit.osmocom.org/#/c/861/2/src/rlc.h File src/rlc.h: Line 305: void set_v_s(int); Members m_v_s and m_v_a are not private/protected, so why add trivial setter functions? Or, alternatively, why not make them private? Is there a thought behind this? https://gerrit.osmocom.org/#/c/861/2/tests/tbf/TbfTest.cpp File tests/tbf/TbfTest.cpp: Line 2019: } a single blank line between function definitions Line 2022: * version 7.27.0 Release 7. which explains the Interpretation of the bitmap ". Which" or ", which" Line 2025: * expects the same bug. which shall be fixed in subsequent patch a lot of "which" ;) but nevermind Line 2036: uint8_t rbb[64/8]; is this 64 by chance related to RLC_EGPRS_MIN_WS, or RLC_EGPRS_WS? Line 2049: * used to simulate the EPDAN out of window scenario. During "used used". This EPDAN out of window scenario is simulated where exactly? Line 2051: * and window size of 480. same has been used below sentence punctuation, capitalisation Line 2072: dl_tbf->m_window.m_v_b.mark_unacked(1286); like in a recent patch, you're using dl_tbf->m_window and .m_v_b awfully often, which deserves a shortcut variable. Do 1286 before 1287, i.e. in sequence? Line 2091: OSMO_ASSERT(dl_tbf->m_window.m_v_b.is_unacked(1286)); Do 1286 before 1287, i.e. in sequence? Line 2107: tbf_free(dl_tbf); You should clearly mark the test expectation that will be changed with the fix. Line 2127: egprs_tbf_epdan_outof_rx_window(&the_bts, 4); If this function is called only once, I'd actually insert the full test here and not spread it across two functions. OTOH, if it makes sense to test with various mcs values, then please do so and invoke the function numerous times. -- To view, visit https://gerrit.osmocom.org/861 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If32b67f5c05707155281128b776a90a1e3d587b2 Gerrit-PatchSet: 2 Gerrit-Project: osmo-pcu Gerrit-Branch: master Gerrit-Owner: arvind.sirsikar <arvind.sirsikar at radisys.com> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de> Gerrit-HasComments: Yes