Change in osmo-pcu[master]: Drop unneeded arg 'ta' in tbf_alloc_ul()

2020-05-14 Thread pespin 
pespin has submitted this change. ( 
https://gerrit.osmocom.org/c/osmo-pcu/+/18224 )

Change subject: Drop unneeded arg 'ta' in tbf_alloc_ul()
..

Drop unneeded arg 'ta' in tbf_alloc_ul()

The function is simply setting the ta on the ms, so simply make sure ta
is set on callers before passing the ms object.

Change-Id: Iebb9c57f458690e045ddc45c800209ad8cf621e0
---
M src/pdch.cpp
M src/tbf.cpp
M src/tbf.h
M tests/tbf/TbfTest.err
4 files changed, 16 insertions(+), 20 deletions(-)

Approvals:
  laforge: Looks good to me, but someone else must approve
  fixeria: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/src/pdch.cpp b/src/pdch.cpp
index 048577d..88d5d31 100644
--- a/src/pdch.cpp
+++ b/src/pdch.cpp
@@ -118,7 +118,7 @@
bts->channel_request_description();

/* This call will register the new TBF with the MS on success */
-   gprs_rlcmac_ul_tbf *ul_tbf = tbf_alloc_ul(bts_data, tbf->ms(), 
tbf->trx->trx_no, tbf->tlli(), tbf->ta());
+   gprs_rlcmac_ul_tbf *ul_tbf = tbf_alloc_ul(bts_data, tbf->ms(), 
tbf->trx->trx_no, tbf->tlli());

/* schedule uplink assignment or reject */
if (ul_tbf) {
@@ -551,7 +551,6 @@
struct gprs_rlcmac_ul_tbf *ul_tbf;
struct gprs_rlcmac_dl_tbf *dl_tbf;
uint32_t tlli = request->ID.u.TLLI;
-   uint8_t ta = GSM48_TA_INVALID;
bool found = true;

GprsMs *ms = bts()->ms_by_tlli(tlli);
@@ -566,7 +565,6 @@
if (found) {
ul_tbf = ms->ul_tbf();
dl_tbf = ms->dl_tbf();
-   ta = ms->ta();
/* We got a RACH so the MS was in packet idle mode and 
thus
 * didn't have any active TBFs */
if (ul_tbf) {
@@ -593,7 +591,7 @@
"block, but there is no resource request "
"scheduled!\n");
} else {
-   ta = sba->ta;
+   ms->set_ta(sba->ta);
bts()->sba()->free_sba(sba);
}
if (request->Exist_MS_Radio_Access_capability2) {
@@ -613,7 +611,7 @@
"MS supports EGPRS multislot class %d.\n",
ms->egprs_ms_class());

-   ul_tbf = tbf_alloc_ul(bts_data(), ms, trx_no(), tlli, ta);
+   ul_tbf = tbf_alloc_ul(bts_data(), ms, trx_no(), tlli);
if (!ul_tbf) {
handle_tbf_reject(bts_data(), ms, tlli,
trx_no(), ts_no);
diff --git a/src/tbf.cpp b/src/tbf.cpp
index 56fdcd1..2693223 100644
--- a/src/tbf.cpp
+++ b/src/tbf.cpp
@@ -416,7 +416,7 @@
 }

 gprs_rlcmac_ul_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts, GprsMs *ms, 
int8_t use_trx,
-uint32_t tlli, uint8_t ta)
+uint32_t tlli)
 {
struct gprs_rlcmac_ul_tbf *tbf;

@@ -435,8 +435,6 @@
tbf->update_ms(tlli, GPRS_RLCMAC_UL_TBF);
OSMO_ASSERT(tbf->ms());

-   tbf->ms()->set_ta(ta);
-
return tbf;
 }

diff --git a/src/tbf.h b/src/tbf.h
index 5ebd2e8..2b4cf6d 100644
--- a/src/tbf.h
+++ b/src/tbf.h
@@ -347,7 +347,7 @@


 struct gprs_rlcmac_ul_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts, GprsMs 
