[PATCH] osmo-bts[master]: DTX AMR HR: fix inhibition

Max gerrit-no-reply at lists.osmocom.org
Wed Jan 4 10:26:11 UTC 2017


Hello Jenkins Builder,

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

    https://gerrit.osmocom.org/1508

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

DTX AMR HR: fix inhibition

* Unlike in AMR FR, in AMR HR incoming ONSET have to be treated
  differently depending on whether we've recently sent SID UPDATE or
  EMPTY frame. Split ST_SID_U FSM state into 2 states to accommodate for
  that and make sure that additional states specific to AMR HR are not
  used for AMR FR.

* Avoid sending E_VOICE and E_SID_U in corresponding states
  as those do not initiate FSM state transitions anyway. This decrease
  extra load from FSM signalling which otherwise would be triggered on
  per-frame basis.

* Introduce separate signal for SID First P1 -> P2 transition to avoid
  confusion with E_COMPL and E_SID_U initiated transitions from P1
  state.

* Don't init DTX FSM for SDCCH channels.

Change-Id: I229ba39a38a223fada4881fc9aca35d3639371f8
Related: OS#1801
---
M include/osmo-bts/dtx_dl_amr_fsm.h
M include/osmo-bts/msg_utils.h
M src/common/bts.c
M src/common/dtx_dl_amr_fsm.c
M src/common/l1sap.c
M src/common/msg_utils.c
M src/osmo-bts-litecell15/l1_if.c
M src/osmo-bts-litecell15/tch.c
M src/osmo-bts-sysmo/l1_if.c
M src/osmo-bts-sysmo/tch.c
10 files changed, 135 insertions(+), 42 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/08/1508/4

diff --git a/include/osmo-bts/dtx_dl_amr_fsm.h b/include/osmo-bts/dtx_dl_amr_fsm.h
index 4fb2f25..f747f9f 100644
--- a/include/osmo-bts/dtx_dl_amr_fsm.h
+++ b/include/osmo-bts/dtx_dl_amr_fsm.h
@@ -14,6 +14,7 @@
 	ST_SID_F2,
 	ST_F1_INH,
 	ST_U_INH,
+	ST_U_NOINH,
 	ST_F1_INH_REC,
 	ST_U_INH_REC,
 	ST_SID_U,
@@ -29,6 +30,7 @@
 	E_ONSET,
 	E_FACCH,
 	E_COMPL,
+	E_FIRST,
 	E_INHIB,
 	E_SID_F,
 	E_SID_U,
diff --git a/include/osmo-bts/msg_utils.h b/include/osmo-bts/msg_utils.h
index 55e8475..7ddbe88 100644
--- a/include/osmo-bts/msg_utils.h
+++ b/include/osmo-bts/msg_utils.h
@@ -37,6 +37,7 @@
 void dtx_dispatch(struct gsm_lchan *lchan, enum dtx_dl_amr_fsm_events e);
 bool dtx_recursion(const struct gsm_lchan *lchan);
 void dtx_int_signal(struct gsm_lchan *lchan);
