[PATCH] Refactor coding scheme assignment code

[Holger Freyther]

> On 24 Feb 2016, at 12:57, msuraev at sysmocom.de wrote:

Moving it to the BTS list. After we had some performance issues today (and just pointing out the need for a stable test setup and some repeated measurements). I looked at the code more carefully and have some more comments we should 



> 
> +inline static GprsCodingScheme assign_cs(GprsCodingScheme current, BTS *bts, bool uplink, GprsCodingScheme::Mode mode)
> +{

"assign" is a misleading name. It doesn't have a side-effect so it should be called pick_cs (or such). Please make "current" const as well.



> +	GprsCodingScheme tmp = GprsCodingScheme::UNKNOWN;
> +	struct gprs_rlcmac_bts * b = bts->bts_data();
> +
> +	if (GprsCodingScheme::GPRS == mode) {
> +		if (!current.isGprs()) {
> +			tmp = GprsCodingScheme::getGprsByNum(uplink ? b->initial_cs_ul : b->initial_cs_dl);
> +			if (!tmp.isValid())
> +				return GprsCodingScheme::CS1;
> +		}
> +	} else {
> +		if (!current.isEgprs()) {
> +			tmp = GprsCodingScheme::getEgprsByNum(uplink ? b->initial_mcs_ul : b->initial_mcs_dl);
> +			if (!tmp.isValid())
> +				return GprsCodingScheme::MCS1;
> +		}
> +	}
> +
> +    return tmp;
> +}
> +
> void GprsMs::set_mode(GprsCodingScheme::Mode mode)
> {
> +	GprsCodingScheme tmp;
> 	m_mode = mode;
> 
> 	if (!m_bts)
> 		return;
> 
> -	switch (m_mode) {
> -	case GprsCodingScheme::GPRS:
> -		if (!m_current_cs_ul.isGprs()) {
> -			m_current_cs_ul = GprsCodingScheme::getGprsByNum(
> -				m_bts->bts_data()->initial_cs_ul);
> -			if (!m_current_cs_ul.isValid())
> -				m_current_cs_ul = GprsCodingScheme::CS1;
> -		}
> -		if (!m_current_cs_dl.isGprs()) {
> -			m_current_cs_dl = GprsCodingScheme::getGprsByNum(
> -				m_bts->bts_data()->initial_cs_dl);
> -			if (!m_current_cs_dl.isValid())
> -				m_current_cs_dl = GprsCodingScheme::CS1;
> -		}
> -		break;
> -
> -	case GprsCodingScheme::EGPRS_GMSK:
> -	case GprsCodingScheme::EGPRS:
> -		if (!m_current_cs_ul.isEgprs()) {
> -			m_current_cs_ul = GprsCodingScheme::getEgprsByNum(
> -				m_bts->bts_data()->initial_mcs_ul);
> -			if (!m_current_cs_dl.isValid())
> -				m_current_cs_ul = GprsCodingScheme::MCS1;
> -		}
> -		if (!m_current_cs_dl.isEgprs()) {
> -			m_current_cs_dl = GprsCodingScheme::getEgprsByNum(
> -				m_bts->bts_data()->initial_mcs_dl);
> -			if (!m_current_cs_dl.isValid())
> -				m_current_cs_dl = GprsCodingScheme::MCS1;
> -		}
> -		break;
> -	}
> +	tmp = assign_cs(m_current_cs_ul, m_bts, true, mode);
> +	if (tmp)
> +		m_current_cs_ul = tmp;
> +
> +	tmp = assign_cs(m_current_cs_dl, m_bts, false, mode);
> +	if (tmp)
> +		m_current_cs_dl = tmp;


let us change the code-flow here. There is a subtle difference between operator bool() and isValid but we avoid that by:

a.) By passing m_current_cs_ul/m_current_cs_dl as pointer and really assign
b.) By making sure pick_cs always returns a valid result?



diff --git a/src/gprs_ms.cpp b/src/gprs_ms.cpp
index 78f03f8..98c4fda 100644
--- a/src/gprs_ms.cpp
+++ b/src/gprs_ms.cpp
@@ -209,6 +209,25 @@ void GprsMs::stop_timer()
 	unref();
 }
 
+static inline void assign_cs(GprsCodingScheme *current, BTS *bts, bool uplink, GprsCodingScheme::Mode mode)
+{
+	struct gprs_rlcmac_bts * b = bts->bts_data();
+
+	if (GprsCodingScheme::GPRS == mode) {
+		if (current->isGprs())
+			return;
+		*current = GprsCodingScheme::getGprsByNum(uplink ? b->initial_cs_ul : b->initial_cs_dl);
+		if (!current->isValid())
+			*current = GprsCodingScheme::CS1;
+	} else {
+		if (current->isEgprs())
+			return;
+		*current = GprsCodingScheme::getEgprsByNum(uplink ? b->initial_mcs_ul : b->initial_mcs_dl);
+		if (!current->isValid())
+			*current = GprsCodingScheme::MCS1;
+	}
+}
+
 void GprsMs::set_mode(GprsCodingScheme::Mode mode)
 {
 	m_mode = mode;
@@ -216,38 +235,11 @@ void GprsMs::set_mode(GprsCodingScheme::Mode mode)
 	if (!m_bts)
 		return;
 
-	switch (m_mode) {
-	case GprsCodingScheme::GPRS:
-		if (!m_current_cs_ul.isGprs()) {
-			m_current_cs_ul = GprsCodingScheme::getGprsByNum(
-				m_bts->bts_data()->initial_cs_ul);
-			if (!m_current_cs_ul.isValid())
-				m_current_cs_ul = GprsCodingScheme::CS1;
-		}
-		if (!m_current_cs_dl.isGprs()) {
-			m_current_cs_dl = GprsCodingScheme::getGprsByNum(
-				m_bts->bts_data()->initial_cs_dl);
-			if (!m_current_cs_dl.isValid())
-				m_current_cs_dl = GprsCodingScheme::CS1;
-		}
-		break;
-
-	case GprsCodingScheme::EGPRS_GMSK:
-	case GprsCodingScheme::EGPRS:
-		if (!m_current_cs_ul.isEgprs()) {
-			m_current_cs_ul = GprsCodingScheme::getEgprsByNum(
-				m_bts->bts_data()->initial_mcs_ul);
-			if (!m_current_cs_dl.isValid())
-				m_current_cs_ul = GprsCodingScheme::MCS1;
-		}
-		if (!m_current_cs_dl.isEgprs()) {
-			m_current_cs_dl = GprsCodingScheme::getEgprsByNum(
-				m_bts->bts_data()->initial_mcs_dl);
-			if (!m_current_cs_dl.isValid())
-				m_current_cs_dl = GprsCodingScheme::MCS1;
-		}
-		break;
-	}
+	assign_cs(&m_current_cs_ul, m_bts, true, mode);
+	assign_cs(&m_current_cs_dl, m_bts, false, mode);
+
+	LOGP(DRLCMAC, LOGL_DEBUG, "MS IMSI=%s mode set to %s: UL=%s, DL=%s\n",
+		imsi(), tmp.modeName(mode), m_current_cs_ul.name(), m_current_cs_dl.name());
 }
 
 void GprsMs::attach_tbf(struct gprs_rlcmac_tbf *tbf)