[PATCH 1/3] amr: Remove some code duplication in preparation
From: Holger Hans Peter Freyther <holger@moiji-mobile.com> Merge two copies into a local static helper function. The format of the message will change and then it is easier to modify it in one place than in two. Sadly the original patch was merged before this clean-up so do the clean-up as second step. Conflicts: openbsc/src/libbsc/abis_rsl.c openbsc/src/libbsc/gsm_04_08_utils.c --- openbsc/src/libbsc/abis_rsl.c | 18 +++++++++--------- openbsc/src/libbsc/gsm_04_08_utils.c | 15 +++++++++------ 2 files changed, 18 insertions(+), 15 deletions(-) diff --git a/openbsc/src/libbsc/abis_rsl.c b/openbsc/src/libbsc/abis_rsl.c index 526b977..8e9258c 100644 --- a/openbsc/src/libbsc/abis_rsl.c +++ b/openbsc/src/libbsc/abis_rsl.c @@ -442,6 +442,13 @@ static int channel_mode_from_lchan(struct rsl_ie_chan_mode *cm, return 0; } +static void mr_config_for_bts(struct gsm_lchan *lchan, struct msgb *msg) +{ + if (lchan->tch_mode == GSM48_CMODE_SPEECH_AMR) + msgb_tlv_put(msg, RSL_IE_MR_CONFIG, lchan->mr_bts_lv[0], + lchan->mr_bts_lv + 1); +} + /* Chapter 8.4.1 */ int rsl_chan_activate_lchan(struct gsm_lchan *lchan, uint8_t act_type, uint8_t ho_ref) @@ -518,10 +525,7 @@ int rsl_chan_activate_lchan(struct gsm_lchan *lchan, uint8_t act_type, msgb_tv_put(msg, RSL_IE_BS_POWER, lchan->bs_power); msgb_tv_put(msg, RSL_IE_MS_POWER, lchan->ms_power); msgb_tv_put(msg, RSL_IE_TIMING_ADVANCE, ta); - - if (lchan->tch_mode == GSM48_CMODE_SPEECH_AMR) - msgb_tlv_put(msg, RSL_IE_MR_CONFIG, lchan->mr_bts_lv[0], - lchan->mr_bts_lv + 1); + mr_config_for_bts(lchan, msg); msg->dst = lchan->ts->trx->rsl_link; @@ -557,11 +561,7 @@ int rsl_chan_mode_modify_req(struct gsm_lchan *lchan) msgb_tlv_put(msg, RSL_IE_ENCR_INFO, rc, encr_info); } - if (lchan->tch_mode == GSM48_CMODE_SPEECH_AMR) -{ - msgb_tlv_put(msg, RSL_IE_MR_CONFIG, lchan->mr_bts_lv[0], - lchan->mr_bts_lv + 1); -} + mr_config_for_bts(lchan, msg); msg->dst = lchan->ts->trx->rsl_link; diff --git a/openbsc/src/libbsc/gsm_04_08_utils.c b/openbsc/src/libbsc/gsm_04_08_utils.c index ef82555..24a1cfd 100644 --- a/openbsc/src/libbsc/gsm_04_08_utils.c +++ b/openbsc/src/libbsc/gsm_04_08_utils.c @@ -214,6 +214,13 @@ int get_reason_by_chreq(uint8_t ra, int neci) return GSM_CHREQ_REASON_OTHER; } +static void mr_config_for_ms(struct gsm_lchan *lchan, struct msgb *msg) +{ + if (lchan->tch_mode == GSM48_CMODE_SPEECH_AMR) + msgb_tlv_put(msg, GSM48_IE_MUL_RATE_CFG, lchan->mr_ms_lv[0], + lchan->mr_ms_lv + 1); +} + /* 7.1.7 and 9.1.7: RR CHANnel RELease */ int gsm48_send_rr_release(struct gsm_lchan *lchan) { @@ -489,9 +496,7 @@ int gsm48_send_rr_ass_cmd(struct gsm_lchan *dest_lchan, struct gsm_lchan *lchan, } /* in case of multi rate we need to attach a config */ - if (lchan->tch_mode == GSM48_CMODE_SPEECH_AMR) - msgb_tlv_put(msg, GSM48_IE_MUL_RATE_CFG, lchan->mr_ms_lv[0], - lchan->mr_ms_lv + 1); + mr_config_for_ms(lchan, msg); return gsm48_sendmsg(msg); } @@ -517,9 +522,7 @@ int gsm48_tx_chan_mode_modify(struct gsm_lchan *lchan, uint8_t mode) cmm->mode = mode; /* in case of multi rate we need to attach a config */ - if (lchan->tch_mode == GSM48_CMODE_SPEECH_AMR) - msgb_tlv_put(msg, GSM48_IE_MUL_RATE_CFG, lchan->mr_ms_lv[0], - lchan->mr_ms_lv + 1); + mr_config_for_ms(lchan, msg); return gsm48_sendmsg(msg); } -- 2.3.5
From: Holger Hans Peter Freyther <holger@moiji-mobile.com> This way a lot of if/else can just be killed by the caller deciding which of the two instances to use. I have copied both branches to new files, replace bts for ms in one of them and ran diff on it. There is no difference. --- openbsc/include/openbsc/gsm_04_08.h | 3 ++- openbsc/include/openbsc/gsm_data_shared.h | 10 ++++---- openbsc/src/libbsc/bsc_api.c | 4 +-- openbsc/src/libbsc/bsc_vty.c | 16 ++++++------ openbsc/src/libbsc/gsm_04_08_utils.c | 42 +++++++++++-------------------- 5 files changed, 31 insertions(+), 44 deletions(-) diff --git a/openbsc/include/openbsc/gsm_04_08.h b/openbsc/include/openbsc/gsm_04_08.h index 02b2e3b..02d67f7 100644 --- a/openbsc/include/openbsc/gsm_04_08.h +++ b/openbsc/include/openbsc/gsm_04_08.h @@ -14,6 +14,7 @@ struct gsm_network; struct gsm_trans; struct gsm_subscriber_connection; struct amr_multirate_conf; +struct amr_mode; #define GSM48_ALLOC_SIZE 2048 #define GSM48_ALLOC_HEADROOM 256 @@ -90,6 +91,6 @@ void gsm48_lchan2chan_desc(struct gsm48_chan_desc *cd, void release_security_operation(struct gsm_subscriber_connection *conn); void allocate_security_operation(struct gsm_subscriber_connection *conn); -int gsm48_multirate_config(uint8_t *lv, struct amr_multirate_conf *mr, int ms); +int gsm48_multirate_config(uint8_t *lv, struct amr_multirate_conf *mr, struct amr_mode *modes); #endif diff --git a/openbsc/include/openbsc/gsm_data_shared.h b/openbsc/include/openbsc/gsm_data_shared.h index 5ff6c20..be3333c 100644 --- a/openbsc/include/openbsc/gsm_data_shared.h +++ b/openbsc/include/openbsc/gsm_data_shared.h @@ -150,14 +150,14 @@ struct bts_codec_conf { struct amr_mode { uint8_t mode; - uint8_t threshold_ms; - uint8_t hysteresis_ms; - uint8_t threshold_bts; - uint8_t hysteresis_bts; + uint8_t threshold; + uint8_t hysteresis; }; + struct amr_multirate_conf { uint8_t gsm48_ie[2]; - struct amr_mode mode[4]; + struct amr_mode ms_mode[4]; + struct amr_mode bts_mode[4]; uint8_t num_modes; }; /* /BTS ONLY */ diff --git a/openbsc/src/libbsc/bsc_api.c b/openbsc/src/libbsc/bsc_api.c index 78e7c87..e157eb9 100644 --- a/openbsc/src/libbsc/bsc_api.c +++ b/openbsc/src/libbsc/bsc_api.c @@ -178,8 +178,8 @@ static void handle_mr_config(struct gsm_subscriber_connection *conn, mr_conf->icmi = 1; mr_conf->m5_90 = 1; } - gsm48_multirate_config(lchan->mr_ms_lv, mr, 1); - gsm48_multirate_config(lchan->mr_bts_lv, mr, 0); + gsm48_multirate_config(lchan->mr_ms_lv, mr, mr->ms_mode); + gsm48_multirate_config(lchan->mr_bts_lv, mr, mr->bts_mode); } /* diff --git a/openbsc/src/libbsc/bsc_vty.c b/openbsc/src/libbsc/bsc_vty.c index 9b0f020..d940624 100644 --- a/openbsc/src/libbsc/bsc_vty.c +++ b/openbsc/src/libbsc/bsc_vty.c @@ -505,22 +505,22 @@ static void config_write_bts_amr(struct vty *vty, struct gsm_bts *bts, if (num > 1) { vty_out(vty, " %s threshold ms", prefix); for (i = 0; i < num - 1; i++) { - vty_out(vty, " %d", mr->mode[i].threshold_ms); + vty_out(vty, " %d", mr->ms_mode[i].threshold); } vty_out(vty, "%s", VTY_NEWLINE); vty_out(vty, " %s hysteresis ms", prefix); for (i = 0; i < num - 1; i++) { - vty_out(vty, " %d", mr->mode[i].hysteresis_ms); + vty_out(vty, " %d", mr->ms_mode[i].hysteresis); } vty_out(vty, "%s", VTY_NEWLINE); vty_out(vty, " %s threshold bts", prefix); for (i = 0; i < num - 1; i++) { - vty_out(vty, " %d", mr->mode[i].threshold_bts); + vty_out(vty, " %d", mr->bts_mode[i].threshold); } vty_out(vty, "%s", VTY_NEWLINE); vty_out(vty, " %s hysteresis bts", prefix); for (i = 0; i < num - 1; i++) { - vty_out(vty, " %d", mr->mode[i].hysteresis_bts); + vty_out(vty, " %d", mr->bts_mode[i].hysteresis); } vty_out(vty, "%s", VTY_NEWLINE); } @@ -2957,10 +2957,10 @@ static void get_amr_th_from_arg(struct vty *vty, int argc, const char *argv[], i if (argv[0][0]=='m') { for (i = 0; i < argc - 1; i++) - mr->mode[i].threshold_ms = atoi(argv[i + 1]); + mr->ms_mode[i].threshold = atoi(argv[i + 1]); } else { for (i = 0; i < argc - 1; i++) - mr->mode[i].threshold_bts = atoi(argv[i + 1]); + mr->bts_mode[i].threshold = atoi(argv[i + 1]); } } @@ -2972,10 +2972,10 @@ static void get_amr_hy_from_arg(struct vty *vty, int argc, const char *argv[], i if (argv[0][0]=='m') { for (i = 0; i < argc - 1; i++) - mr->mode[i].hysteresis_ms = atoi(argv[i + 1]); + mr->ms_mode[i].hysteresis = atoi(argv[i + 1]); } else { for (i = 0; i < argc - 1; i++) - mr->mode[i].hysteresis_bts = atoi(argv[i + 1]); + mr->bts_mode[i].hysteresis = atoi(argv[i + 1]); } } diff --git a/openbsc/src/libbsc/gsm_04_08_utils.c b/openbsc/src/libbsc/gsm_04_08_utils.c index 24a1cfd..4901912 100644 --- a/openbsc/src/libbsc/gsm_04_08_utils.c +++ b/openbsc/src/libbsc/gsm_04_08_utils.c @@ -364,7 +364,7 @@ void gsm48_lchan2chan_desc(struct gsm48_chan_desc *cd, } } -int gsm48_multirate_config(uint8_t *lv, struct amr_multirate_conf *mr, int ms) +int gsm48_multirate_config(uint8_t *lv, struct amr_multirate_conf *mr, struct amr_mode *modes) { int num = 0, i; @@ -387,33 +387,19 @@ int gsm48_multirate_config(uint8_t *lv, struct amr_multirate_conf *mr, int ms) memcpy(lv + 1, mr->gsm48_ie, 2); if (num == 1) return 0; - if (ms) { - lv[3] = mr->mode[0].threshold_ms & 0x3f; - lv[4] = mr->mode[0].hysteresis_ms << 4; - if (num == 2) - return 0; - lv[4] |= (mr->mode[1].threshold_ms & 0x3f) >> 2; - lv[5] = mr->mode[1].threshold_ms << 6; - lv[5] |= (mr->mode[1].hysteresis_ms & 0x0f) << 2; - if (num == 3) - return 0; - lv[5] |= (mr->mode[2].threshold_ms & 0x3f) >> 4; - lv[6] = mr->mode[2].threshold_ms << 4; - lv[6] |= mr->mode[2].hysteresis_ms & 0x0f; - } else { - lv[3] = mr->mode[0].threshold_bts & 0x3f; - lv[4] = mr->mode[0].hysteresis_bts << 4; - if (num == 2) - return 0; - lv[4] |= (mr->mode[1].threshold_bts & 0x3f) >> 2; - lv[5] = mr->mode[1].threshold_bts << 6; - lv[5] |= (mr->mode[1].hysteresis_bts & 0x0f) << 2; - if (num == 3) - return 0; - lv[5] |= (mr->mode[2].threshold_bts & 0x3f) >> 4; - lv[6] = mr->mode[2].threshold_bts << 4; - lv[6] |= mr->mode[2].hysteresis_bts & 0x0f; - } + + lv[3] = modes[0].threshold & 0x3f; + lv[4] = modes[0].hysteresis << 4; + if (num == 2) + return 0; + lv[4] |= (modes[1].threshold & 0x3f) >> 2; + lv[5] = modes[1].threshold << 6; + lv[5] |= (modes[1].hysteresis & 0x0f) << 2; + if (num == 3) + return 0; + lv[5] |= (modes[2].threshold & 0x3f) >> 4; + lv[6] = modes[2].threshold << 4; + lv[6] |= modes[2].hysteresis & 0x0f; return 0; } -- 2.3.5
On 24 Sep 2015, at 12:29, Holger Hans Peter Freyther <holger@freyther.de> wrote:
From: Holger Hans Peter Freyther <holger@moiji-mobile.com>
This way a lot of if/else can just be killed by the caller deciding which of the two instances to use.
We should be able to shrink the amount of VTY commands down to one by making all the other modes optional (by adding ‘(‘ ‘)’) around these options. This would reduce the amount of copy and paste for hysteris and threshold too. Any volunteers? holger
From: Holger Hans Peter Freyther <holger@moiji-mobile.com> --- openbsc/src/libbsc/bsc_vty.c | 59 +++++++++++++++++++------------------------- 1 file changed, 25 insertions(+), 34 deletions(-) diff --git a/openbsc/src/libbsc/bsc_vty.c b/openbsc/src/libbsc/bsc_vty.c index d940624..f8ef833 100644 --- a/openbsc/src/libbsc/bsc_vty.c +++ b/openbsc/src/libbsc/bsc_vty.c @@ -479,6 +479,21 @@ static void config_write_bts_model(struct vty *vty, struct gsm_bts *bts) config_write_trx_single(vty, trx); } +static void write_amr_modes(struct vty *vty, const char *prefix, + const char *name, struct amr_mode *modes, int num) +{ + int i; + + vty_out(vty, " %s threshold %s", prefix, name); + for (i = 0; i < num - 1; i++) + vty_out(vty, " %d", modes[i].threshold); + vty_out(vty, "%s", VTY_NEWLINE); + vty_out(vty, " %s hysteresis %s", prefix, name); + for (i = 0; i < num - 1; i++) + vty_out(vty, " %d", modes[i].hysteresis); + vty_out(vty, "%s", VTY_NEWLINE); +} + static void config_write_bts_amr(struct vty *vty, struct gsm_bts *bts, struct amr_multirate_conf *mr, int full) { @@ -503,26 +518,8 @@ static void config_write_bts_amr(struct vty *vty, struct gsm_bts *bts, if (num > 4) num = 4; if (num > 1) { - vty_out(vty, " %s threshold ms", prefix); - for (i = 0; i < num - 1; i++) { - vty_out(vty, " %d", mr->ms_mode[i].threshold); - } - vty_out(vty, "%s", VTY_NEWLINE); - vty_out(vty, " %s hysteresis ms", prefix); - for (i = 0; i < num - 1; i++) { - vty_out(vty, " %d", mr->ms_mode[i].hysteresis); - } - vty_out(vty, "%s", VTY_NEWLINE); - vty_out(vty, " %s threshold bts", prefix); - for (i = 0; i < num - 1; i++) { - vty_out(vty, " %d", mr->bts_mode[i].threshold); - } - vty_out(vty, "%s", VTY_NEWLINE); - vty_out(vty, " %s hysteresis bts", prefix); - for (i = 0; i < num - 1; i++) { - vty_out(vty, " %d", mr->bts_mode[i].hysteresis); - } - vty_out(vty, "%s", VTY_NEWLINE); + write_amr_modes(vty, prefix, "ms", mr->ms_mode, num); + write_amr_modes(vty, prefix, "bts", mr->bts_mode, num); } vty_out(vty, " %s start-mode ", prefix); if (mr_conf->icmi) { @@ -2953,30 +2950,24 @@ static void get_amr_th_from_arg(struct vty *vty, int argc, const char *argv[], i { struct gsm_bts *bts = vty->index; struct amr_multirate_conf *mr = (full) ? &bts->mr_full: &bts->mr_half; + struct amr_mode *modes; int i; - if (argv[0][0]=='m') { - for (i = 0; i < argc - 1; i++) - mr->ms_mode[i].threshold = atoi(argv[i + 1]); - } else { - for (i = 0; i < argc - 1; i++) - mr->bts_mode[i].threshold = atoi(argv[i + 1]); - } + modes = argv[0][0]=='m' ? mr->ms_mode : mr->bts_mode; + for (i = 0; i < argc - 1; i++) + modes[i].threshold = atoi(argv[i + 1]); } static void get_amr_hy_from_arg(struct vty *vty, int argc, const char *argv[], int full) { struct gsm_bts *bts = vty->index; struct amr_multirate_conf *mr = (full) ? &bts->mr_full: &bts->mr_half; + struct amr_mode *modes; int i; - if (argv[0][0]=='m') { - for (i = 0; i < argc - 1; i++) - mr->ms_mode[i].hysteresis = atoi(argv[i + 1]); - } else { - for (i = 0; i < argc - 1; i++) - mr->bts_mode[i].hysteresis = atoi(argv[i + 1]); - } + modes = argv[0][0]=='m' ? mr->ms_mode : mr->bts_mode; + for (i = 0; i < argc - 1; i++) + modes[i].hysteresis = atoi(argv[i + 1]); } static void get_amr_start_from_arg(struct vty *vty, const char *argv[], int full) -- 2.3.5
participants (2)
-
Holger Freyther -
Holger Hans Peter Freyther