[PATCH] Uplink PAN handling for FANR feature

Sangamesh Sajjan Sangamesh.Sajjan at radisys.com
Thu Mar 17 06:24:56 UTC 2016


Hello,

Thank you for the comments,
Currently I'm working on merging of compression algorithm to master branch.
I will resubmit the patch by considering all your comments after completion of current activity.

Thanks,
Sangamesh

-----Original Message-----
From: Holger Freyther [mailto:holger at freyther.de] 
Sent: Wednesday, March 16, 2016 7:18 PM
To: Sangamesh Sajjan <Sangamesh.Sajjan at radisys.com>
Cc: osmocom-net-gprs at lists.osmocom.org
Subject: Re: [PATCH] Uplink PAN handling for FANR feature


> 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


More information about the osmocom-net-gprs mailing list