diff mbox

Change in openbsc[master]: Fix DTX indicator in SI

Message ID gerrit.1462893551979.I3d55168475ad47044b6238b55846ea22bdd518a4@gerrit.osmocom.org
State New
Headers show

Commit Message

gerrit-no-reply@lists.osmocom.org May 10, 2016, 3:19 p.m. UTC 
From Max <msuraev@sysmocom.de>:

Max has uploaded a new change for review.

  https://gerrit.osmocom.org/40

Change subject: Fix DTX indicator in SI
......................................................................

Fix DTX indicator in SI

Use libsmocore function to properly set DTX value in system information
messages: the value depends on SI type.

Change-Id: I3d55168475ad47044b6238b55846ea22bdd518a4
---
M openbsc/src/libbsc/bsc_init.c
M openbsc/src/libbsc/system_information.c
2 files changed, 17 insertions(+), 6 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/40/40/1

Comments

gerrit-no-reply@lists.osmocom.org May 10, 2016, 7:05 p.m. UTC | #1 
From Harald Welte <laforge@gnumonks.org>:

Harald Welte has posted comments on this change.

Change subject: Fix DTX indicator in SI
......................................................................


Patch Set 1: Code-Review+1

(1 comment)

https://gerrit.osmocom.org/#/c/40/1/openbsc/src/libbsc/system_information.c
File openbsc/src/libbsc/system_information.c:

Line 722: 	if (bts->network->dtx_enabled)
slightly unrelated to this patch, bu still worth addressing in a separate fix:  The DTX configuration should be per BTS and not a global, network-wide setting.  There might be a network-wide default with BTS-local overrides, but that probably gets too complex to understand?
gerrit-no-reply@lists.osmocom.org May 11, 2016, 9:39 a.m. UTC | #2 
From Max <msuraev@sysmocom.de>:

Hello Harald Welte,

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

    https://gerrit.osmocom.org/40

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

Change subject: Fix DTX indicator in SI
......................................................................

Fix DTX indicator in SI

Previously hardcoded values were used for DTX indicator which is error
prone. The DTX was assigned regardless of SI type.

* Use libsmocore function to properly set DTX value in system
  information messages: the value depends on SI type.
* Use flags from gsm_08_58.h for DTX

Change-Id: I3d55168475ad47044b6238b55846ea22bdd518a4
---
M openbsc/src/libbsc/abis_rsl.c
M openbsc/src/libbsc/bsc_init.c
M openbsc/src/libbsc/system_information.c
3 files changed, 18 insertions(+), 7 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/40/40/2
gerrit-no-reply@lists.osmocom.org May 11, 2016, 9:43 a.m. UTC | #3 
From Max <msuraev@sysmocom.de>:

Max has posted comments on this change.

Change subject: Fix DTX indicator in SI
......................................................................


Patch Set 2:

Updated to match changes in libosmocore patch.

As for vty change - I agree that it would be simpler to deprecate old network option (which was never fully supported anyway) and introduce per-bts options instead, preferably - separate for DTXu and DTXd. I'll make separate patch for that.
gerrit-no-reply@lists.osmocom.org May 11, 2016, 10:12 a.m. UTC | #4 
From Harald Welte <laforge@gnumonks.org>:

Harald Welte has posted comments on this change.

Change subject: Fix DTX indicator in SI
......................................................................


Patch Set 2: Code-Review+2
gerrit-no-reply@lists.osmocom.org May 11, 2016, 2:10 p.m. UTC | #5 
From Max <msuraev@sysmocom.de>:

Hello Harald Welte,

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

    https://gerrit.osmocom.org/40

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

Change subject: Move DTX settings to BTS
......................................................................

Move DTX settings to BTS

* Add per-BTS DTX settings
* Configure Uplink and Downlink DTX separately
* Remove global DTX option (it was never tested/used anyway)
* Use libosmocore function for DTX indicator in System
  Information (previously it was incorrectly assigned for half-rate
  channels)

