Change in osmo-bts[master]: measurement: expect at least 1 SUB frame for AMR
laforge has submitted this change. (
https://gerrit.osmocom.org/c/osmo-bts/+/17929 )
Change subject: measurement: expect at least 1 SUB frame for AMR
..
measurement: expect at least 1 SUB frame for AMR
The amount of SUB frames that may occur in AMR is not fixed, nor can it
be determined by some formular or lookup table. The reason for this is
that the DTX period may negotiated dynamicly. This means that the lower
layers must make the decision if an AMR sub frame is a SUB frame early
and tag the repective measurement / frame they hand up to the upper
layers accordingly. However, regardles of the occurrence of DTX periods
the amount of SUB frames in AMR must be always 1 or more because SACCH
frames always count as SUB frames as well. Lets make sure that this is
respected in the debug log as well as in the logic that tries to
substitue missing SUB frame measuremnets.
Change-Id: I1fd91b576ff7274caa6d4356bcd7a4fa4311219d
Related: OS#4465
---
M src/common/measurement.c
1 file changed, 48 insertions(+), 19 deletions(-)
Approvals:
laforge: Looks good to me, but someone else must approve
pespin: Looks good to me, approved
Jenkins Builder: Verified
diff --git a/src/common/measurement.c b/src/common/measurement.c
index 3ee9ffe..3170e4a 100644
--- a/src/common/measurement.c
+++ b/src/common/measurement.c
@@ -571,10 +571,11 @@
if (lchan->tch_mode != GSM48_CMODE_SPEECH_AMR)
num_meas_sub_expect = lchan_meas_sub_num_expected(lchan);
else {
- /* FIXME: the amount of SUB Measurements is a dynamic parameter
-* in AMR and can not be determined by using a lookup table.
-* See also: OS#2978 */
- num_meas_sub_expect = 0;
+ /* When AMR is used, we expect at least one SUB frame, since
+* the SACCH will always be SUB frame. There may occur more
+* SUB frames but since DTX periods in AMR are dynamic, we
+* can not know how much exactly. */
+ num_meas_sub_expect = 1;
}
if (lchan->meas.num_ul_meas > num_ul_meas_expect)
@@ -618,9 +619,16 @@
num_ul_meas_actual++;
} else {
m = &measurement_dummy;
- if (num_ul_meas_expect - i <= num_meas_sub_expect -
num_meas_sub) {
- num_meas_sub_subst++;
- is_sub = true;
+
+ /* For AMR the amount of SUB frames is defined by the
+* the occurrence of DTX periods, which are dynamically
+* negotiated in AMR, so we can not know if and how many
+* SUB frames are missing. */
+ if (lchan->tch_mode != GSM48_CMODE_SPEECH_AMR) {
+ if (num_ul_meas_expect - i <=
num_meas_sub_expect - num_meas_sub) {
+ num_meas_sub_subst++;
+ is_sub = true;
+ }
}
num_ul_meas_subst++;
@@ -633,21 +641,42 @@
}
}
- LOGP(DMEAS, LOGL_DEBUG, "%s received UL measurements contain %u SUB
measurements, expected %u\n",
-gsm_lchan_name(lchan), num_meas_sub_actual, num_meas_sub_expect);
+ if (lchan->tch_mode != GSM48_CMODE_SPEECH_AMR) {
+ LOGP(DMEAS, LOGL_DEBUG,
+"%s received UL measurements contain %u SUB measurements,
expected %u\n",
+gsm_lchan_name(lchan), num_meas_sub_actual,
+num_meas_sub_expect);
+ } else {
+ LOGP(DMEAS, LOGL_DEBUG,
+"%s received UL measurements contain %u SUB measurements,
expected at least %u\n",
+gsm_lchan_name(lchan), num_meas_sub_actual,
+num_meas_sub_expect);
+ }
+
LOGP(DMEAS, LOGL_DEBUG, "%s replaced %u measurements with dummy values,
from which %u were SUB measurements\n",
gsm_lchan_name(lchan), num_ul_meas_subst, num_meas_sub_subst);
- if (num_meas_sub != num_meas_sub_expect) {
- LOGP(DMEAS, LOGL_ERROR, "%s Incorrect number of SUB
measurements detected! (%u vs exp %u)\n",
-gsm_lchan_name(lchan), num_meas_sub, num_meas_sub_expect);
- /* Normally the logic above should make sure that there is
-* always the exact amount of SUB measurements taken into
-* account. If not then the logic that decides tags the received
-* measurements as is_sub works incorrectly. Since the logic
-* above only adds missing measurements during the calculation
-* it can not remove excess SUB measurements or add missing SUB
-* measurements when there is no more
Change in osmo-bts[master]: measurement: expect at least 1 SUB frame for AMR
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/17929 ) Change subject: measurement: expect at least 1 SUB frame for AMR .. Patch Set 6: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/17929 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I1fd91b576ff7274caa6d4356bcd7a4fa4311219d Gerrit-Change-Number: 17929 Gerrit-PatchSet: 6 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: dexter Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 13 May 2020 11:48:49 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in osmo-bts[master]: measurement: expect at least 1 SUB frame for AMR
dexter has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-bts/+/17929 )
Change subject: measurement: expect at least 1 SUB frame for AMR
..
Patch Set 6:
(3 comments)
(forcing gerrit to send stuck review comments)
https://gerrit.osmocom.org/c/osmo-bts/+/17929/1/src/common/measurement.c
File src/common/measurement.c:
https://gerrit.osmocom.org/c/osmo-bts/+/17929/1/src/common/measurement.c@623
PS1, Line 623: * negotiated in AMD, so we can not know if and
how many
> AMD? you mean AMR
Done
https://gerrit.osmocom.org/c/osmo-bts/+/17929/1/src/common/measurement.c@627
PS1, Line 627: num_meas_sub_expect - num_meas_sub)
{
> better keep this in one line, it's more readable.
Done
https://gerrit.osmocom.org/c/osmo-bts/+/17929/1/src/common/measurement.c@665
PS1, Line 665: if (lchan->tch_mode != GSM48_CMODE_SPEECH_AMR) {
> Can we move all these checks together with the ones above in line 643 please?
(I am sorry, gerrit did not send this for some reason in the previous patchset)
I have checked this. I don't think that this is a good idea. What I wanted to
express in the log here is something like this:
1. Thats what we received ...
2. Thats what we have replaced ...
3. Error! Looks like we couldn't make it fix.
Unfortunately 2 is inbetween and it makes less sense if I move it to the
outside. Just add another comment if you still want to have that changed.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/17929
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I1fd91b576ff7274caa6d4356bcd7a4fa4311219d
Gerrit-Change-Number: 17929
Gerrit-PatchSet: 6
Gerrit-Owner: dexter
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter
Gerrit-Reviewer: laforge
Gerrit-Reviewer: pespin
Gerrit-Comment-Date: Wed, 13 May 2020 10:48:07 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin
Gerrit-MessageType: comment
Change in osmo-bts[master]: measurement: expect at least 1 SUB frame for AMR
laforge has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-bts/+/17929 )
Change subject: measurement: expect at least 1 SUB frame for AMR
..
Patch Set 5:
(1 comment)
https://gerrit.osmocom.org/c/osmo-bts/+/17929/1/src/common/measurement.c
File src/common/measurement.c:
https://gerrit.osmocom.org/c/osmo-bts/+/17929/1/src/common/measurement.c@665
PS1, Line 665: if (lchan->tch_mode != GSM48_CMODE_SPEECH_AMR) {
> This comment has not yet been addressesd
It again has not been addressed?
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/17929
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I1fd91b576ff7274caa6d4356bcd7a4fa4311219d
Gerrit-Change-Number: 17929
Gerrit-PatchSet: 5
Gerrit-Owner: dexter
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge
Gerrit-Reviewer: pespin
Gerrit-Comment-Date: Tue, 12 May 2020 18:01:08 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin
Gerrit-MessageType: comment
Change in osmo-bts[master]: measurement: expect at least 1 SUB frame for AMR
pespin has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-bts/+/17929 )
Change subject: measurement: expect at least 1 SUB frame for AMR
..
Patch Set 4:
(1 comment)
https://gerrit.osmocom.org/c/osmo-bts/+/17929/1/src/common/measurement.c
File src/common/measurement.c:
https://gerrit.osmocom.org/c/osmo-bts/+/17929/1/src/common/measurement.c@665
PS1, Line 665: if (lchan->tch_mode != GSM48_CMODE_SPEECH_AMR) {
> Can we move all these checks together with the ones above in line 643 please?
This comment has not yet been addressesd
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/17929
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I1fd91b576ff7274caa6d4356bcd7a4fa4311219d
Gerrit-Change-Number: 17929
Gerrit-PatchSet: 4
Gerrit-Owner: dexter
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge
Gerrit-Reviewer: pespin
Gerrit-Comment-Date: Sun, 26 Apr 2020 15:48:37 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin
Gerrit-MessageType: comment
Change in osmo-bts[master]: measurement: expect at least 1 SUB frame for AMR
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/17929 ) Change subject: measurement: expect at least 1 SUB frame for AMR .. Patch Set 4: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/17929 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I1fd91b576ff7274caa6d4356bcd7a4fa4311219d Gerrit-Change-Number: 17929 Gerrit-PatchSet: 4 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-Comment-Date: Sun, 26 Apr 2020 14:46:11 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in osmo-bts[master]: measurement: expect at least 1 SUB frame for AMR
Hello pespin, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/osmo-bts/+/17929 to look at the new patch set (#3). Change subject: measurement: expect at least 1 SUB frame for AMR .. measurement: expect at least 1 SUB frame for AMR The amount of SUB frames that may occur in AMR is not fixed, nor can it be determined by some formular or lookup table. The reason for this is that the DTX period may negotiated dynamicly. This means that the lower layers must make the decision if an AMR sub frame is a SUB frame early and tag the repective measurement / frame they hand up to the upper layers accordingly. However, regardles of the occurrence of DTX periods the amount of SUB frames in AMR must be always 1 or more because SACCH frames always count as SUB frames as well. Lets make sure that this is respected in the debug log as well as in the logic that tries to substitue missing SUB frame measuremnets. Change-Id: I1fd91b576ff7274caa6d4356bcd7a4fa4311219d Related: OS#4465 --- M src/common/measurement.c 1 file changed, 48 insertions(+), 19 deletions(-) git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/29/17929/3 -- To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/17929 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Change-Id: I1fd91b576ff7274caa6d4356bcd7a4fa4311219d Gerrit-Change-Number: 17929 Gerrit-PatchSet: 3 Gerrit-Owner: dexter Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin Gerrit-MessageType: newpatchset
Change in osmo-bts[master]: measurement: expect at least 1 SUB frame for AMR
pespin has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-bts/+/17929 )
Change subject: measurement: expect at least 1 SUB frame for AMR
..
Patch Set 1: Code-Review-1
(3 comments)
https://gerrit.osmocom.org/c/osmo-bts/+/17929/1/src/common/measurement.c
File src/common/measurement.c:
https://gerrit.osmocom.org/c/osmo-bts/+/17929/1/src/common/measurement.c@623
PS1, Line 623: * negotiated in AMD, so we can not know if and
how many
AMD? you mean AMR
https://gerrit.osmocom.org/c/osmo-bts/+/17929/1/src/common/measurement.c@627
PS1, Line 627: num_meas_sub_expect - num_meas_sub)
{
better keep this in one line, it's more readable.
https://gerrit.osmocom.org/c/osmo-bts/+/17929/1/src/common/measurement.c@665
PS1, Line 665: if (lchan->tch_mode != GSM48_CMODE_SPEECH_AMR) {
Can we move all these checks together with the ones above in line 643 please?
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/17929
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I1fd91b576ff7274caa6d4356bcd7a4fa4311219d
Gerrit-Change-Number: 17929
Gerrit-PatchSet: 1
Gerrit-Owner: dexter
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin
Gerrit-Comment-Date: Wed, 22 Apr 2020 15:17:49 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Change in osmo-bts[master]: measurement: expect at least 1 SUB frame for AMR
dexter has uploaded this change for review. (
https://gerrit.osmocom.org/c/osmo-bts/+/17929 )
Change subject: measurement: expect at least 1 SUB frame for AMR
..
measurement: expect at least 1 SUB frame for AMR
The amount of SUB frames that may occur in AMR is not fixed, nor can it
be determined by some formular or lookup table. The reason for this is
that the DTX period may negotiated dynamicly. This means that the lower
layers must make the decision if an AMR sub frame is a SUB frame early
and tag the repective measurement / frame they hand up to the upper
layers accordingly. However, regardles of the occurrence of DTX periods
the amount of SUB frames in AMR must be always 1 or more because SACCH
frames always count as SUB frames as well. Lets make sure that this is
respected in the debug log as well as in the logic that tries to
substitue missing SUB frame measuremnets.
Change-Id: I1fd91b576ff7274caa6d4356bcd7a4fa4311219d
Related: OS#4465
---
M src/common/measurement.c
1 file changed, 49 insertions(+), 19 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/29/17929/1
diff --git a/src/common/measurement.c b/src/common/measurement.c
index c5cbbb0..4ea820a 100644
--- a/src/common/measurement.c
+++ b/src/common/measurement.c
@@ -569,10 +569,11 @@
if (lchan->tch_mode != GSM48_CMODE_SPEECH_AMR)
num_meas_sub_expect = lchan_meas_sub_num_expected(lchan);
else {
- /* FIXME: the amount of SUB Measurements is a dynamic parameter
-* in AMR and can not be determined by using a lookup table.
-* See also: OS#2978 */
- num_meas_sub_expect = 0;
+ /* When AMR is used, we expect at least one SUB frame, since
+* the SACCH will always be SUB frame. There may occur more
+* SUB frames but since DTX periods in AMR are dynamic, we
+* can not know how much exactly. */
+ num_meas_sub_expect = 1;
}
if (lchan->meas.num_ul_meas > num_ul_meas_expect)
@@ -616,9 +617,17 @@
num_ul_meas_actual++;
} else {
m = &measurement_dummy;
- if (num_ul_meas_expect - i <= num_meas_sub_expect -
num_meas_sub) {
- num_meas_sub_subst++;
- is_sub = true;
+
+ /* For AMR the amount of SUB frames is defined by the
+* the occurrence of DTX periods, which are dynamically
+* negotiated in AMD, so we can not know if and how many
+* SUB frames are missing. */
+ if (lchan->tch_mode != GSM48_CMODE_SPEECH_AMR) {
+ if (num_ul_meas_expect - i <=
+ num_meas_sub_expect - num_meas_sub) {
+ num_meas_sub_subst++;
+ is_sub = true;
+ }
}
num_ul_meas_subst++;
@@ -631,21 +640,42 @@
}
}
- LOGP(DMEAS, LOGL_DEBUG, "%s received UL measurements contain %u SUB
measurements, expected %u\n",
-gsm_lchan_name(lchan), num_meas_sub_actual, num_meas_sub_expect);
+ if (lchan->tch_mode != GSM48_CMODE_SPEECH_AMR) {
+ LOGP(DMEAS, LOGL_DEBUG,
+"%s received UL measurements contain %u SUB measurements,
expected %u\n",
+gsm_lchan_name(lchan), num_meas_sub_actual,
+num_meas_sub_expect);
+ } else {
+ LOGP(DMEAS, LOGL_DEBUG,
+"%s received UL measurements contain %u SUB measurements,
expected at least %u\n",
+gsm_lchan_name(lchan), num_meas_sub_actual,
+num_meas_sub_expect);
+ }
+
LOGP(DMEAS, LOGL_DEBUG, "%s replaced %u measurements with dummy values,
from which %u were SUB measurements\n",
gsm_lchan_name(lchan), num_ul_meas_subst, num_meas_sub_subst);
- if (num_meas_sub != num_meas_sub_expect) {
- LOGP(DMEAS, LOGL_ERROR, "%s Incorrect number of SUB
measurements detected! (%u vs exp %u)\n",
-gsm_lchan_name(lchan), num_meas_sub, num_meas_sub_expect);
- /* Normally the logic above should make sure that there is
-* always the exact amount of SUB measurements taken into
-* account. If not then the logic that decides tags the received
-* measurements as is_sub works incorrectly. Since the logic
-* above only adds missing measurements during the calculation
-* it can not remove excess SUB measurements or add missing SUB
-* measurements when there is no more room in the inte