+bool dtx_is_first_p1(const struct gsm_lchan *lchan);
 void dtx_cache_payload(struct gsm_lchan *lchan, const uint8_t *l1_payload,
 		       size_t length, uint32_t fn, int update);
 int dtx_dl_amr_fsm_step(struct gsm_lchan *lchan, const uint8_t *rtp_pl,
diff --git a/src/common/bts.c b/src/common/bts.c
index 9c2f0e0..d249137 100644
--- a/src/common/bts.c
+++ b/src/common/bts.c
@@ -45,6 +45,7 @@
 #include <osmo-bts/rsl.h>
 #include <osmo-bts/oml.h>
 #include <osmo-bts/signal.h>
+#include <osmo-bts/dtx_dl_amr_fsm.h>
 
 #define MIN_QUAL_RACH    5.0f   /* at least  5 dB C/I */
 #define MIN_QUAL_NORM   -0.5f   /* at least -1 dB C/I */
diff --git a/src/common/dtx_dl_amr_fsm.c b/src/common/dtx_dl_amr_fsm.c
index d903b0c..832e8b4 100644
--- a/src/common/dtx_dl_amr_fsm.c
+++ b/src/common/dtx_dl_amr_fsm.c
@@ -47,7 +47,7 @@
    Was observed during testing, let's just ignore it for now */
 		break;
 	case E_SID_U:
-		osmo_fsm_inst_state_chg(fi, ST_SID_U, 0, 0);
+		osmo_fsm_inst_state_chg(fi, ST_U_NOINH, 0, 0);
 		break;
 	case E_VOICE:
 		osmo_fsm_inst_state_chg(fi, ST_VOICE, 0, 0);
@@ -55,7 +55,7 @@
 	case E_FACCH:
 		osmo_fsm_inst_state_chg(fi, ST_ONSET_F, 0, 0);
 		break;
-	case E_COMPL:
+	case E_FIRST:
 		osmo_fsm_inst_state_chg(fi, ST_SID_F2, 0, 0);
 		break;
 	case E_INHIB:
@@ -74,7 +74,7 @@
 void dtx_fsm_sid_f2(struct osmo_fsm_inst *fi, uint32_t event, void *data)
 {
 	switch (event) {
-	case E_SID_U:
+	case E_COMPL:
 		osmo_fsm_inst_state_chg(fi, ST_SID_U, 0, 0);
 		break;
 	case E_VOICE:
@@ -145,6 +145,33 @@
 	}
 }
 
+void dtx_fsm_u_noinh(struct osmo_fsm_inst *fi, uint32_t event, void *data)
+{
+	switch (event) {
+	case E_FACCH:
+		osmo_fsm_inst_state_chg(fi, ST_ONSET_F, 0, 0);
+		break;
+	case E_VOICE:
+		osmo_fsm_inst_state_chg(fi, ST_VOICE, 0, 0);
+		break;
+	case E_COMPL:
+		osmo_fsm_inst_state_chg(fi, ST_SID_U, 0, 0);
+		break;
+	case E_SID_U:
+	case E_SID_F:
+/* FIXME: what shall we do if we get SID-FIRST _after_ sending SID-UPDATE?
+   Was observed during testing, let's just ignore it for now */
+		break;
+	case E_ONSET:
+		osmo_fsm_inst_state_chg(fi, ST_ONSET_V, 0, 0);
+		break;
+	default:
+		LOGP(DL1P, LOGL_ERROR, "Unexpected event %d\n", event);
+		OSMO_ASSERT(0);
+		break;
+	}
+}
+
 void dtx_fsm_sid_upd(struct osmo_fsm_inst *fi, uint32_t event, void *data)
 {
 	switch (event) {
@@ -159,11 +186,7 @@
 		break;
 	case E_SID_U:
 	case E_SID_F:
-/* FIXME: what shall we do if we get SID-FIRST _after_ sending SID-UPDATE?
-   Was observed during testing, let's just ignore it for now */
-		break;
-	case E_ONSET:
-		osmo_fsm_inst_state_chg(fi, ST_ONSET_V, 0, 0);
+		osmo_fsm_inst_state_chg(fi, ST_U_NOINH, 0, 0);
 		break;
 	default:
 		LOGP(DL1P, LOGL_ERROR, "Unexpected event %d\n", event);
@@ -255,15 +278,15 @@
 	/* SID-FIRST or SID-FIRST-P1 in case of AMR HR:
 	   start of silence period (might be interrupted in case of AMR HR) */
 	[ST_SID_F1]= {
-		.in_event_mask = X(E_SID_F) | X(E_SID_U) | X(E_VOICE) | X(E_FACCH) | X(E_COMPL) | X(E_INHIB) | X(E_ONSET),
-		.out_state_mask = X(ST_SID_U) | X(ST_VOICE) | X(ST_ONSET_F) | X(ST_SID_F2) | X(ST_F1_INH) | X(ST_ONSET_V),
+		.in_event_mask = X(E_SID_F) | X(E_SID_U) | X(E_VOICE) | X(E_FACCH) | X(E_FIRST) | X(E_INHIB) | X(E_ONSET),
+		.out_state_mask = X(ST_U_NOINH) | X(ST_VOICE) | X(ST_ONSET_F) | X(ST_SID_F2) | X(ST_F1_INH) | X(ST_ONSET_V),
 		.name = "SID-FIRST (P1)",
 		.action = dtx_fsm_sid_f1,
 	},
 	/* SID-FIRST P2 (only for AMR HR):
 	   actual start of silence period in case of AMR HR */
 	[ST_SID_F2]= {
-		.in_event_mask = X(E_SID_U) | X(E_VOICE) | X(E_FACCH) | X(E_ONSET),
+		.in_event_mask = X(E_COMPL) | X(E_VOICE) | X(E_FACCH) | X(E_ONSET),
 		.out_state_mask = X(ST_SID_U) | X(ST_VOICE) | X(ST_ONSET_F) | X(ST_ONSET_V),
 		.name = "SID-FIRST (P2)",
 		.action = dtx_fsm_sid_f2,
@@ -281,6 +304,13 @@
 		.out_state_mask = X(ST_U_INH_REC),
 		.name = "SID-UPDATE (Inh)",
 		.action = dtx_fsm_u_inh,
+	},
+	/* SID-UPDATE: Inhibited not allowed (only for AMR HR) */
+	[ST_U_NOINH]= {
+		.in_event_mask = X(E_FACCH) | X(E_VOICE) | X(E_COMPL) | X(E_SID_U) | X(E_SID_F) | X(E_ONSET),
+		.out_state_mask = X(ST_ONSET_F) | X(ST_VOICE) | X(ST_SID_U) | X(ST_ONSET_V),
+		.name = "SID-UPDATE (NoInh)",
+		.action = dtx_fsm_u_noinh,
 	},
 	/* SID-FIRST Inhibition recursion in progress:
 	   Inhibit itself was already sent, now have to send the voice that caused it */
@@ -300,9 +330,9 @@
 	},
 	/* Silence period with periodic comfort noise data updates */
 	[ST_SID_U]= {
-		.in_event_mask = X(E_FACCH) | X(E_VOICE) | X(E_INHIB) | X(E_SID_U) | X(E_SID_F) | X(E_ONSET),
-		.out_state_mask = X(ST_ONSET_F) | X(ST_VOICE) | X(ST_U_INH) | X(ST_SID_U) | X(ST_ONSET_V),
-		.name = "SID-UPDATE",
+		.in_event_mask = X(E_FACCH) | X(E_VOICE) | X(E_INHIB) | X(E_SID_U) | X(E_SID_F),
+		.out_state_mask = X(ST_ONSET_F) | X(ST_VOICE) | X(ST_U_INH) | X(ST_U_NOINH),
+		.name = "SID-UPDATE (AMR/HR)",
 		.action = dtx_fsm_sid_upd,
 	},
 	/* ONSET - end of silent period due to incoming SPEECH frame */
@@ -350,6 +380,7 @@
 	{ E_ONSET,	"ONSET" },
 	{ E_FACCH,	"FACCH" },
 	{ E_COMPL,	"Complete" },
+	{ E_FIRST,	"FIRST P1->P2" },
 	{ E_INHIB,	"Inhibit" },
 	{ E_SID_F,	"SID-FIRST" },
 	{ E_SID_U,	"SID-UPDATE" },
diff --git a/src/common/l1sap.c b/src/common/l1sap.c
index e9c94f0..e6e53db 100644
--- a/src/common/l1sap.c
+++ b/src/common/l1sap.c
@@ -1161,8 +1161,7 @@
 		return -RSL_ERR_EQUIPMENT_FAIL;
 
 	/* Init DTX DL FSM if necessary */
-	//FIXME: only do it for AMR TCH/*
-	if (trx->bts->dtxd)
+	if (trx->bts->dtxd && lchan->type != GSM_LCHAN_SDCCH)
 		lchan->tch.dtx.dl_amr_fsm = osmo_fsm_inst_alloc(&dtx_dl_amr_fsm,
 								tall_bts_ctx,
 								lchan,
diff --git a/src/common/msg_utils.c b/src/common/msg_utils.c
index 9de9b6d..062f5e3 100644
--- a/src/common/msg_utils.c
+++ b/src/common/msg_utils.c
@@ -93,6 +93,28 @@
 	return type;
 }
 
+/* check that DTX is in the middle of silence */
+static inline bool dtx_is_update(const struct gsm_lchan *lchan)
+{
+	if (!dtx_dl_amr_enabled(lchan))
+		return false;
+	if (lchan->tch.dtx.dl_amr_fsm->state == ST_SID_U ||
+	    lchan->tch.dtx.dl_amr_fsm->state == ST_U_NOINH)
+		return true;
+	return false;
+}
+
+/* check that DTX is in the beginning of silence for AMR HR */
+bool dtx_is_first_p1(const struct gsm_lchan *lchan)
+{
+	if (!dtx_dl_amr_enabled(lchan))
+		return false;
+	if ((lchan->type == GSM_LCHAN_TCH_H &&
+	     lchan->tch.dtx.dl_amr_fsm->state == ST_SID_F1))
+		return true;
+	return false;
+}
+
 /* update lchan SID status */
 void lchan_set_marker(bool t, struct gsm_lchan *lchan)
 {
@@ -123,9 +145,7 @@
 	if (update == 0) {
 		lchan->tch.dtx.is_update = false; /* Mark SID FIRST explicitly */
 		/* for non-AMR case - always update FN for incoming SID FIRST */
-		if (!amr ||
-		    (dtx_dl_amr_enabled(lchan) &&
-		     lchan->tch.dtx.dl_amr_fsm->state != ST_SID_U))
+		if (!amr || !dtx_is_update(lchan))
 			lchan->tch.dtx.fn = fn;
 		/* for AMR case - do not update FN if SID FIRST arrives in a
 		   middle of silence: this should not be happening according to
@@ -157,12 +177,24 @@
 	int rc;
 
 	if (dtx_dl_amr_enabled(lchan)) {
-		if (lchan->type == GSM_LCHAN_TCH_H &&
-		    lchan->tch.dtx.dl_amr_fsm->state == ST_SID_F2 && !rtp_pl) {
-			*len = 3; /* SID-FIRST P1 -> P2 completion */
-			memcpy(l1_payload, lchan->tch.dtx.cache, 2);
-			dtx_dispatch(lchan, E_SID_U);
-			return 0;
+		if (lchan->type == GSM_LCHAN_TCH_H && !rtp_pl) {
+			/* we're called by gen_empty_tch_msg() to handle states
+			   specific to AMR HR DTX */
+			switch (lchan->tch.dtx.dl_amr_fsm->state) {
+			case ST_SID_F2:
+				*len = 3; /* SID-FIRST P1 -> P2 completion */
+				memcpy(l1_payload, lchan->tch.dtx.cache, 2);
+				rc = 0;
+				dtx_dispatch(lchan, E_COMPL);
+				break;
+			case ST_SID_U:
+				rc = -EBADMSG;
+				dtx_dispatch(lchan, E_SID_U);
+				break;
+			default:
+				rc = -EBADMSG;
+			}
+			return rc;
 		}
 	}
 
@@ -171,8 +203,8 @@
 
 	rc = osmo_amr_rtp_dec(rtp_pl, rtp_pl_len, &cmr, &cmi, &ft, &bfi, &sti);
 	if (rc < 0) {
-		LOGP(DRTP, LOGL_ERROR, "failed to decode AMR RTP (length %zu)\n",
-		     rtp_pl_len);
+		LOGP(DRTP, LOGL_ERROR, "failed to decode AMR RTP (length %zu, "
+		     "%p)\n", rtp_pl_len, rtp_pl);
 		return rc;
 	}
 
@@ -197,19 +229,29 @@
 		return 0;
 
 	if (osmo_amr_is_speech(ft)) {
-		/* AMR HR - Inhibition */
-		if (lchan->type == GSM_LCHAN_TCH_H && marker &&
-		    lchan->tch.dtx.dl_amr_fsm->state == ST_SID_F1)
+		/* AMR HR - SID-FIRST_P1 Inhibition */
+		if (marker && dtx_is_first_p1(lchan))
 			return osmo_fsm_inst_dispatch(lchan->tch.dtx.dl_amr_fsm,
 						      E_INHIB, (void *)lchan);
+
+		/* AMR HR - SID-UPDATE Inhibition */
+		if (marker && lchan->type == GSM_LCHAN_TCH_H &&
+		    lchan->tch.dtx.dl_amr_fsm->state == ST_SID_U)
+			return osmo_fsm_inst_dispatch(lchan->tch.dtx.dl_amr_fsm,
+						      E_INHIB, (void *)lchan);
+
 		/* AMR FR & HR - generic */
 		if (marker && (lchan->tch.dtx.dl_amr_fsm->state == ST_SID_F1 ||
 			       lchan->tch.dtx.dl_amr_fsm->state == ST_SID_F2 ||
-			       lchan->tch.dtx.dl_amr_fsm->state == ST_SID_U ))
+			       lchan->tch.dtx.dl_amr_fsm->state == ST_U_NOINH))
 			return osmo_fsm_inst_dispatch(lchan->tch.dtx.dl_amr_fsm,
 						      E_ONSET, (void *)lchan);
-		return osmo_fsm_inst_dispatch(lchan->tch.dtx.dl_amr_fsm, E_VOICE,
-					      (void *)lchan);
+
+		if (lchan->tch.dtx.dl_amr_fsm->state != ST_VOICE)
+			return osmo_fsm_inst_dispatch(lchan->tch.dtx.dl_amr_fsm,
+						      E_VOICE, (void *)lchan);
+
+		return 0;
 	}
 
 	if (ft == AMR_SID) {
@@ -220,8 +262,11 @@
 			dtx_cache_payload(lchan, rtp_pl, rtp_pl_len, fn, false);
 			return osmo_fsm_inst_dispatch(lchan->tch.dtx.dl_amr_fsm,
 						      E_SID_F, (void *)lchan);
-		} else
+		} else if (lchan->tch.dtx.dl_amr_fsm->state != ST_FACCH)
 			dtx_cache_payload(lchan, rtp_pl, rtp_pl_len, fn, sti);