Change-Id: I3d55168475ad47044b6238b55846ea22bdd518a4
---
M openbsc/doc/examples/osmo-bsc/osmo-bsc.cfg
M openbsc/include/openbsc/gsm_data.h
M openbsc/include/openbsc/gsm_data_shared.h
M openbsc/src/libbsc/abis_rsl.c
M openbsc/src/libbsc/bsc_init.c
M openbsc/src/libbsc/bsc_vty.c
M openbsc/src/libbsc/system_information.c
7 files changed, 64 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/40/40/3
gerrit-no-reply@lists.osmocom.org May 11, 2016, 6:27 p.m. UTC | #6 
From Harald Welte <laforge@gnumonks.org>:

Harald Welte has posted comments on this change.

Change subject: Move DTX settings to BTS
......................................................................


Patch Set 3: Code-Review-1 Verified-1

(1 comment)

https://gerrit.osmocom.org/#/c/40/3/openbsc/doc/examples/osmo-bsc/osmo-bsc.cfg
File openbsc/doc/examples/osmo-bsc/osmo-bsc.cfg:

Line 57:   dtxu force
I would prefer something more hierarchical like

dtx uplink force
dtx downlink

And of course the respetive "no dtx ..." commands need to exist for disablign the feature.

Also, why only change osmo-bsc.cfg? What about all the other config file examples in the tree that have 'dtx-used 0'? They would all fail to start.
gerrit-no-reply@lists.osmocom.org May 12, 2016, 2:36 p.m. UTC | #7 
From Harald Welte <laforge@gnumonks.org>:

Harald Welte has posted comments on this change.

Change subject: Move DTX settings to BTS
......................................................................


Patch Set 5: Code-Review+2
gerrit-no-reply@lists.osmocom.org May 12, 2016, 4:57 p.m. UTC | #8 
From Holger Freyther <holger@freyther.de>:

Holger Freyther has posted comments on this change.

Change subject: Move DTX settings to BTS
......................................................................


Patch Set 5: Code-Review-1

(2 comments)

https://gerrit.osmocom.org/#/c/40/5//COMMIT_MSG
Commit Message:

Line 15: 
Is there a reference to a ticket?


https://gerrit.osmocom.org/#/c/40/5/openbsc/src/libbsc/bsc_vty.c
File openbsc/src/libbsc/bsc_vty.c:

PS5, Line 1616: 
Make this deprecated with a warning output and no other implementation. People upgrading should end up with a OpenBSC that continues to start
gerrit-no-reply@lists.osmocom.org May 12, 2016, 5:18 p.m. UTC | #9 
From Max <msuraev@sysmocom.de>:

Max has posted comments on this change.

Change subject: Move DTX settings to BTS
......................................................................


Patch Set 5:

Related ticket is OS#22 but this change is only part of the work. How shall I add this to commit message?
gerrit-no-reply@lists.osmocom.org May 12, 2016, 5:19 p.m. UTC | #10 
From Holger Freyther <holger@freyther.de>:

Holger Freyther has posted comments on this change.

Change subject: Move DTX settings to BTS
......................................................................


Patch Set 5:

(1 comment)

https://gerrit.osmocom.org/#/c/40/5//COMMIT_MSG
Commit Message:

Line 15: 
> Is there a reference to a ticket?
Related: OS#...
gerrit-no-reply@lists.osmocom.org May 12, 2016, 5:48 p.m. UTC | #11 
From Max <msuraev@sysmocom.de>:

Hello Harald Welte, Jenkins Builder, Holger Freyther,

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

    https://gerrit.osmocom.org/40

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

Change subject: Move DTX settings to BTS
......................................................................

Move DTX settings to BTS

* Add per-BTS DTX settings
* Configure Uplink and Downlink DTX separately
* Deprecate global DTX option (it was never tested/used anyway)
* Use libosmocore function for DTX indicator in System
  Information (previously it was incorrectly assigned for half-rate
  channels)

Related: OS#22
Change-Id: I3d55168475ad47044b6238b55846ea22bdd518a4
---
M openbsc/doc/examples/osmo-bsc/osmo-bsc.cfg
M openbsc/doc/examples/osmo-nitb/rbs2308/openbsc.cfg
M openbsc/include/openbsc/gsm_data.h
M openbsc/include/openbsc/gsm_data_shared.h
M openbsc/src/libbsc/abis_rsl.c
M openbsc/src/libbsc/bsc_init.c
M openbsc/src/libbsc/bsc_vty.c
M openbsc/src/libbsc/system_information.c
8 files changed, 95 insertions(+), 20 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/40/40/6
gerrit-no-reply@lists.osmocom.org May 12, 2016, 6:01 p.m. UTC | #12 
From Harald Welte <laforge@gnumonks.org>:

