From: Max msuraev@sysmocom.de
Consistently use enums instead of (u)int, char etc. Use bool for boolean types. Add extra debug output with channel mode and type to simplify troubleshooting. --- openbsc/include/openbsc/bsc_api.h | 4 +++- openbsc/include/openbsc/gsm_04_08.h | 2 +- openbsc/include/openbsc/mncc.h | 6 ++++-- openbsc/include/openbsc/mncc_int.h | 5 ++++- openbsc/src/libbsc/bsc_api.c | 21 ++++++++++++--------- openbsc/src/libbsc/gsm_04_08_utils.c | 12 +++--------- openbsc/src/libmsc/gsm_04_08.c | 22 +++++++++++++++++----- openbsc/src/libmsc/mncc_builtin.c | 15 +++++++++++---- openbsc/src/osmo-nitb/bsc_hack.c | 4 +++- 9 files changed, 58 insertions(+), 33 deletions(-)
diff --git a/openbsc/include/openbsc/bsc_api.h b/openbsc/include/openbsc/bsc_api.h index a3d12f2..d0a0164 100644 --- a/openbsc/include/openbsc/bsc_api.h +++ b/openbsc/include/openbsc/bsc_api.h @@ -3,6 +3,8 @@ #ifndef OPENBSC_BSC_API_H #define OPENBSC_BSC_API_H
+#include <stdbool.h> + #include "gsm_data.h"
#define BSC_API_CONN_POL_ACCEPT 0 @@ -45,7 +47,7 @@ struct bsc_api {
int bsc_api_init(struct gsm_network *network, struct bsc_api *api); int gsm0808_submit_dtap(struct gsm_subscriber_connection *conn, struct msgb *msg, int link_id, int allow_sacch); -int gsm0808_assign_req(struct gsm_subscriber_connection *conn, int chan_mode, int full_rate); +int gsm0808_assign_req(struct gsm_subscriber_connection *conn, enum gsm48_chan_mode chan_mode, bool full_rate); int gsm0808_cipher_mode(struct gsm_subscriber_connection *conn, int cipher, const uint8_t *key, int len, int include_imeisv); int gsm0808_page(struct gsm_bts *bts, unsigned int page_group, diff --git a/openbsc/include/openbsc/gsm_04_08.h b/openbsc/include/openbsc/gsm_04_08.h index fd0b89d..d333e13 100644 --- a/openbsc/include/openbsc/gsm_04_08.h +++ b/openbsc/include/openbsc/gsm_04_08.h @@ -79,7 +79,7 @@ int gsm48_extract_mi(uint8_t *classmark2, int length, char *mi_string, uint8_t * int gsm48_paging_extract_mi(struct gsm48_pag_resp *pag, int length, char *mi_string, uint8_t *mi_type); int gsm48_handle_paging_resp(struct gsm_subscriber_connection *conn, struct msgb *msg, struct gsm_subscriber *subscr);
-int gsm48_lchan_modify(struct gsm_lchan *lchan, uint8_t lchan_mode); +int gsm48_lchan_modify(struct gsm_lchan *lchan, enum gsm48_chan_mode lchan_mode); int gsm48_rx_rr_modif_ack(struct msgb *msg); int gsm48_parse_meas_rep(struct gsm_meas_rep *rep, struct msgb *msg);
diff --git a/openbsc/include/openbsc/mncc.h b/openbsc/include/openbsc/mncc.h index 49f0c8b..ce2f87d 100644 --- a/openbsc/include/openbsc/mncc.h +++ b/openbsc/include/openbsc/mncc.h @@ -26,6 +26,8 @@
#include <osmocom/core/linuxlist.h> #include <osmocom/gsm/mncc.h> +#include <osmocom/gsm/gsm_utils.h> +#include <osmocom/gsm/protocol/gsm_04_08.h>
#include <stdint.h>
@@ -156,8 +158,8 @@ struct gsm_mncc { int emergency; char imsi[16];
- unsigned char lchan_type; - unsigned char lchan_mode; + enum gsm_chan_t lchan_type; + enum gsm48_chan_mode lchan_mode; };
struct gsm_data_frame { diff --git a/openbsc/include/openbsc/mncc_int.h b/openbsc/include/openbsc/mncc_int.h index 213ce14..dafef25 100644 --- a/openbsc/include/openbsc/mncc_int.h +++ b/openbsc/include/openbsc/mncc_int.h @@ -3,12 +3,15 @@
#include <stdint.h>
+#include <osmocom/gsm/gsm_utils.h> +#include <osmocom/gsm/protocol/gsm_04_08.h> + struct mncc_int { uint8_t def_codec[2]; };
extern struct mncc_int mncc_int;
-uint8_t mncc_codec_for_mode(int lchan_type); +enum gsm48_chan_mode mncc_codec_for_mode(enum gsm_chan_t lchan_type);
#endif diff --git a/openbsc/src/libbsc/bsc_api.c b/openbsc/src/libbsc/bsc_api.c index e6d820d..dadb082 100644 --- a/openbsc/src/libbsc/bsc_api.c +++ b/openbsc/src/libbsc/bsc_api.c @@ -21,6 +21,8 @@ * */
+#include <stdbool.h> + #include <openbsc/bsc_api.h> #include <openbsc/bsc_rll.h> #include <openbsc/gsm_data.h> @@ -34,7 +36,7 @@ #include <openbsc/trau_mux.h>
#include <osmocom/gsm/protocol/gsm_08_08.h> - +#include <osmocom/gsm/protocol/gsm_04_08.h> #include <osmocom/core/talloc.h>
#define GSM0808_T10_VALUE 6, 0 @@ -155,7 +157,7 @@ static void assignment_t10_timeout(void *_conn) * Handle the multirate config */ static void handle_mr_config(struct gsm_subscriber_connection *conn, - struct gsm_lchan *lchan, int full_rate) + struct gsm_lchan *lchan, bool full_rate) { struct bsc_api *api; api = conn->bts->network->bsc_api; @@ -197,12 +199,10 @@ static void handle_mr_config(struct gsm_subscriber_connection *conn, * -> Assignment Complete/Assignment Failure * 5.) Release the SDCCH, continue signalling on the new link */ -static int handle_new_assignment(struct gsm_subscriber_connection *conn, int chan_mode, int full_rate) +static int handle_new_assignment(struct gsm_subscriber_connection *conn, enum gsm48_chan_mode chan_mode, bool full_rate) { struct gsm_lchan *new_lchan; - int chan_type; - - chan_type = full_rate ? GSM_LCHAN_TCH_F : GSM_LCHAN_TCH_H; + enum gsm_chan_t chan_type = full_rate ? GSM_LCHAN_TCH_F : GSM_LCHAN_TCH_H;
new_lchan = lchan_alloc(conn->bts, chan_type, 0);
@@ -337,7 +337,7 @@ int gsm0808_submit_dtap(struct gsm_subscriber_connection *conn, /* * \brief Check if the given channel is compatible with the mode/fullrate */ -static int chan_compat_with_mode(struct gsm_lchan *lchan, int chan_mode, int full_rate) +static int chan_compat_with_mode(struct gsm_lchan *lchan, enum gsm48_chan_mode chan_mode, bool full_rate) { switch (chan_mode) { case GSM48_CMODE_SIGN: @@ -384,7 +384,8 @@ static int chan_compat_with_mode(struct gsm_lchan *lchan, int chan_mode, int ful * * TODO: Add multirate configuration, make it work for more than audio. */ -int gsm0808_assign_req(struct gsm_subscriber_connection *conn, int chan_mode, int full_rate) +int gsm0808_assign_req(struct gsm_subscriber_connection *conn, + enum gsm48_chan_mode chan_mode, bool full_rate) { struct bsc_api *api; api = conn->bts->network->bsc_api; @@ -394,7 +395,9 @@ int gsm0808_assign_req(struct gsm_subscriber_connection *conn, int chan_mode, in goto error; } else { LOGP(DMSC, LOGL_NOTICE, - "Sending ChanModify for speech %d %d\n", chan_mode, full_rate); + "Sending ChanModify for speech: %s on channel %s, " + "full_rate %d\n", osmo_gsm48_chan_mode2str(chan_mode), + osmo_gsm48_chan_type2str(conn->lchan->type), full_rate); if (chan_mode == GSM48_CMODE_SPEECH_AMR) handle_mr_config(conn, conn->lchan, full_rate);
diff --git a/openbsc/src/libbsc/gsm_04_08_utils.c b/openbsc/src/libbsc/gsm_04_08_utils.c index 8c6dbef..79a8547 100644 --- a/openbsc/src/libbsc/gsm_04_08_utils.c +++ b/openbsc/src/libbsc/gsm_04_08_utils.c @@ -488,7 +488,7 @@ int gsm48_send_rr_ass_cmd(struct gsm_lchan *dest_lchan, struct gsm_lchan *lchan, }
/* 9.1.5 Channel mode modify: Modify the mode on the MS side */ -int gsm48_tx_chan_mode_modify(struct gsm_lchan *lchan, uint8_t mode) +int gsm48_tx_chan_mode_modify(struct gsm_lchan *lchan, enum gsm48_chan_mode mode) { struct msgb *msg = gsm48_msgb_alloc_name("GSM 04.08 CHN MOD"); struct gsm48_hdr *gh = (struct gsm48_hdr *) msgb_put(msg, sizeof(*gh)); @@ -513,15 +513,9 @@ int gsm48_tx_chan_mode_modify(struct gsm_lchan *lchan, uint8_t mode) return gsm48_sendmsg(msg); }
-int gsm48_lchan_modify(struct gsm_lchan *lchan, uint8_t lchan_mode) +int gsm48_lchan_modify(struct gsm_lchan *lchan, enum gsm48_chan_mode lchan_mode) { - int rc; - - rc = gsm48_tx_chan_mode_modify(lchan, lchan_mode); - if (rc < 0) - return rc; - - return rc; + return gsm48_tx_chan_mode_modify(lchan, lchan_mode); }
int gsm48_rx_rr_modif_ack(struct msgb *msg) diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c index 1524ec4..95dd647 100644 --- a/openbsc/src/libmsc/gsm_04_08.c +++ b/openbsc/src/libmsc/gsm_04_08.c @@ -59,6 +59,7 @@ #include <osmocom/gsm/gsm48.h> #include <osmocom/gsm/gsm0480.h> #include <osmocom/gsm/gsm_utils.h> +#include <osmocom/gsm/protocol/gsm_04_08.h> #include <osmocom/core/msgb.h> #include <osmocom/core/talloc.h> #include <osmocom/gsm/tlv.h> @@ -1598,11 +1599,15 @@ static int tch_map(struct gsm_lchan *lchan, struct gsm_lchan *remote_lchan) { struct gsm_bts *bts = lchan->ts->trx->bts; struct gsm_bts *remote_bts = remote_lchan->ts->trx->bts; + enum gsm_chan_t lt = lchan->type, rt = remote_lchan->type; int rc;
- DEBUGP(DCC, "Setting up TCH map between (bts=%u,trx=%u,ts=%u) and (bts=%u,trx=%u,ts=%u)\n", - bts->nr, lchan->ts->trx->nr, lchan->ts->nr, - remote_bts->nr, remote_lchan->ts->trx->nr, remote_lchan->ts->nr); + DEBUGP(DCC, "Setting up TCH map between (bts=%u,trx=%u,ts=%u,%s) and " + "(bts=%u,trx=%u,ts=%u,%s)\n", + bts->nr, lchan->ts->trx->nr, lchan->ts->nr, + osmo_gsm48_chan_type2str(lt), + remote_bts->nr, remote_lchan->ts->trx->nr, remote_lchan->ts->nr, + osmo_gsm48_chan_type2str(rt));
if (bts->type != remote_bts->type) { LOGP(DCC, LOGL_ERROR, "Cannot switch calls between different BTS types yet\n"); @@ -2999,6 +3004,7 @@ static int tch_rtp_create(struct gsm_network *net, uint32_t callref) struct gsm_bts *bts; struct gsm_lchan *lchan; struct gsm_trans *trans; + enum gsm48_chan_mode m;
/* Find callref */ trans = trans_find_by_callref(net, callref); @@ -3038,8 +3044,11 @@ static int tch_rtp_create(struct gsm_network *net, uint32_t callref) */ if (lchan->tch_mode == GSM48_CMODE_SIGN) { trans->conn->mncc_rtp_create_pending = 1; - return gsm0808_assign_req(trans->conn, - mncc_codec_for_mode(lchan->type), + m = mncc_codec_for_mode(lchan->type); + LOGP(DMNCC, LOGL_DEBUG, "RTP create: codec=%s, chan_type=%s\n", + osmo_gsm48_chan_mode2str(m), + osmo_gsm48_chan_type2str(lchan->type)); + return gsm0808_assign_req(trans->conn, m, lchan->type != GSM_LCHAN_TCH_H); }
@@ -3068,6 +3077,9 @@ static int tch_rtp_connect(struct gsm_network *net, void *arg) }
lchan = trans->conn->lchan; + LOGP(DMNCC, LOGL_DEBUG, "RTP connect: codec=%s, chan_type=%s\n", + osmo_gsm48_chan_mode2str(mncc_codec_for_mode(lchan->type)), + osmo_gsm48_chan_type2str(lchan->type));
/* TODO: Check if payload_msg_type is compatible with what we have */ if (rtp->payload_type != lchan->abis_ip.rtp_payload) { diff --git a/openbsc/src/libmsc/mncc_builtin.c b/openbsc/src/libmsc/mncc_builtin.c index 77df6fb..b1993ca 100644 --- a/openbsc/src/libmsc/mncc_builtin.c +++ b/openbsc/src/libmsc/mncc_builtin.c @@ -27,6 +27,9 @@ #include <string.h> #include <errno.h>
+#include <osmocom/gsm/gsm_utils.h> +#include <osmocom/gsm/protocol/gsm_04_08.h> + #include <openbsc/gsm_04_08.h> #include <openbsc/debug.h> #include <openbsc/mncc.h> @@ -65,7 +68,7 @@ static struct gsm_call *get_call_ref(uint32_t callref) return NULL; }
-uint8_t mncc_codec_for_mode(int lchan_type) +enum gsm48_chan_mode mncc_codec_for_mode(enum gsm_chan_t lchan_type) { /* FIXME: check codec capabilities of the phone */
@@ -75,7 +78,7 @@ uint8_t mncc_codec_for_mode(int lchan_type) return mncc_int.def_codec[1]; }
-static uint8_t determine_lchan_mode(struct gsm_mncc *setup) +static enum gsm48_chan_mode determine_lchan_mode(struct gsm_mncc *setup) { return mncc_codec_for_mode(setup->lchan_type); } @@ -138,7 +141,8 @@ static int mncc_setup_ind(struct gsm_call *call, int msg_type, memset(&mncc, 0, sizeof(struct gsm_mncc)); mncc.callref = call->callref; mncc.lchan_mode = determine_lchan_mode(setup); - DEBUGP(DMNCC, "(call %x) Modify channel mode.\n", call->callref); + DEBUGP(DMNCC, "(call %x) Modify channel mode: %s\n", call->callref, + osmo_gsm48_chan_mode2str(mncc.lchan_mode)); mncc_tx_to_cc(call->net, MNCC_LCHAN_MODIFY, &mncc);
/* send setup to remote */ @@ -207,13 +211,16 @@ static int mncc_setup_cnf(struct gsm_call *call, int msg_type, DEBUGP(DMNCC, "(call %x) Bridging with remote.\n", call->callref);
/* in direct mode, we always have to bridge the channels */ - if (ipacc_rtp_direct) + if (ipacc_rtp_direct) { + DEBUGP(DMNCC, "Bridging: direct RTP.\n"); return mncc_tx_to_cc(call->net, MNCC_BRIDGE, &bridge); + }
/* proxy mode */ if (!net->handover.active) { /* in the no-handover case, we can bridge, i.e. use * the old RTP proxy code */ + DEBUGP(DMNCC, "Bridging: no handover is active.\n"); return mncc_tx_to_cc(call->net, MNCC_BRIDGE, &bridge); } else { /* in case of handover, we need to re-write the RTP diff --git a/openbsc/src/osmo-nitb/bsc_hack.c b/openbsc/src/osmo-nitb/bsc_hack.c index dffe642..0b360dc 100644 --- a/openbsc/src/osmo-nitb/bsc_hack.c +++ b/openbsc/src/osmo-nitb/bsc_hack.c @@ -288,8 +288,10 @@ int main(int argc, char **argv) rc = bsc_bootstrap_network(mncc_sock_from_cc, config_file); if (rc >= 0) mncc_sock_init(bsc_gsmnet, mncc_sock_path); - } else + } else { + DEBUGP(DMNCC, "Using internal MNCC handler.\n"); rc = bsc_bootstrap_network(int_mncc_recv, config_file); + } if (rc < 0) exit(1); #ifdef BUILD_SMPP
From: Max msuraev@sysmocom.de
Note: ideally this situation should not happen - we should check channel compatibility before paging 2nd leg of the call.
Fixes: OS#1663 --- openbsc/src/libmsc/gsm_04_08.c | 34 +++++++++++++++++++++++++++++++++- 1 file changed, 33 insertions(+), 1 deletion(-)
diff --git a/openbsc/src/libmsc/gsm_04_08.c b/openbsc/src/libmsc/gsm_04_08.c index 95dd647..2f03f75 100644 --- a/openbsc/src/libmsc/gsm_04_08.c +++ b/openbsc/src/libmsc/gsm_04_08.c @@ -1614,6 +1614,13 @@ static int tch_map(struct gsm_lchan *lchan, struct gsm_lchan *remote_lchan) return -EINVAL; }
+ if (lt != rt) { + LOGP(DCC, LOGL_ERROR, "Cannot patch through call with different" + " channel types: local = %s, remote = %s\n", + osmo_gsm48_chan_type2str(lt), osmo_gsm48_chan_type2str(rt)); + return -EBADSLT; + } + // todo: map between different bts types switch (bts->type) { case GSM_BTS_TYPE_NANOBTS: @@ -1851,6 +1858,26 @@ static void gsm48_cc_timeout(void *arg)
}
+static inline void disconnect_bridge(struct gsm_network *net, + struct gsm_mncc_bridge *bridge) +{ + struct gsm_trans *trans0 = trans_find_by_callref(net, bridge->callref[0]); + struct gsm_trans *trans1 = trans_find_by_callref(net, bridge->callref[1]); + struct gsm_mncc mx_rel; + if (!trans0 || !trans1) + return; + + memset(&mx_rel, 0, sizeof(struct gsm_mncc)); + mncc_set_cause(&mx_rel, GSM48_CAUSE_LOC_INN_NET, + GSM48_CC_CAUSE_CHAN_UNACCEPT); + + mx_rel.callref = trans0->callref; + gsm48_cc_tx_disconnect(trans0, &mx_rel); + + mx_rel.callref = trans1->callref; + gsm48_cc_tx_disconnect(trans1, &mx_rel); +} + static void gsm48_start_cc_timer(struct gsm_trans *trans, int current, int sec, int micro) { @@ -3220,7 +3247,12 @@ int mncc_tx_to_cc(struct gsm_network *net, int msg_type, void *arg) /* handle special messages */ switch(msg_type) { case MNCC_BRIDGE: - return tch_bridge(net, arg); + rc = tch_bridge(net, arg); + if (rc < 0) { + DEBUGP(DCC, "Failed to bridge TCH: %s\n", strerror(-rc)); + disconnect_bridge(net, arg); + } + return rc; case MNCC_FRAME_DROP: return tch_recv_mncc(net, data->callref, 0); case MNCC_FRAME_RECV:
On 23 Mar 2016, at 19:14, msuraev@sysmocom.de wrote:
From: Max msuraev@sysmocom.de
Note: ideally this situation should not happen - we should check channel compatibility before paging 2nd leg of the call.
- if (lt != rt) {
lt / rt is not declared in this patch (and yes, it belongs into this one. But channel type is still not good enough. Let's assume I have a TCH/H and a TCH/F and I use AMR5.9 on both of these channels. Then the voice codec (and its parameters) are compatible.
I don't know the specific ticket and ultimate goal but I think either you check for codec compatibility or change the wording to refer only to channel type and leave the actual voice codec incompat for another day.
LOGP(DCC, LOGL_ERROR, "Cannot patch through call with different"" channel types: local = %s, remote = %s\n",osmo_gsm48_chan_type2str(lt), osmo_gsm48_chan_type2str(rt));return -EBADSLT;- }
- // todo: map between different bts types switch (bts->type) { case GSM_BTS_TYPE_NANOBTS:
@@ -1851,6 +1858,26 @@ static void gsm48_cc_timeout(void *arg)
}
+static inline void disconnect_bridge(struct gsm_network *net,
struct gsm_mncc_bridge *bridge)+{
- struct gsm_trans *trans0 = trans_find_by_callref(net, bridge->callref[0]);
- struct gsm_trans *trans1 = trans_find_by_callref(net, bridge->callref[1]);
- struct gsm_mncc mx_rel;
- if (!trans0 || !trans1)
return;
Can you please elaborate about the intention of this method? You try to undo a failed mapping and do this by disconnecting the call? Is that the right thing to do? Has there been any side effect by the call of tch_map?
In the long run should there be a MNCC_BRIDGE_REJ answer to the MNCC_BRIDGE call?
@@ -3220,7 +3247,12 @@ int mncc_tx_to_cc(struct gsm_network *net, int msg_type, void *arg) /* handle special messages */ switch(msg_type) { case MNCC_BRIDGE:
return tch_bridge(net, arg);
rc = tch_bridge(net, arg);if (rc < 0) {DEBUGP(DCC, "Failed to bridge TCH: %s\n", strerror(-rc));
Do you think it makes sense to include both callrefs?
holger
Comments are inline.
On 03/23/2016 08:05 PM, Holger Freyther wrote:
Can you please elaborate about the intention of this method? You try to undo a failed mapping and do this by disconnecting the call? Is that the right thing to do? Has there been any side effect by the call of tch_map?
Right now the call with incompatible channels is patched through and we hear broken audio. This method instead disconnects the call for both subscribers as bug suggested.
In the long run should there be a MNCC_BRIDGE_REJ answer to the MNCC_BRIDGE call?
I don't think it's worth changing the protocol - this situation only happens with internal MNCC handler on particular configuration (mixed TCH/F and H) because we do not support transcoding. We can assume that all the external MNCC handlers can hadnle it just fine - otherwise there's no point in using them. We should document that mixing F and H channels is discouraged when no external MNCC handler is available.
On 23 Mar 2016, at 19:14, msuraev@sysmocom.de wrote:
From: Max msuraev@sysmocom.de
Consistently use enums instead of (u)int, char etc. Use bool for boolean types. Add extra debug output with channel mode and type to simplify troubleshooting.
Nack.
* You touch an external protocol that is versioned. Does the size of the structure change? Either way it needs to be in the commit messages.
* You are mixing "add debug messages" and "change types". There is no connection between these two logical changes and they should not be in the same commit.
-
Holger Freyther -
Max -
msuraev＠sysmocom.de