+		if (lchan->tch.dtx.dl_amr_fsm->state == ST_SID_F2)
+			return osmo_fsm_inst_dispatch(lchan->tch.dtx.dl_amr_fsm,
+						      E_COMPL, (void *)lchan);
 		return osmo_fsm_inst_dispatch(lchan->tch.dtx.dl_amr_fsm,
 					      sti ? E_SID_U : E_SID_F,
 					      (void *)lchan);
@@ -357,6 +402,7 @@
 		return false;
 
 	if (lchan->tch.dtx.dl_amr_fsm->state == ST_U_INH ||
+	    lchan->tch.dtx.dl_amr_fsm->state == ST_U_INH_REC ||
 	    lchan->tch.dtx.dl_amr_fsm->state == ST_F1_INH ||
 	    lchan->tch.dtx.dl_amr_fsm->state == ST_ONSET_F ||
 	    lchan->tch.dtx.dl_amr_fsm->state == ST_ONSET_V ||
@@ -389,9 +435,7 @@
 	if (!dtx_dl_amr_enabled(lchan))
 		return;
 
-	if ((lchan->type == GSM_LCHAN_TCH_H &&
-	     lchan->tch.dtx.dl_amr_fsm->state == ST_SID_F1) ||
-	    dtx_recursion(lchan))
+	if (dtx_is_first_p1(lchan) || dtx_recursion(lchan))
 		dtx_dispatch(lchan, E_COMPL);
 }
 
