17 Mar
2016
17 Mar
'16
7:24 a.m.
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@freyther.de]
Sent: Wednesday, March 16, 2016 7:18 PM
To: Sangamesh Sajjan <Sangamesh.Sajjan(a)radisys.com>
Cc: osmocom-net-gprs(a)lists.osmocom.org
Subject: Re: [PATCH] Uplink PAN handling for FANR feature
On 10 Mar 2016, at 15:20, sangamesh sajjan
<sangamesh.sajjan(a)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