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/osmocom-net-gprs@lists.osmocom.org/.
Holger Freyther holger at freyther.de
> On 10 Mar 2016, at 15:20, sangamesh sajjan <sangamesh.sajjan at radisys.com> wrote:
>
Hi,
> Changes includes uplink PAN handling for Downlink tbf and
> bitmap handling to update Tentative ack or nacked
we have been waiting for the second version of it. There are too many coding style violations so it doesn't make sense.
> ---
> src/decoding.cpp | 40 +++++++++++++++++++++
> src/tbf.cpp | 2 ++
> src/tbf.h | 1 +
> src/tbf_ul.cpp | 92 +++++++++++++++++++++++++++++++++++++++++++++--
> tests/edge/EdgeTest.cpp | 68 +++++++++++++++++++++++++++++++++++
> 5 files changed, 201 insertions(+), 2 deletions(-)
>
> diff --git a/src/decoding.cpp b/src/decoding.cpp
> index 649f68d..d3d0d89 100644
> --- a/src/decoding.cpp
> +++ b/src/decoding.cpp
> @@ -423,6 +423,19 @@ int Decoding::decode_ul_data_fanr_type1(struct gprs_rlc_ul_header_generic *rlc,
> rlc->block_info[1].data_len = data_len;
> rlc->data_offs_bytes[0] = 7;
> rlc->data_offs_bytes[1] = 7 + data_len + 1;
> + if ( rlc->pani == 1) {
> + const struct egprs_ssn_based_pan_header *egprs1_fanr;
> + egprs1_fanr = static_cast<struct egprs_ssn_based_pan_header*>
> + ((void *)&data[rlc->block_info[1].data_len + rlc->data_offs_bytes[1]]);
why do you need to combine old C style cast with C++ one?
> +uint16_t log2ws[9] = {6,7,8,8,9,9,9,9,10};
why is it define in tbf.cpp and not used there? ws == window size?
> +
> gprs_rlcmac_tbf::Meas::Meas() :
> rssi_sum(0),
> rssi_num(0)
> diff --git a/src/tbf.h b/src/tbf.h
> index a6878d7..ba0dce1 100644
> --- a/src/tbf.h
> +++ b/src/tbf.h
> @@ -504,6 +504,7 @@ struct gprs_rlcmac_ul_tbf : public gprs_rlcmac_tbf {
> uint8_t m_usf[8]; /* list USFs per PDCH (timeslot) */
> uint8_t m_contention_resolution_done; /* set after done */
> uint8_t m_final_ack_sent; /* set if we sent final ack */
> + int uplink_pan_handling(const gprs_rlc_ul_header_generic *rlc);
What should it do? What should this method do? I can not say based on the name.
> +int gprs_rlcmac_ul_tbf::uplink_pan_handling(
> + const gprs_rlc_ul_header_generic *rlc)
> +{
>
Code is read a lot more frequently than it is written. For being able to unit test and also for allowing people to read, methods should be small. I think you will find a nice way to split the different concerns and make it more straight forward.
> + return 1;
> }
>
> void gprs_rlcmac_ul_tbf::maybe_schedule_uplink_acknack(
> diff --git a/tests/edge/EdgeTest.cpp b/tests/edge/EdgeTest.cpp
> index 7177832..924728f 100644
> --- a/tests/edge/EdgeTest.cpp
> +++ b/tests/edge/EdgeTest.cpp
> @@ -1641,7 +1641,74 @@ static gprs_rlcmac_ul_tbf *establish_ul_tbf_two_phase_cv0(BTS *the_bts,
> return ul_tbf;
> }
>
> +void fanr_handling(gprs_rlcmac_ul_tbf *ul_tbf, BTS *the_bts, uint32_t *fn, uint8_t ts_no,
> + GprsMs *ms , uint32_t tlli, uint16_t qta)
> +{
>
> + ms = the_bts->ms_by_tlli(tlli);
> + OSMO_ASSERT(ms != NULL);
> + OSMO_ASSERT(ms->ta() == qta/4);
> + OSMO_ASSERT(ms->ul_tbf() == ul_tbf);
these are the only asserts in the testcase and they do not concern to anything related to FANR. Could you please elaborate about it?
holger