*ms,
-   int8_t use_trx, uint32_t tlli, uint8_t 
ta);
+   int8_t use_trx, uint32_t tlli);

 struct gprs_rlcmac_ul_tbf *tbf_alloc_ul_tbf(struct gprs_rlcmac_bts *bts, 
GprsMs *ms, int8_t use_trx, bool single_slot);

diff --git a/tests/tbf/TbfTest.err b/tests/tbf/TbfTest.err
index 6019def..ca16149 100644
--- a/tests/tbf/TbfTest.err
+++ b/tests/tbf/TbfTest.err
@@ -1526,6 +1526,7 @@
 - RX : Uplink Control Block -
 Creating MS object, TLLI = 0x
 MS requests UL TBF in packet resource request of single block, so we provide 
one:
+Modifying MS object, TLLI = 0x, TA 220 -> 7
 Modifying MS object, TLLI = 0x, MS class 0 -> 1
 ** UL-TBF starts here **
 Allocating UL TBF: MS_CLASS=1/0
@@ -1545,7 +1546,6 @@
 TBF(TFI=0 TLLI=0x DIR=UL STATE=NULL) changes state from NULL to ASSIGN
 TBF(TFI=0 TLLI=0x DIR=UL STATE=ASSIGN) starting timer T3169 
[allocation (UL-TBF)] with 5 sec. 0 microsec, cur_fn=0
 Modifying MS object, UL TLLI: 0x -> 0xf1223344, not yet confirmed
-Modifying MS object, TLLI = 0xf1223344, TA 220 -> 7
 TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=ASSIGN) change control TS 7 -> 7 until 
assignment is complete.
 TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=ASSIGN) changes UL ASS state from 
GPRS_RLCMAC_UL_ASS_NONE to GPRS_RLCMAC_UL_ASS_SEND_ASS
 TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=ASSIGN) start Packet Uplink Assignment 
(PACCH)
@@ -1608,6

Change in osmo-pcu[master]: Drop unneeded arg 'ta' in tbf_alloc_ul()

2020-05-14 Thread fixeria 
fixeria has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-pcu/+/18224 )

Change subject: Drop unneeded arg 'ta' in tbf_alloc_ul()
..


Patch Set 1: Code-Review+2


--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/18224
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: Iebb9c57f458690e045ddc45c800209ad8cf621e0
Gerrit-Change-Number: 18224
Gerrit-PatchSet: 1
Gerrit-Owner: pespin 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria 
Gerrit-Reviewer: laforge 
Gerrit-Reviewer: pespin 
Gerrit-CC: neels 
Gerrit-Comment-Date: Thu, 14 May 2020 11:16:58 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Change in osmo-pcu[master]: Drop unneeded arg 'ta' in tbf_alloc_ul()

2020-05-13 Thread pespin 
pespin has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-pcu/+/18224 )

Change subject: Drop unneeded arg 'ta' in tbf_alloc_ul()
..


Patch Set 1:

> Patch Set 1:
>
> Requiring a ta argument can be a good way to make sure callers never forget 
> to set the ta.
> For sched_ul_ass_or_rej(), can there ever be a situation where ta was 
> forgotten to set in the ms? IIUC this isn't a functional change. API design 
> wise I tend to prefer the "safer" API that ensures ta is never omitted...?
>
> I'm not sure why you spent time to write this patch, but to not drag out a 
> bikeshed: if you insist that this must happen, and if you are certain that ta 
> is never forgotten, consider this a +1 from me. If you're unsure then why not 
> just leave this API as it is...

The data structure architecture in osmo-pcu is totally fucked up, with classes 
totally entangled one with another, and these patches clearly show what I'm 
saying. I'm trying to split stuff consistently so we can finally make some 
sense out of this code.

It makes no sense to pass a TA value (even invalid one) to a TBF function which 
then sets that MS attribute to the MS object.

If the TA is forgotten to be set at any time, it contains the same INVALID 
default value, so there's no change.

So just to be really clear here: I don't think I'm losing my time, and I think 
all these patches are needed to be able to finally have some code which we can 
extend and understand properly. Be sure that a lot more of these patches will 
come over time.