Harald Welte has posted comments on this change.

Change subject: Move DTX settings to BTS
......................................................................


Patch Set 6: Code-Review+2
gerrit-no-reply@lists.osmocom.org May 12, 2016, 6:14 p.m. UTC | #13 
From Holger Freyther <holger@freyther.de>:

Holger Freyther has posted comments on this change.

Change subject: Move DTX settings to BTS
......................................................................


Patch Set 6: Code-Review-1

(1 comment)

https://gerrit.osmocom.org/#/c/40/6/openbsc/src/libbsc/bsc_vty.c
File openbsc/src/libbsc/bsc_vty.c:

Line 1638:        return CMD_SUCCESS;
DEFUN_DEPRECATED... and maybe return CMD_WARNING but the later doe snot make a difference. By convention our warnings include %% as well
gerrit-no-reply@lists.osmocom.org May 13, 2016, 8:10 a.m. UTC | #14 
From Max <msuraev@sysmocom.de>:

Hello Harald Welte, Jenkins Builder, Holger Freyther,

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

    https://gerrit.osmocom.org/40

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

Change subject: Move DTX settings to BTS
......................................................................

Move DTX settings to BTS

* Add per-BTS DTX settings
* Configure Uplink and Downlink DTX separately
* Deprecate global DTX option (it was never tested/used anyway)
* Use libosmocore function for DTX indicator in System
  Information (previously it was incorrectly assigned for half-rate
  channels)

Related: OS#22
Change-Id: I3d55168475ad47044b6238b55846ea22bdd518a4
---
M openbsc/doc/examples/osmo-bsc/osmo-bsc.cfg
M openbsc/doc/examples/osmo-nitb/rbs2308/openbsc.cfg
M openbsc/include/openbsc/gsm_data.h
M openbsc/include/openbsc/gsm_data_shared.h
M openbsc/src/libbsc/abis_rsl.c
M openbsc/src/libbsc/bsc_init.c
M openbsc/src/libbsc/bsc_vty.c
M openbsc/src/libbsc/system_information.c
8 files changed, 99 insertions(+), 25 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/40/40/7
gerrit-no-reply@lists.osmocom.org May 13, 2016, 8:35 a.m. UTC | #15 
From Holger Freyther <holger@freyther.de>:

Holger Freyther has posted comments on this change.

Change subject: Move DTX settings to BTS
......................................................................


Patch Set 7: Code-Review+2

(1 comment)

Looks good now. Saying +2 because harald said it before and my feedback has been addresses.

https://gerrit.osmocom.org/#/c/40/7/openbsc/src/libbsc/bsc_vty.c
File openbsc/src/libbsc/bsc_vty.c:

Line 1633: 		 ".HIDDEN\n""Obsolete\n""Obsolete\n")
Ah interesting. I didn't know .HIDDEN
gerrit-no-reply@lists.osmocom.org May 13, 2016, 3:59 p.m. UTC | #16 
From Max <msuraev@sysmocom.de>:

Hello Harald Welte, Jenkins Builder, Holger Freyther,

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

    https://gerrit.osmocom.org/40

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

Change subject: Move DTX settings to BTS
......................................................................

Move DTX settings to BTS

* Add per-BTS DTX settings
* Configure Uplink and Downlink DTX separately
* Deprecate global DTX option (it was never tested/used anyway)
* Use libosmocore function for DTX indicator in System
  Information (previously it was incorrectly assigned for half-rate
  channels)