@@ -425,12 +469,17 @@
 				osmo_fsm_inst_dispatch(lchan->tch.dtx.dl_amr_fsm,
 						       E_SID_U, (void *)lchan);
 				dtx_sti_unset(lchan);
-			} else if (lchan->tch.dtx.dl_amr_fsm->state ==
-				   ST_SID_U) {
+			} else if (dtx_is_update(lchan)) {
 				/* enforce SID UPDATE for next repetition: it
 				   might have been altered by FACCH handling */
 				dtx_sti_set(lchan);
-			lchan->tch.dtx.is_update = true;
+				if (lchan->type == GSM_LCHAN_TCH_H &&
+				    lchan->tch.dtx.dl_amr_fsm->state ==
+				    ST_U_NOINH)
+					osmo_fsm_inst_dispatch(lchan->tch.dtx.dl_amr_fsm,
+							       E_COMPL,
+							       (void *)lchan);
+				lchan->tch.dtx.is_update = true;
 			}
 		}
 		memcpy(dst, lchan->tch.dtx.cache, lchan->tch.dtx.len);
diff --git a/src/osmo-bts-litecell15/l1_if.c b/src/osmo-bts-litecell15/l1_if.c
index d959338..99533d7 100644
--- a/src/osmo-bts-litecell15/l1_if.c
+++ b/src/osmo-bts-litecell15/l1_if.c
@@ -416,6 +416,7 @@
 			}
 			if (sapi == GsmL1_Sapi_FacchH) {
 				sapi = GsmL1_Sapi_TchH;
+				subCh = L1SAP_CHAN2SS_TCHH(chan_nr);
 			}
 			if (sapi == GsmL1_Sapi_TchH || sapi == GsmL1_Sapi_TchF) {
 				/* FACCH interruption of DTX silence */
@@ -542,7 +543,10 @@
 	}
 	/* send message to DSP's queue */
 	osmo_wqueue_enqueue(&fl1->write_q[MQ_L1_WRITE], nmsg);
-	dtx_int_signal(lchan);
+	if (dtx_is_first_p1(lchan))
+		dtx_dispatch(lchan, E_FIRST);
+	else
+		dtx_int_signal(lchan);
 
 	if (dtx_recursion(lchan)) /* DTX: send voice after ONSET was sent */
 		return ph_tch_req(trx, l1sap->oph.msg, l1sap, true, false);
diff --git a/src/osmo-bts-litecell15/tch.c b/src/osmo-bts-litecell15/tch.c
index de3c7e3..a47a88f 100644
--- a/src/osmo-bts-litecell15/tch.c
+++ b/src/osmo-bts-litecell15/tch.c
@@ -322,6 +322,7 @@
 			dtx_cache_payload(lchan, rtp_pl, rtp_pl_len, fn, 0);
 			return 1;
 		case ST_SID_U:
+		case ST_U_NOINH:
 			return -EAGAIN;
 		case ST_FACCH:
 			return -EBADMSG;
diff --git a/src/osmo-bts-sysmo/l1_if.c b/src/osmo-bts-sysmo/l1_if.c
index 2a3caf9..ad9aa64 100644
--- a/src/osmo-bts-sysmo/l1_if.c
+++ b/src/osmo-bts-sysmo/l1_if.c
@@ -411,6 +411,7 @@
 			}
 			if (sapi == GsmL1_Sapi_FacchH) {
 				sapi = GsmL1_Sapi_TchH;
+				subCh = L1SAP_CHAN2SS_TCHH(chan_nr);
 			}
 			if (sapi == GsmL1_Sapi_TchH || sapi == GsmL1_Sapi_TchF) {
 				/* FACCH interruption of DTX silence */
@@ -537,7 +538,10 @@
 	}
 	/* send message to DSP's queue */
 	osmo_wqueue_enqueue(&fl1->write_q[MQ_L1_WRITE], nmsg);