--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/18224
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: Iebb9c57f458690e045ddc45c800209ad8cf621e0
Gerrit-Change-Number: 18224
Gerrit-PatchSet: 1
Gerrit-Owner: pespin 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge 
Gerrit-Reviewer: pespin 
Gerrit-CC: neels 
Gerrit-Comment-Date: Wed, 13 May 2020 13:21:36 +
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in osmo-pcu[master]: Drop unneeded arg 'ta' in tbf_alloc_ul()

2020-05-13 Thread neels 
neels has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-pcu/+/18224 )

Change subject: Drop unneeded arg 'ta' in tbf_alloc_ul()
..


Patch Set 1:

Requiring a ta argument can be a good way to make sure callers never forget to 
set the ta.
For sched_ul_ass_or_rej(), can there ever be a situation where ta was forgotten 
to set in the ms? IIUC this isn't a functional change. API design wise I tend 
to prefer the "safer" API that ensures ta is never omitted...?

I'm not sure why you spent time to write this patch, but to not drag out a 
bikeshed: if you insist that this must happen, and if you are certain that ta 
is never forgotten, consider this a +1 from me. If you're unsure then why not 
just leave this API as it is...


--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/18224
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: Iebb9c57f458690e045ddc45c800209ad8cf621e0
Gerrit-Change-Number: 18224
Gerrit-PatchSet: 1
Gerrit-Owner: pespin 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge 
Gerrit-CC: neels 
Gerrit-Comment-Date: Wed, 13 May 2020 12:42:25 +
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment

Change in osmo-pcu[master]: Drop unneeded arg 'ta' in tbf_alloc_ul()

2020-05-12 Thread laforge 
laforge has posted comments on this change. ( 
https://gerrit.osmocom.org/c/osmo-pcu/+/18224 )

Change subject: Drop unneeded arg 'ta' in tbf_alloc_ul()
..


Patch Set 1: Code-Review+1


--
To view, visit https://gerrit.osmocom.org/c/osmo-pcu/+/18224
To unsubscribe, or for help writing mail filters, visit 
https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-pcu
Gerrit-Branch: master
Gerrit-Change-Id: Iebb9c57f458690e045ddc45c800209ad8cf621e0
Gerrit-Change-Number: 18224
Gerrit-PatchSet: 1
Gerrit-Owner: pespin 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge 
Gerrit-Comment-Date: Tue, 12 May 2020 18:00:00 +
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment

Change in osmo-pcu[master]: Drop unneeded arg 'ta' in tbf_alloc_ul()

2020-05-12 Thread pespin 
pespin has uploaded this change for review. ( 
https://gerrit.osmocom.org/c/osmo-pcu/+/18224 )


Change subject: Drop unneeded arg 'ta' in tbf_alloc_ul()
..

Drop unneeded arg 'ta' in tbf_alloc_ul()

The function is simply setting the ta on the ms, so simply make sure ta
is set on callers before passing the ms object.

Change-Id: Iebb9c57f458690e045ddc45c800209ad8cf621e0
---
M src/pdch.cpp
M src/tbf.cpp
M src/tbf.h
M tests/tbf/TbfTest.err
4 files changed, 16 insertions(+), 20 deletions(-)



  git pull ssh://gerrit.osmocom.org:29418/osmo-pcu refs/changes/24/18224/1

diff --git a/src/pdch.cpp b/src/pdch.cpp
index 048577d..88d5d31 100644
--- a/src/pdch.cpp
+++ b/src/pdch.cpp
@@ -118,7 +118,7 @@
bts->channel_request_description();

/* This call will register the new TBF with the MS on success */
-   gprs_rlcmac_ul_tbf *ul_tbf = tbf_alloc_ul(bts_data, tbf->ms(), 
tbf->trx->trx_no, tbf->tlli(), tbf->ta());
+   gprs_rlcmac_ul_tbf *ul_tbf = tbf_alloc_ul(bts_data, tbf->ms(), 
tbf->trx->trx_no, tbf->tlli());

/* schedule uplink assignment or reject */
if (ul_tbf) {
@@ -551,7 +551,6 @@
struct gprs_rlcmac_ul_tbf *ul_tbf;
struct gprs_rlcmac_dl_tbf *dl_tbf;
uint32_t tlli = request->ID.u.TLLI;
-   uint8_t ta = GSM48_TA_INVALID;
bool found = true;

GprsMs *ms = bts()->ms_by_tlli(tlli);
@@ -566,7 +565,6 @@
if (found) {
ul_tbf = ms->ul_tbf();
dl_tbf = ms->dl_tbf();
-   ta = ms->ta();
/* We got a RACH so the MS was in packet idle mode and 
thus
 * didn't have any active TBFs */
if (ul_tbf) {
@@ -593,7 +591,7 @@
"block, but there is no resource request "
"scheduled!\n");
} else {
-   ta = sba->ta;
+   ms->set_ta(sba->ta);
bts()->sba()->free_sba(sba);
}
if (request->Exist_MS_Radio_Access_capability2) {
@@ -613,7 +611,7 @@
"MS supports EGPRS multislot class %d.\n",
ms->egprs_ms_class());

-   ul_tbf = tbf_alloc_ul(bts_data(), ms, trx_no(), tlli, ta);
+   ul_tbf = tbf_alloc_ul(bts_data(), ms, trx_no(), tlli);
if (!ul_tbf) {
handle_tbf_reject(bts_data(), ms, tlli,
trx_no(), ts_no);
diff --git a/src/tbf.cpp b/src/tbf.cpp
index 56fdcd1..2693223 100644
--- a/src/tbf.cpp
+++ b/src/tbf.cpp
@@ -416,7 +416,7 @@
 }

 gprs_rlcmac_ul_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts, GprsMs *ms, 
int8_t use_trx,
-uint32_t tlli, uint8_t ta)
+uint32_t tlli)
 {
struct gprs_rlcmac_ul_tbf *tbf;

@@ -435,8 +435,6 @@
tbf->update_ms(tlli, GPRS_RLCMAC_UL_TBF);
OSMO_ASSERT(tbf->ms());

-   tbf->ms()->set_ta(ta);
-
return tbf;
 }

diff --git a/src/tbf.h b/src/tbf.h
index 5ebd2e8..2b4cf6d 100644
--- a/src/tbf.h
+++ b/src/tbf.h
@@ -347,7 +347,7 @@


 struct gprs_rlcmac_ul_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts, GprsMs 
*ms,
-   int8_t use_trx, uint32_t tlli, uint8_t 
ta);
+   int8_t use_trx, uint32_t tlli);

 struct gprs_rlcmac_ul_tbf *tbf_alloc_ul_tbf(struct gprs_rlcmac_bts *bts, 
GprsMs *ms, int8_t use_trx, bool single_slot);

