Hi all,
I have continued with my refactorings and mostly done:
* Remove global state (e.g. the global lists for ta, sba, tbf, bts
* Add back-pointers to tbf, trx, pdch which will allow to kill methods
that take trx, ts as parameters.
* Started to use C++ classes and functions.
* Stop poking of internals from other classes areas (gprs_rlcmac_data
is still in front of me and the most hairy part).
I will probably need to spend another 40h on this code to finally
have structure in it. Once this is done I will add counters and
rate counters, improve the VTY inspection... and then I can search
for the actual defects we are experiencing.
While doing the refactorings I noticed that both pcu_l1_if.cpp and
sysmo_sock.cpp have different ways to reset the BTS/PCU state. Both
of them have different issues and are both incomplete. It is the
perfect example where structure would have saved timed and made
the code more reliable at the same time.
I am going to work on gprs_rlcmac_data.cpp and while my refactorings
so far where low risk.. the risk will increase when touching and
structuring the above, e.g. it is hard to judge if the difference in
the code-clones is on purpose or actually a bug. There will be only
one way to find it out though.
I am afraid that I have 52 commits compares to master and I am only
half way done. Review and changing directions will be quite difficult
at this point in time.
holger
--
- Holger Freyther <hfreyther(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Schivelbeiner Str. 5
* 10439 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
From: Holger Hans Peter Freyther <holger(a)moiji-mobile.com>
This is the begin of a long march of turning tbf into a C++ class
and properly hiding the secrets inside this implementation instead
of having it spread across various different files.
---
src/gprs_rlcmac.cpp | 1 +
src/gprs_rlcmac.h | 97 --------------------------------------------
src/gprs_rlcmac_meas.cpp | 1 +
src/gprs_rlcmac_sched.cpp | 1 +
src/pcu_l1_if.cpp | 1 +
src/tbf.h | 100 +++++++++++++++++++++++++++++++++++++++++++++-
6 files changed, 103 insertions(+), 98 deletions(-)
diff --git a/src/gprs_rlcmac.cpp b/src/gprs_rlcmac.cpp
index 6546f8a..3dab44f 100644
--- a/src/gprs_rlcmac.cpp
+++ b/src/gprs_rlcmac.cpp
@@ -22,6 +22,7 @@
#include <gprs_bssgp_pcu.h>
#include <pcu_l1_if.h>
#include <gprs_rlcmac.h>
+#include <tbf.h>
/* 3GPP TS 05.02 Annex B.1 */
diff --git a/src/gprs_rlcmac.h b/src/gprs_rlcmac.h
index 8de7417..6fdf600 100644
--- a/src/gprs_rlcmac.h
+++ b/src/gprs_rlcmac.h
@@ -155,103 +155,6 @@ enum gprs_rlcmac_tbf_direction {
#define GPRS_RLCMAC_FLAG_TO_DL_ASS 7
#define GPRS_RLCMAC_FLAG_TO_MASK 0xf0 /* timeout bits */
-struct gprs_rlcmac_tbf {
- struct llist_head list;
- enum gprs_rlcmac_tbf_state state;
- uint32_t state_flags;
- enum gprs_rlcmac_tbf_direction direction;
- uint8_t tfi;
- uint32_t tlli;
- uint8_t tlli_valid;
- uint8_t trx;
- uint16_t arfcn;
- uint8_t tsc;
- uint8_t first_ts; /* first TS used by TBF */
- uint8_t first_common_ts; /* first TS that the phone can send and
- reveive simultaniously */
- uint8_t control_ts; /* timeslot control messages and polling */
- uint8_t ms_class;
- struct gprs_rlcmac_pdch *pdch[8]; /* list of PDCHs allocated to TBF */
- uint16_t ta;
- uint8_t llc_frame[LLC_MAX_LEN]; /* current DL or UL frame */
- uint16_t llc_index; /* current write/read position of frame */
- uint16_t llc_length; /* len of current DL LLC_frame, 0 == no frame */
- struct llist_head llc_queue; /* queued LLC DL data */
-
- enum gprs_rlcmac_tbf_dl_ass_state dl_ass_state;
- enum gprs_rlcmac_tbf_ul_ass_state ul_ass_state;
- enum gprs_rlcmac_tbf_ul_ack_state ul_ack_state;
-
- enum gprs_rlcmac_tbf_poll_state poll_state;
- uint32_t poll_fn; /* frame number to poll */
-
- uint16_t ws; /* window size */
- uint16_t sns; /* sequence number space */
-
- /* Please note that all variables here will be reset when changing
- * from WAIT RELEASE back to FLOW state (re-use of TBF).
- * All states that need reset must be in this struct, so this is why
- * variables are in both (dl and ul) structs and not outside union.
- */
- union {
- struct {
- uint16_t bsn; /* block sequence number */
- uint16_t v_s; /* send state */
- uint16_t v_a; /* ack state */
- char v_b[RLC_MAX_SNS/2]; /* acknowledge state array */
- int32_t tx_counter; /* count all transmitted blocks */
- char imsi[16]; /* store IMSI for PCH retransmission */
- uint8_t wait_confirm; /* wait for CCCH IMM.ASS cnf */
- } dl;
- struct {
- uint16_t bsn; /* block sequence number */
- uint16_t v_r; /* receive state */
- uint16_t v_q; /* receive window state */
- char v_n[RLC_MAX_SNS/2]; /* receive state array */
- int32_t rx_counter; /* count all received blocks */
- uint8_t n3103; /* N3103 counter */
- uint8_t usf[8]; /* list USFs per PDCH (timeslot) */
- uint8_t contention_resolution_done; /* set after done */
- uint8_t final_ack_sent; /* set if we sent final ack */
- } ul;
- } dir;
- uint8_t rlc_block[RLC_MAX_SNS/2][RLC_MAX_LEN]; /* block history */
- uint8_t rlc_block_len[RLC_MAX_SNS/2]; /* block len of history */
-
- uint8_t n3105; /* N3105 counter */
-
- struct osmo_timer_list timer;
- unsigned int T; /* Txxxx number */
- unsigned int num_T_exp; /* number of consecutive T expirations */
-
- struct osmo_gsm_timer_list gsm_timer;
- unsigned int fT; /* fTxxxx number */
- unsigned int num_fT_exp; /* number of consecutive fT expirations */
-
- struct {
- char imsi[16];
-
- struct timeval dl_bw_tv; /* timestamp for dl bw calculation */
- uint32_t dl_bw_octets; /* number of octets since bw_tv */
-
- struct timeval rssi_tv; /* timestamp for rssi calculation */
- int32_t rssi_sum; /* sum of rssi values */
- int rssi_num; /* number of rssi values added since rssi_tv */
-
- struct timeval dl_loss_tv; /* timestamp for loss calculation */
- uint16_t dl_loss_lost; /* sum of lost packets */
- uint16_t dl_loss_received; /* sum of received packets */
-
- } meas;
-
- uint8_t cs; /* current coding scheme */
-
-#ifdef DEBUG_DIAGRAM
- int diag; /* number where TBF is presented in diagram */
- int diag_new; /* used to format output of new TBF */
-#endif
-};
-
extern struct llist_head gprs_rlcmac_ul_tbfs; /* list of uplink TBFs */
extern struct llist_head gprs_rlcmac_dl_tbfs; /* list of downlink TBFs */
extern struct llist_head gprs_rlcmac_sbas; /* list of single block allocs */
diff --git a/src/gprs_rlcmac_meas.cpp b/src/gprs_rlcmac_meas.cpp
index 75da835..3229795 100644
--- a/src/gprs_rlcmac_meas.cpp
+++ b/src/gprs_rlcmac_meas.cpp
@@ -20,6 +20,7 @@
#include <gprs_rlcmac.h>
#include <gprs_debug.h>
#include <pcu_l1_if.h>
+#include <tbf.h>
#include <string.h>
#include <errno.h>
diff --git a/src/gprs_rlcmac_sched.cpp b/src/gprs_rlcmac_sched.cpp
index 6290c5d..f3edaac 100644
--- a/src/gprs_rlcmac_sched.cpp
+++ b/src/gprs_rlcmac_sched.cpp
@@ -20,6 +20,7 @@
#include <gprs_bssgp_pcu.h>
#include <gprs_rlcmac.h>
#include <pcu_l1_if.h>
+#include <tbf.h>
uint32_t sched_poll(uint8_t trx, uint8_t ts, uint32_t fn, uint8_t block_nr,
struct gprs_rlcmac_tbf **poll_tbf,
diff --git a/src/pcu_l1_if.cpp b/src/pcu_l1_if.cpp
index 218dc23..47bf740 100644
--- a/src/pcu_l1_if.cpp
+++ b/src/pcu_l1_if.cpp
@@ -37,6 +37,7 @@ extern "C" {
#include <gprs_debug.h>
#include <gprs_bssgp_pcu.h>
#include <pcuif_proto.h>
+#include <tbf.h>
// FIXME: move this, when changed from c++ to c.
extern "C" {
diff --git a/src/tbf.h b/src/tbf.h
index 330eac1..a6dfced 100644
--- a/src/tbf.h
+++ b/src/tbf.h
@@ -18,9 +18,107 @@
#pragma once
+#include "gprs_rlcmac.h"
+
#include <stdint.h>
-struct gprs_rlcmac_bts;
+struct gprs_rlcmac_tbf {
+ struct llist_head list;
+ enum gprs_rlcmac_tbf_state state;
+ uint32_t state_flags;
+ enum gprs_rlcmac_tbf_direction direction;
+ uint8_t tfi;
+ uint32_t tlli;
+ uint8_t tlli_valid;
+ uint8_t trx;
+ uint16_t arfcn;
+ uint8_t tsc;
+ uint8_t first_ts; /* first TS used by TBF */
+ uint8_t first_common_ts; /* first TS that the phone can send and
+ reveive simultaniously */
+ uint8_t control_ts; /* timeslot control messages and polling */
+ uint8_t ms_class;
+ struct gprs_rlcmac_pdch *pdch[8]; /* list of PDCHs allocated to TBF */
+ uint16_t ta;
+ uint8_t llc_frame[LLC_MAX_LEN]; /* current DL or UL frame */
+ uint16_t llc_index; /* current write/read position of frame */
+ uint16_t llc_length; /* len of current DL LLC_frame, 0 == no frame */
+ struct llist_head llc_queue; /* queued LLC DL data */
+
+ enum gprs_rlcmac_tbf_dl_ass_state dl_ass_state;
+ enum gprs_rlcmac_tbf_ul_ass_state ul_ass_state;
+ enum gprs_rlcmac_tbf_ul_ack_state ul_ack_state;
+
+ enum gprs_rlcmac_tbf_poll_state poll_state;
+ uint32_t poll_fn; /* frame number to poll */
+
+ uint16_t ws; /* window size */
+ uint16_t sns; /* sequence number space */
+
+ /* Please note that all variables here will be reset when changing
+ * from WAIT RELEASE back to FLOW state (re-use of TBF).
+ * All states that need reset must be in this struct, so this is why
+ * variables are in both (dl and ul) structs and not outside union.
+ */
+ union {
+ struct {
+ uint16_t bsn; /* block sequence number */
+ uint16_t v_s; /* send state */
+ uint16_t v_a; /* ack state */
+ char v_b[RLC_MAX_SNS/2]; /* acknowledge state array */
+ int32_t tx_counter; /* count all transmitted blocks */
+ char imsi[16]; /* store IMSI for PCH retransmission */
+ uint8_t wait_confirm; /* wait for CCCH IMM.ASS cnf */
+ } dl;
+ struct {
+ uint16_t bsn; /* block sequence number */
+ uint16_t v_r; /* receive state */
+ uint16_t v_q; /* receive window state */
+ char v_n[RLC_MAX_SNS/2]; /* receive state array */
+ int32_t rx_counter; /* count all received blocks */
+ uint8_t n3103; /* N3103 counter */
+ uint8_t usf[8]; /* list USFs per PDCH (timeslot) */
+ uint8_t contention_resolution_done; /* set after done */
+ uint8_t final_ack_sent; /* set if we sent final ack */
+ } ul;
+ } dir;
+ uint8_t rlc_block[RLC_MAX_SNS/2][RLC_MAX_LEN]; /* block history */
+ uint8_t rlc_block_len[RLC_MAX_SNS/2]; /* block len of history */
+
+ uint8_t n3105; /* N3105 counter */
+
+ struct osmo_timer_list timer;
+ unsigned int T; /* Txxxx number */
+ unsigned int num_T_exp; /* number of consecutive T expirations */
+
+ struct osmo_gsm_timer_list gsm_timer;
+ unsigned int fT; /* fTxxxx number */
+ unsigned int num_fT_exp; /* number of consecutive fT expirations */
+
+ struct {
+ char imsi[16];
+
+ struct timeval dl_bw_tv; /* timestamp for dl bw calculation */
+ uint32_t dl_bw_octets; /* number of octets since bw_tv */
+
+ struct timeval rssi_tv; /* timestamp for rssi calculation */
+ int32_t rssi_sum; /* sum of rssi values */
+ int rssi_num; /* number of rssi values added since rssi_tv */
+
+ struct timeval dl_loss_tv; /* timestamp for loss calculation */
+ uint16_t dl_loss_lost; /* sum of lost packets */
+ uint16_t dl_loss_received; /* sum of received packets */
+
+ } meas;
+
+ uint8_t cs; /* current coding scheme */
+
+#ifdef DEBUG_DIAGRAM
+ int diag; /* number where TBF is presented in diagram */
+ int diag_new; /* used to format output of new TBF */
+#endif
+};
+
/* dispatch Unitdata.DL messages */
int tbf_handle(struct gprs_rlcmac_bts *bts,
--
1.8.4.rc3
Good Morning,
sched_poll is doing:
llist_for_each_entry(tbf, &gprs_rlcmac_ul_tbfs, list) {
/* this trx, this ts */
if (tbf->trx_no != trx || tbf->control_ts != ts)
continue;
/* polling for next uplink block */
if (tbf->poll_state == GPRS_RLCMAC_POLL_SCHED
&& tbf->poll_fn == poll_fn)
*poll_tbf = tbf;
if (tbf->ul_ack_state == GPRS_RLCMAC_UL_ACK_SEND_ACK)
*ul_ack_tbf = tbf;
if (tbf->dl_ass_state == GPRS_RLCMAC_DL_ASS_SEND_ASS)
*dl_ass_tbf = tbf;
if (tbf->ul_ass_state == GPRS_RLCMAC_UL_ASS_SEND_ASS)
*ul_ass_tbf = tbf;
}
llist_for_each_entry(tbf, &gprs_rlcmac_dl_tbfs, list) {
/* this trx, this ts */
if (tbf->trx_no != trx || tbf->control_ts != ts)
continue;
/* polling for next uplink block */
if (tbf->poll_state == GPRS_RLCMAC_POLL_SCHED
&& tbf->poll_fn == poll_fn)
*poll_tbf = tbf;
if (tbf->dl_ass_state == GPRS_RLCMAC_DL_ASS_SEND_ASS)
*dl_ass_tbf = tbf;
if (tbf->ul_ass_state == GPRS_RLCMAC_UL_ASS_SEND_ASS)
*ul_ass_tbf = tbf;
}
New tbf's will be added to the front (llist_add). Is it on purpose
that the dl_tbfs are preferred over ul_tbfs and that the last of
each tbf's will be found? What is the reasoning for this? What will
collect/catch poll_fn's that are in the past?
holger
--
- Holger Freyther <hfreyther(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Schivelbeiner Str. 5
* 10439 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
Hi,
in my refactorings (creating a TBF class and moving code that modifes
the internals into the TBF) I am trying to make gprs_rlcmac_ul_tbfs
and gprs_rlcmac_dl_tbfs private and I notice code like:
if (dir == GPRS_RLCMAC_UL_TBF) {
llist_for_each_entry(tbf, &gprs_rlcmac_ul_tbfs, list) {
if (tbf->state_is_not(GPRS_RLCMAC_RELEASING)
&& tbf->tlli == tlli && tbf->tlli_valid)
return tbf;
}
} else {
llist_for_each_entry(tbf, &gprs_rlcmac_dl_tbfs, list) {
if (tbf->state_is_not(GPRS_RLCMAC_RELEASING)
&& tbf->tlli == tlli)
return tbf;
}
}
llist_for_each_entry(tbf, &gprs_rlcmac_ul_tbfs, list) {
if (tbf->state_is_not(GPRS_RLCMAC_RELEASING)
&& tbf->poll_state == GPRS_RLCMAC_POLL_SCHED
&& tbf->poll_fn == fn && tbf->trx_no == trx
&& tbf->control_ts == ts)
return tbf;
}
llist_for_each_entry(tbf, &gprs_rlcmac_dl_tbfs, list) {
if (tbf->state_is_not(GPRS_RLCMAC_RELEASING)
&& tbf->poll_state == GPRS_RLCMAC_POLL_SCHED
&& tbf->poll_fn == fn && tbf->trx_no == trx
&& tbf->control_ts == ts)
return tbf;
}
In the first code. Why is tlli_valid only checked for the UL TBF
and not the downlink one?
holger
--
- Holger Freyther <hfreyther(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Schivelbeiner Str. 5
* 10439 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
Hi,
after the report about the missing usf assignment I started to
refactor the allocation algorithms (remove code duplication, apply
information hiding, directly access things).
Looking at the code it appears that algorithm A and B will always
select the first (in case of A) or the first and following (in case
of B) enabled PDCH.
I have the suspicion that in case of algorithm A only the first
PDCH will be used to assign to phones? And in case of algorithm B
it is likely that the assignments are not spread equally across the
available timeslots?
can someone confirm/deny?
holger
--
- Holger Freyther <hfreyther(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Schivelbeiner Str. 5
* 10439 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte
Good Evening,
I started a branch to add a basic structure to the TBF handling. The
reasons why I am doing this is for another lengthy email. I just came
across a piece of code and would like to create some awareness:
int write_immediate_assignment(bitvec * dest, uint8_t downlink, uint8_t ra,
uint32_t ref_fn, uint8_t ta, uint16_t arfcn, uint8_t ts, uint8_t tsc,
uint8_t tfi, uint8_t usf, uint32_t tlli, uint8_t polling,
uint32_t fn, uint8_t single_block, uint8_t alpha, uint8_t gamma,
int8_t ta_idx);
and invocations like:
if (sb)
plen = write_immediate_assignment(immediate_assignment, 0, ra,
Fn, qta >> 2, bts->trx[trx].arfcn, ts,
bts->trx[trx].pdch[ts].tsc, 0, 0, 0, 0, sb_fn, 1,
bts->alpha, bts->gamma, -1);
else
plen = write_immediate_assignment(immediate_assignment, 0, ra,
Fn, tbf->ta, tbf->arfcn, tbf->first_ts, tbf->tsc,
tbf->tfi, tbf->dir.ul.usf[tbf->first_ts], 0, 0, 0, 0,
bts->alpha, bts->gamma, -1);
With the above method and two invocations it is difficult to get all
arguments right and it is a strong argument against ever writing code
like this. With C++ one could create overloads that take the bts and
tbf as parameters. Or if one really only wants to have a single method
use a struct to pass the parameters. This has the benefit of naming
the parameters and will certainly require the same amount of stack
space as the current option.
So please, don't write methods like the above as they make maintaining
the code more difficult than it needs to be.
holger
--
- Holger Freyther <hfreyther(a)sysmocom.de> http://www.sysmocom.de/
=======================================================================
* sysmocom - systems for mobile communications GmbH
* Schivelbeiner Str. 5
* 10439 Berlin, Germany
* Sitz / Registered office: Berlin, HRB 134158 B
* Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte