[PATCH] osmo-pcu[master]: Cleanup TBF state handling

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/.

Max gerrit-no-reply at lists.osmocom.org
Thu Jun 8 14:05:13 UTC 2017


Hello Jenkins Builder,

I'd like you to reexamine a change.  Please visit

    https://gerrit.osmocom.org/2858

to look at the new patch set (#2).

Cleanup TBF state handling

* use OSMO_BIT_* macros for manipulating TBF state flags to improve code
readability and make state change tracking easier
* move copy-pasted code into inline function

Change-Id: Icca5ce3273799e0165adcc062db8a4b71dd9fb04
Related: OS#1524
---
M src/bts.cpp
M src/tbf.cpp
M src/tbf.h
M src/tbf_dl.cpp
M src/tbf_ul.cpp
5 files changed, 53 insertions(+), 56 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/58/2858/2

diff --git a/src/bts.cpp b/src/bts.cpp
index 1d27284..3bc02a2 100644
--- a/src/bts.cpp
+++ b/src/bts.cpp
@@ -651,7 +651,7 @@
 		} else {
 			tbf->set_ta(ta);
 			tbf->set_state(GPRS_RLCMAC_FLOW);
-			tbf->state_flags |= (1 << GPRS_RLCMAC_FLAG_CCCH);
+			OSMO_BIT_SET(tbf->state_flags, GPRS_RLCMAC_FLAG_CCCH);
 			tbf_timer_start(tbf, 3169, m_bts.t3169, 0);
 			LOGP(DRLCMAC, LOGL_DEBUG, "%s [UPLINK] START\n",
 					tbf_name(tbf));
@@ -788,8 +788,8 @@
 
 		/* change state */
 		dl_tbf->set_state(GPRS_RLCMAC_ASSIGN);
-		if (!(dl_tbf->state_flags & (1 << GPRS_RLCMAC_FLAG_CCCH)))
-			dl_tbf->state_flags |= (1 << GPRS_RLCMAC_FLAG_PACCH);
+		if (!OSMO_BIT_GET(dl_tbf->state_flags, GPRS_RLCMAC_FLAG_CCCH))
+			OSMO_BIT_SET(dl_tbf->state_flags, GPRS_RLCMAC_FLAG_PACCH);
 		/* start timer */
 		tbf_timer_start(dl_tbf, 0, Tassign_pacch);
 	} else {
@@ -797,7 +797,7 @@
 		dl_tbf->was_releasing = dl_tbf->state_is(GPRS_RLCMAC_WAIT_RELEASE);
 		/* change state */
 		dl_tbf->set_state(GPRS_RLCMAC_ASSIGN);
-		dl_tbf->state_flags |= (1 << GPRS_RLCMAC_FLAG_CCCH);
+		OSMO_BIT_SET(dl_tbf->state_flags, GPRS_RLCMAC_FLAG_CCCH);
 		/* send immediate assignment */
 		dl_tbf->bts->snd_dl_ass(dl_tbf, 0, dl_tbf->imsi());
 		dl_tbf->m_wait_confirm = 1;
@@ -1007,10 +1007,8 @@
 	if (tbf->ul_ack_state == GPRS_RLCMAC_UL_ACK_WAIT_ACK) {
 		LOGP(DRLCMAC, LOGL_DEBUG, "TBF: [UPLINK] END %s\n", tbf_name(tbf));
 		tbf->ul_ack_state = GPRS_RLCMAC_UL_ACK_NONE;
-		if ((tbf->state_flags &
-			(1 << GPRS_RLCMAC_FLAG_TO_UL_ACK))) {
-			tbf->state_flags &=
-				~(1 << GPRS_RLCMAC_FLAG_TO_UL_ACK);
+		if (OSMO_BIT_GET(tbf->state_flags, GPRS_RLCMAC_FLAG_TO_UL_ACK)) {
+			OSMO_BIT_CLEAR(tbf->state_flags, GPRS_RLCMAC_FLAG_TO_UL_ACK);
 				LOGP(DRLCMAC, LOGL_NOTICE, "Recovered uplink "
 					"ack for UL %s\n", tbf_name(tbf));
 		}
@@ -1033,22 +1031,20 @@
 				tbf->direction == new_tbf->direction)
 			tbf_free(tbf);
 
-		if ((new_tbf->state_flags & (1 << GPRS_RLCMAC_FLAG_CCCH))) {
+		if (OSMO_BIT_GET(new_tbf->state_flags, GPRS_RLCMAC_FLAG_CCCH)) {
 			/* We now know that the PACCH really existed */
 			LOGP(DRLCMAC, LOGL_INFO,
 				"The TBF has been confirmed on the PACCH, "
 				"changed type from CCCH to PACCH for %s\n",
 				tbf_name(new_tbf));
-			new_tbf->state_flags &= ~(1 << GPRS_RLCMAC_FLAG_CCCH);
-			new_tbf->state_flags |= (1 << GPRS_RLCMAC_FLAG_PACCH);
+			OSMO_BIT_CLEAR(new_tbf->state_flags, GPRS_RLCMAC_FLAG_CCCH);
+			OSMO_BIT_SET(new_tbf->state_flags, GPRS_RLCMAC_FLAG_PACCH);
 		}
 		new_tbf->set_state(GPRS_RLCMAC_FLOW);
 		/* stop pending assignment timer */
 		new_tbf->stop_timer();
-		if ((new_tbf->state_flags &
-			(1 << GPRS_RLCMAC_FLAG_TO_DL_ASS))) {
-			new_tbf->state_flags &=
-				~(1 << GPRS_RLCMAC_FLAG_TO_DL_ASS);
+		if (OSMO_BIT_GET(new_tbf->state_flags, GPRS_RLCMAC_FLAG_TO_DL_ASS)) {
+			OSMO_BIT_CLEAR(new_tbf->state_flags, GPRS_RLCMAC_FLAG_TO_DL_ASS);
 			LOGP(DRLCMAC, LOGL_NOTICE, "Recovered downlink "
 				"assignment for %s\n", tbf_name(new_tbf));
 		}
@@ -1072,10 +1068,8 @@
 			tbf_free(tbf);
 
 		new_tbf->set_state(GPRS_RLCMAC_FLOW);
-		if ((new_tbf->state_flags &
-			(1 << GPRS_RLCMAC_FLAG_TO_UL_ASS))) {
-			new_tbf->state_flags &=
-				~(1 << GPRS_RLCMAC_FLAG_TO_UL_ASS);
+		if (OSMO_BIT_GET(new_tbf->state_flags, GPRS_RLCMAC_FLAG_TO_UL_ASS)) {
+			OSMO_BIT_CLEAR(new_tbf->state_flags, GPRS_RLCMAC_FLAG_TO_UL_ASS);
 			LOGP(DRLCMAC, LOGL_NOTICE, "Recovered uplink "
 				"assignment for UL %s\n", tbf_name(new_tbf));
 		}
@@ -1150,6 +1144,15 @@
 	}
 }
 
+static inline void try_dl_ack_recovery(struct gprs_rlcmac_dl_tbf *tbf)
+{
+	OSMO_BIT_SET(tbf->state_flags, GPRS_RLCMAC_FLAG_DL_ACK);
+	if (OSMO_BIT_GET(tbf->state_flags, GPRS_RLCMAC_FLAG_TO_DL_ACK)) {
+		OSMO_BIT_CLEAR(tbf->state_flags, GPRS_RLCMAC_FLAG_TO_DL_ACK);
+		LOGP(DRLCMAC, LOGL_NOTICE, "Recovered downlink ack for %s\n", tbf_name(tbf));
+	}
+}
+
 void gprs_rlcmac_pdch::rcv_control_dl_ack_nack(Packet_Downlink_Ack_Nack_t *ack_nack, uint32_t fn)
 {
 	int8_t tfi = 0; /* must be signed */
@@ -1175,12 +1178,9 @@
 			"wrong TFI=%d, ignoring!\n", tfi);
 		return;
 	}
-	tbf->state_flags |= (1 << GPRS_RLCMAC_FLAG_DL_ACK);
-	if ((tbf->state_flags & (1 << GPRS_RLCMAC_FLAG_TO_DL_ACK))) {
-		tbf->state_flags &= ~(1 << GPRS_RLCMAC_FLAG_TO_DL_ACK);
-		LOGP(DRLCMAC, LOGL_NOTICE, "Recovered downlink ack "
-			"for %s\n", tbf_name(tbf));
-	}
+
+	try_dl_ack_recovery(tbf);
+
 	/* reset N3105 */
 	tbf->n3105 = 0;
 	tbf->stop_t3191();
@@ -1263,12 +1263,9 @@
 			"wrong TFI=%d, ignoring!\n", tfi);
 		return;
 	}