-	dtx_int_signal(lchan);
+	if (dtx_is_first_p1(lchan))
+		dtx_dispatch(lchan, E_FIRST);
+	else
+		dtx_int_signal(lchan);
 
 	if (dtx_recursion(lchan)) /* DTX: send voice after ONSET was sent */
 		return ph_tch_req(trx, l1sap->oph.msg, l1sap, true, false);
diff --git a/src/osmo-bts-sysmo/tch.c b/src/osmo-bts-sysmo/tch.c
index 16c2cf3..bc495d9 100644
--- a/src/osmo-bts-sysmo/tch.c
+++ b/src/osmo-bts-sysmo/tch.c
@@ -420,6 +420,7 @@
 			dtx_cache_payload(lchan, rtp_pl, rtp_pl_len, fn, 0);
 			return 1;
 		case ST_SID_U:
+		case ST_U_NOINH:
 			return -EAGAIN;
 		case ST_FACCH:
 			return -EBADMSG;

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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I229ba39a38a223fada4881fc9aca35d3639371f8
Gerrit-PatchSet: 4
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Owner: Max <msuraev at sysmocom.de>
Gerrit-Reviewer: Jean-Francois Dionne <jf.dionne at nutaq.com>
Gerrit-Reviewer: Jenkins Builder


More information about the gerrit-log mailing list