diff --git a/tests/tbf/TbfTest.err b/tests/tbf/TbfTest.err
index 6019def..ca16149 100644
--- a/tests/tbf/TbfTest.err
+++ b/tests/tbf/TbfTest.err
@@ -1526,6 +1526,7 @@
 - RX : Uplink Control Block -
 Creating MS object, TLLI = 0x
 MS requests UL TBF in packet resource request of single block, so we provide 
one:
+Modifying MS object, TLLI = 0x, TA 220 -> 7
 Modifying MS object, TLLI = 0x, MS class 0 -> 1
 ** UL-TBF starts here **
 Allocating UL TBF: MS_CLASS=1/0
@@ -1545,7 +1546,6 @@
 TBF(TFI=0 TLLI=0x DIR=UL STATE=NULL) changes state from NULL to ASSIGN
 TBF(TFI=0 TLLI=0x DIR=UL STATE=ASSIGN) starting timer T3169 
[allocation (UL-TBF)] with 5 sec. 0 microsec, cur_fn=0
 Modifying MS object, UL TLLI: 0x -> 0xf1223344, not yet confirmed
-Modifying MS object, TLLI = 0xf1223344, TA 220 -> 7
 TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=ASSIGN) change control TS 7 -> 7 until 
assignment is complete.
 TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=ASSIGN) changes UL ASS state from 
GPRS_RLCMAC_UL_ASS_NONE to GPRS_RLCMAC_UL_ASS_SEND_ASS
 TBF(TFI=0 TLLI=0xf1223344 DIR=UL STATE=ASSIGN) start Packet Uplink Assignment 
(PACCH)
@@ -1608,6 +1608,7 @@
 - RX : Uplink