-	tbf->state_flags |= (1 << GPRS_RLCMAC_FLAG_DL_ACK);
-	if ((tbf->state_flags & (1 << GPRS_RLCMAC_FLAG_TO_DL_ACK))) {
-		tbf->state_flags &= ~(1 << GPRS_RLCMAC_FLAG_TO_DL_ACK);
-		LOGP(DRLCMAC, LOGL_NOTICE, "Recovered EGPRS downlink ack "
-			"for %s\n", tbf_name(tbf));
-	}
+
+	try_dl_ack_recovery(tbf);
+
 	/* reset N3105 */
 	tbf->n3105 = 0;
 	tbf->stop_t3191();
diff --git a/src/tbf.cpp b/src/tbf.cpp
index 48e8289..5a38c49 100644
--- a/src/tbf.cpp
+++ b/src/tbf.cpp
@@ -393,7 +393,7 @@
 	}
 	tbf->m_contention_resolution_done = 1;
 	tbf->set_state(GPRS_RLCMAC_ASSIGN);
-	tbf->state_flags |= (1 << GPRS_RLCMAC_FLAG_PACCH);
+	OSMO_BIT_SET(tbf->state_flags, GPRS_RLCMAC_FLAG_PACCH);
 	tbf_timer_start(tbf, 3169, bts->t3169, 0);
 	tbf->update_ms(tlli, GPRS_RLCMAC_UL_TBF);
 	OSMO_ASSERT(tbf->ms());