Related: OS#22
Change-Id: I3d55168475ad47044b6238b55846ea22bdd518a4
---
M openbsc/doc/examples/osmo-bsc/osmo-bsc.cfg
M openbsc/doc/examples/osmo-nitb/rbs2308/openbsc.cfg
M openbsc/include/openbsc/gsm_data.h
M openbsc/include/openbsc/gsm_data_shared.h
M openbsc/src/libbsc/abis_rsl.c
M openbsc/src/libbsc/bsc_init.c
M openbsc/src/libbsc/bsc_vty.c
M openbsc/src/libbsc/system_information.c
M openbsc/src/libcommon/gsm_data.c
9 files changed, 98 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/openbsc refs/changes/40/40/8
gerrit-no-reply@lists.osmocom.org May 13, 2016, 4 p.m. UTC | #17 
From Max <msuraev@sysmocom.de>:

Max has posted comments on this change.

Change subject: Move DTX settings to BTS
......................................................................


Patch Set 8:

Moved BTS DTX init to proper place.
gerrit-no-reply@lists.osmocom.org May 17, 2016, 4:17 p.m. UTC | #18 
From Holger Freyther <holger@freyther.de>:

Holger Freyther has posted comments on this change.

Change subject: Move DTX settings to BTS
......................................................................


Patch Set 8: Code-Review+2

(1 comment)

https://gerrit.osmocom.org/#/c/40/8/openbsc/doc/examples/osmo-bsc/osmo-bsc.cfg
File openbsc/doc/examples/osmo-bsc/osmo-bsc.cfg:

Line 58:   dtx downlink
It was off before and is now enabled in both directions?
diff mbox

Patch

diff --git a/openbsc/src/libbsc/bsc_init.c b/openbsc/src/libbsc/bsc_init.c
index fea6562..5c27862 100644
--- a/openbsc/src/libbsc/bsc_init.c
+++ b/openbsc/src/libbsc/bsc_init.c
@@ -458,12 +458,6 @@ 
 		return -EINVAL;
 	}
 
-	/* allow/disallow DTXu */
-	if (bts->network->dtx_enabled)
-		bts->si_common.cell_options.dtx = 0;
-	else
-		bts->si_common.cell_options.dtx = 2;
-
 	bts->si_common.cell_options.pwrc = 0; /* PWRC not set */
 
 	bts->si_common.cell_sel_par.acs = 0;
diff --git a/openbsc/src/libbsc/system_information.c b/openbsc/src/libbsc/system_information.c
index 0d96621..af3b07d 100644
--- a/openbsc/src/libbsc/system_information.c
+++ b/openbsc/src/libbsc/system_information.c
@@ -30,6 +30,7 @@ 
 #include <osmocom/core/bitvec.h>
 #include <osmocom/core/utils.h>
 #include <osmocom/gsm/sysinfo.h>
+#include <osmocom/gsm/protocol/gsm_04_08.h>
 
 #include <openbsc/debug.h>
 #include <openbsc/gsm_04_08.h>
@@ -717,6 +718,14 @@ 
 	si3->cell_sel_par = bts->si_common.cell_sel_par;
 	si3->rach_control = bts->si_common.rach_control;
 
+	/* allow/disallow DTXu */
+	if (bts->network->dtx_enabled)
+		gsm48_set_dtx(&si3->cell_options, GSM48_MAY_USE, GSM48_MAY_USE,
+			      true);
+	else
+		gsm48_set_dtx(&si3->cell_options, GSM48_SHALL_NOT,
+			      GSM48_SHALL_NOT, true);
+
 	if ((bts->si_valid & (1 << SYSINFO_TYPE_2ter))) {
 		LOGP(DRR, LOGL_INFO, "SI 2ter is included.\n");
 		si_info.si2ter_indicator = 1;
@@ -929,6 +938,14 @@ 
 	si6->cell_options = bts->si_common.cell_options;
 	si6->ncc_permitted = bts->si_common.ncc_permitted;
 
+	/* allow/disallow DTXu */
+	if (bts->network->dtx_enabled)
+		gsm48_set_dtx(&si6->cell_options, GSM48_MAY_USE, GSM48_MAY_USE,
+			      false);
+	else
+		gsm48_set_dtx(&si6->cell_options, GSM48_SHALL_NOT,
+			      GSM48_SHALL_NOT, false);
+
 	/* SI6 Rest Octets: 10.5.2.35a: PCH / NCH info, VBS/VGCS options */
 
 	return l2_plen;