@@ -613,11 +613,11 @@
 	poll_state = GPRS_RLCMAC_POLL_NONE;
 
 	if (ul_ack_state == GPRS_RLCMAC_UL_ACK_WAIT_ACK) {
-		if (!(state_flags & (1 << GPRS_RLCMAC_FLAG_TO_UL_ACK))) {
+		if (!OSMO_BIT_GET(state_flags, GPRS_RLCMAC_FLAG_TO_UL_ACK)) {
 			LOGP(DRLCMAC, LOGL_NOTICE, "- Timeout for polling "
 				"PACKET CONTROL ACK for PACKET UPLINK ACK\n");
 			rlcmac_diag();
-			state_flags |= (1 << GPRS_RLCMAC_FLAG_TO_UL_ACK);
+			OSMO_BIT_SET(state_flags, GPRS_RLCMAC_FLAG_TO_UL_ACK);
 		}
 		ul_ack_state = GPRS_RLCMAC_UL_ACK_NONE;
 		bts->rlc_ack_timedout();
@@ -638,12 +638,12 @@
 		}
 
 	} else if (ul_ass_state == GPRS_RLCMAC_UL_ASS_WAIT_ACK) {
-		if (!(state_flags & (1 << GPRS_RLCMAC_FLAG_TO_UL_ASS))) {
+		if (!OSMO_BIT_GET(state_flags, GPRS_RLCMAC_FLAG_TO_UL_ASS)) {
 			LOGP(DRLCMAC, LOGL_NOTICE, "- Timeout for polling "
 				"PACKET CONTROL ACK for PACKET UPLINK "
 				"ASSIGNMENT.\n");
 			rlcmac_diag();
-			state_flags |= (1 << GPRS_RLCMAC_FLAG_TO_UL_ASS);
+			OSMO_BIT_SET(state_flags, GPRS_RLCMAC_FLAG_TO_UL_ASS);
 		}
 		ul_ass_state = GPRS_RLCMAC_UL_ASS_NONE;
 		n3105++;
@@ -660,12 +660,12 @@
 		/* reschedule UL assignment */
 		ul_ass_state = GPRS_RLCMAC_UL_ASS_SEND_ASS;
 	} else if (dl_ass_state == GPRS_RLCMAC_DL_ASS_WAIT_ACK) {
-		if (!(state_flags & (1 << GPRS_RLCMAC_FLAG_TO_DL_ASS))) {
+		if (!OSMO_BIT_GET(state_flags, GPRS_RLCMAC_FLAG_TO_DL_ASS)) {
 			LOGP(DRLCMAC, LOGL_NOTICE, "- Timeout for polling "
 				"PACKET CONTROL ACK for PACKET DOWNLINK "
 				"ASSIGNMENT.\n");
 			rlcmac_diag();
-			state_flags |= (1 << GPRS_RLCMAC_FLAG_TO_DL_ASS);
+			OSMO_BIT_SET(state_flags, GPRS_RLCMAC_FLAG_TO_DL_ASS);
 		}
 		dl_ass_state = GPRS_RLCMAC_DL_ASS_NONE;
 		n3105++;
@@ -684,11 +684,11 @@
 	} else if (direction == GPRS_RLCMAC_DL_TBF) {
 		gprs_rlcmac_dl_tbf *dl_tbf = as_dl_tbf(this);
 
-		if (!(dl_tbf->state_flags & (1 << GPRS_RLCMAC_FLAG_TO_DL_ACK))) {
+		if (!OSMO_BIT_GET(dl_tbf->state_flags, GPRS_RLCMAC_FLAG_TO_DL_ACK)) {
 			LOGP(DRLCMAC, LOGL_NOTICE, "- Timeout for polling "
 				"PACKET DOWNLINK ACK.\n");
 			dl_tbf->rlcmac_diag();
-			dl_tbf->state_flags |= (1 << GPRS_RLCMAC_FLAG_TO_DL_ACK);
+			OSMO_BIT_SET(dl_tbf->state_flags, GPRS_RLCMAC_FLAG_TO_DL_ACK);
 		}
 		dl_tbf->n3105++;
 		if (dl_tbf->state_is(GPRS_RLCMAC_RELEASING))
@@ -706,8 +706,8 @@
 			return;
 		}
 		/* resend IMM.ASS on CCCH on timeout */
-		if ((dl_tbf->state_flags & (1 << GPRS_RLCMAC_FLAG_CCCH))
-		 && !(dl_tbf->state_flags & (1 << GPRS_RLCMAC_FLAG_DL_ACK))) {
+		if (OSMO_BIT_GET(dl_tbf->state_flags, GPRS_RLCMAC_FLAG_CCCH)
+		    && !OSMO_BIT_GET(dl_tbf->state_flags, GPRS_RLCMAC_FLAG_DL_ACK)) {
 			LOGP(DRLCMAC, LOGL_DEBUG, "Re-send dowlink assignment "
 				"for %s on PCH (IMSI=%s)\n",
 				tbf_name(dl_tbf),
@@ -961,7 +961,7 @@
 
 	switch (T) {
 	case 0: /* assignment */
-		if ((state_flags & (1 << GPRS_RLCMAC_FLAG_PACCH))) {
+		if (OSMO_BIT_GET(state_flags, GPRS_RLCMAC_FLAG_PACCH)) {
 			if (state_is(GPRS_RLCMAC_ASSIGN)) {
 				LOGP(DRLCMAC, LOGL_NOTICE, "%s releasing due to "
 					"PACCH assignment timeout.\n", tbf_name(this));
@@ -971,7 +971,7 @@
 				LOGP(DRLCMAC, LOGL_ERROR, "Error: %s is not "
 					"in assign state\n", tbf_name(this));
 		}
-		if ((state_flags & (1 << GPRS_RLCMAC_FLAG_CCCH))) {
+		if (OSMO_BIT_GET(state_flags, GPRS_RLCMAC_FLAG_CCCH)) {
 			gprs_rlcmac_dl_tbf *dl_tbf = as_dl_tbf(this);
 			dl_tbf->m_wait_confirm = 0;
 			if (dl_tbf->state_is(GPRS_RLCMAC_ASSIGN)) {
@@ -1022,15 +1022,15 @@
 
 int gprs_rlcmac_tbf::rlcmac_diag()
 {
-	if ((state_flags & (1 << GPRS_RLCMAC_FLAG_CCCH)))
+	if (OSMO_BIT_GET(state_flags, GPRS_RLCMAC_FLAG_CCCH))
 		LOGP(DRLCMAC, LOGL_NOTICE, "- Assignment was on CCCH\n");
-	if ((state_flags & (1 << GPRS_RLCMAC_FLAG_PACCH)))
+	if (OSMO_BIT_GET(state_flags, GPRS_RLCMAC_FLAG_PACCH))
 		LOGP(DRLCMAC, LOGL_NOTICE, "- Assignment was on PACCH\n");
-	if ((state_flags & (1 << GPRS_RLCMAC_FLAG_UL_DATA)))
+	if (OSMO_BIT_GET(state_flags, GPRS_RLCMAC_FLAG_UL_DATA))
 		LOGP(DRLCMAC, LOGL_NOTICE, "- Uplink data was received\n");
 	else if (direction == GPRS_RLCMAC_UL_TBF)
 		LOGP(DRLCMAC, LOGL_NOTICE, "- No uplink data received yet\n");
-	if ((state_flags & (1 << GPRS_RLCMAC_FLAG_DL_ACK)))
+	if (OSMO_BIT_GET(state_flags, GPRS_RLCMAC_FLAG_DL_ACK))
 		LOGP(DRLCMAC, LOGL_NOTICE, "- Downlink ACK was received\n");
 	else if (direction == GPRS_RLCMAC_DL_TBF)
 		LOGP(DRLCMAC, LOGL_NOTICE, "- No downlink ACK received yet\n");
@@ -1427,7 +1427,7 @@
 	llist_add(&ul_tbf->list(), &bts->bts->ul_tbfs());
 	ul_tbf->bts->tbf_ul_created();
 	ul_tbf->set_state(GPRS_RLCMAC_ASSIGN);
-	ul_tbf->state_flags |= (1 << GPRS_RLCMAC_FLAG_PACCH);
+	OSMO_BIT_SET(ul_tbf->state_flags, GPRS_RLCMAC_FLAG_PACCH);
 
 	ul_tbf->set_ms(ms);
 	ul_tbf->update_ms(tlli, GPRS_RLCMAC_UL_TBF);
diff --git a/src/tbf.h b/src/tbf.h
index 09e3122..46c43a2 100644
--- a/src/tbf.h
+++ b/src/tbf.h
@@ -377,7 +377,7 @@
 	return state > GPRS_RLCMAC_ASSIGN ||
 		(direction == GPRS_RLCMAC_DL_TBF &&
 		 state == GPRS_RLCMAC_ASSIGN &&
-		 (state_flags & (1 << GPRS_RLCMAC_FLAG_CCCH)));
+		 OSMO_BIT_GET(state_flags, GPRS_RLCMAC_FLAG_CCCH));
 }
 
 inline uint8_t gprs_rlcmac_tbf::tfi() const
diff --git a/src/tbf_dl.cpp b/src/tbf_dl.cpp
index 24c6385..0efa893 100644
--- a/src/tbf_dl.cpp
+++ b/src/tbf_dl.cpp
@@ -777,7 +777,7 @@
 	if (m_last_dl_poll_fn < 0)
 		m_last_dl_poll_fn = fn;
 
-	need_poll = state_flags & (1 << GPRS_RLCMAC_FLAG_TO_DL_ACK);
+	need_poll = OSMO_BIT_GET(state_flags, GPRS_RLCMAC_FLAG_TO_DL_ACK);
 
 	/* poll after POLL_ACK_AFTER_FRAMES frames, or when final block is tx.
 	 */
@@ -808,7 +808,7 @@
 				tbf_timer_start(this, 3191, bts_data()->t3191, 0);
 
 			/* Clear poll timeout flag */
-			state_flags &= ~(1 << GPRS_RLCMAC_FLAG_TO_DL_ACK);
+			OSMO_BIT_CLEAR(state_flags, GPRS_RLCMAC_FLAG_TO_DL_ACK);
 
 			/* Clear request flag */
 			m_dl_ack_requested = false;
@@ -1080,7 +1080,7 @@
 
 	/* keep to flags */
 	state_flags &= GPRS_RLCMAC_FLAG_TO_MASK;
-	state_flags &= ~(1 << GPRS_RLCMAC_FLAG_CCCH);
+	OSMO_BIT_CLEAR(state_flags, GPRS_RLCMAC_FLAG_CCCH);
 
 	return 0;
 }
@@ -1108,7 +1108,7 @@
 
 	/* keep to flags */
 	state_flags &= GPRS_RLCMAC_FLAG_TO_MASK;
-	state_flags &= ~(1 << GPRS_RLCMAC_FLAG_CCCH);
+	OSMO_BIT_CLEAR(state_flags, GPRS_RLCMAC_FLAG_CCCH);
 
 	return 0;
 }
@@ -1159,7 +1159,7 @@
 	if (poll_state != GPRS_RLCMAC_POLL_NONE)
 		return false;
 
-	return state_flags & (1 << GPRS_RLCMAC_FLAG_TO_DL_ACK) ||
+	return OSMO_BIT_GET(state_flags, GPRS_RLCMAC_FLAG_TO_DL_ACK) ||
 		m_tx_counter >= POLL_ACK_AFTER_FRAMES ||
 		m_dl_ack_requested;
 }
diff --git a/src/tbf_ul.cpp b/src/tbf_ul.cpp
index 1eee41a..7c9f2f4 100644
--- a/src/tbf_ul.cpp
+++ b/src/tbf_ul.cpp
@@ -156,7 +156,7 @@
 
 	const uint16_t ws = m_window.ws();
 
-	this->state_flags |= (1 << GPRS_RLCMAC_FLAG_UL_DATA);
+	OSMO_BIT_SET(this->state_flags, GPRS_RLCMAC_FLAG_UL_DATA);
 
 	LOGP(DRLCMACUL, LOGL_DEBUG, "UL DATA TFI=%d received (V(Q)=%d .. "
 		"V(R)=%d)\n", rlc->tfi, this->m_window.v_q(),

-- 
To view, visit https://gerrit.osmocom.org/2858
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Icca5ce3273799e0165adcc062db8a4b71dd9fb04
Gerrit-PatchSet: 2
Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Owner: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder



More information about the gerrit-log mailing list