Attention is currently required from: osmith, laforge, fixeria.
Hello osmith, Jenkins Builder, laforge, pespin,
I'd like you to reexamine a change. Please visit
https://gerrit.osmocom.org/c/osmocom-bb/+/33414
to look at the new patch set (#4).
Change subject: modem: properly handle Dedicated mode or TBF IE
......................................................................
modem: properly handle Dedicated mode or TBF IE
We need to distinguish between Uplink and Downlink TBF assignment in
grr_rx_imm_ass(), because matching the Request Reference IE makes
sense only for the Uplink TBF assignment.
Uplink TBFs are requested by the UEs by sending RACH, while Downlink
TBFs are assigned by the network itself. The Request Reference IE
is only valid for Uplink assignments and shall be ignored in messages
assigning Downlink TBFs.
Change-Id: Idb9b3203147be3b42256c0bcab3ecdabcf2d2fa9
Related: OS#5500
---
M src/host/layer23/src/modem/grr.c
1 file changed, 54 insertions(+), 13 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmocom-bb refs/changes/14/33414/4
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/33414
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: Idb9b3203147be3b42256c0bcab3ecdabcf2d2fa9
Gerrit-Change-Number: 33414
Gerrit-PatchSet: 4
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newpatchset
Attention is currently required from: osmith, laforge, fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/33414 )
Change subject: modem: properly handle Dedicated mode or TBF IE
......................................................................
Patch Set 3:
(1 comment)
File src/host/layer23/src/modem/grr.c:
https://gerrit.osmocom.org/c/osmocom-bb/+/33414/comment/2a51cdda_7d87f8e0
PS3, Line 278: if ((dm_or_tbf & (1 << 1)) == 0) {
> I don't see how this improves readability, sorry. […]
"check Request Reference IE if this is an Uplink TBF assignment." That's a better and clearer comment than the one you have in the current patch.
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/33414
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: Idb9b3203147be3b42256c0bcab3ecdabcf2d2fa9
Gerrit-Change-Number: 33414
Gerrit-PatchSet: 3
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 29 Jun 2023 16:40:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: osmith, laforge, pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/33414 )
Change subject: modem: properly handle Dedicated mode or TBF IE
......................................................................
Patch Set 3:
(1 comment)
File src/host/layer23/src/modem/grr.c:
https://gerrit.osmocom.org/c/osmocom-bb/+/33414/comment/49b2475a_4458d86b
PS3, Line 278: if ((dm_or_tbf & (1 << 1)) == 0) {
> You can use "bool is_ul_ass = (dm_or_tbf & (1 << 1))", or rather in line 277 simply write "Check UL […]
I don't see how this improves readability, sorry. A boolean flag would improve a bit, but it's redundant to add a bool for one-time use in the if statement next line. The comment suggestions... there is already a comment stating that we check Request Reference IE if this is an Uplink TBF assignment.
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/33414
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: Idb9b3203147be3b42256c0bcab3ecdabcf2d2fa9
Gerrit-Change-Number: 33414
Gerrit-PatchSet: 3
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 29 Jun 2023 16:39:10 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: osmith, laforge, pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/33414 )
Change subject: modem: properly handle Dedicated mode or TBF IE
......................................................................
Patch Set 3:
(1 comment)
File src/host/layer23/src/modem/grr.c:
https://gerrit.osmocom.org/c/osmocom-bb/+/33414/comment/1ce75163_7c817ce1
PS3, Line 278: if ((dm_or_tbf & (1 << 1)) == 0) {
> it may be clear to you, I see references to UL TBF, assignment, request reference IE and then some b […]
Ok, what exactly am I supposed to clarify? How do I make it cleaner?
Copy-paste text/tables from 3GPP TS 44.018, section 10.5.2.25b in a comment?
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/33414
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: Idb9b3203147be3b42256c0bcab3ecdabcf2d2fa9
Gerrit-Change-Number: 33414
Gerrit-PatchSet: 3
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 29 Jun 2023 15:59:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: osmith, laforge, fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/33414 )
Change subject: modem: properly handle Dedicated mode or TBF IE
......................................................................
Patch Set 3:
(1 comment)
File src/host/layer23/src/modem/grr.c:
https://gerrit.osmocom.org/c/osmocom-bb/+/33414/comment/2b495544_e0faa6c1
PS3, Line 278: if ((dm_or_tbf & (1 << 1)) == 0) {
> The comment clearly states "Uplink TBF", so the if-condition is obviously checking if it's an Uplink […]
it may be clear to you, I see references to UL TBF, assignment, request reference IE and then some bitmask check, so not clear at all for the rest :)
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/33414
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: Idb9b3203147be3b42256c0bcab3ecdabcf2d2fa9
Gerrit-Change-Number: 33414
Gerrit-PatchSet: 3
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: osmith <osmith(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: osmith <osmith(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 29 Jun 2023 15:47:21 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: laforge, pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/33533 )
Change subject: common/Makefile.am: comment out 'libbts_la_LDADD = probes.lo'
......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1:
> maybe ping whoever added that line? git blame/log.
It was Harald in `430954630`, adding him as a reviewer. I think it's fine to have this line commented out for now because we don't have any probes for `libbts`.
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/33533
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I829c63988fd51b481cb9f13a81dfaf5e78beb1b8
Gerrit-Change-Number: 33533
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 29 Jun 2023 15:40:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin(a)sysmocom.de>
Gerrit-MessageType: comment
fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmo-bts/+/33533 )
Change subject: common/Makefile.am: comment out 'libbts_la_LDADD = probes.lo'
......................................................................
common/Makefile.am: comment out 'libbts_la_LDADD = probes.lo'
This line makes no sense and triggers the following warnings:
src/common/Makefile.am:83: warning: variable 'libbts_la_LDADD' is defined but no program or
src/common/Makefile.am:83: library has 'libbts_la' as canonical name (possible typo)
The problem is that there is no 'libbts.la' (libtool archive), but
'libbts.a' (normal archive file). There is no way to attach
dependencies to *.a archives, it's only possible for *.la archives.
Either each of the BTS variants needs to add this 'probes.lo' to
their LDADD list manually, or the 'libbts.a' needs to be converted
to a libtool archive 'libbts.la'. For comment out this line.
Change-Id: I829c63988fd51b481cb9f13a81dfaf5e78beb1b8
---
M src/common/Makefile.am
1 file changed, 24 insertions(+), 1 deletion(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-bts refs/changes/33/33533/1
diff --git a/src/common/Makefile.am b/src/common/Makefile.am
index 32f644c..0da62bb 100644
--- a/src/common/Makefile.am
+++ b/src/common/Makefile.am
@@ -80,5 +80,6 @@
$(LIBTOOL) --mode=compile $(AM_V_lt) --tag=CC env CFLAGS="$(CFLAGS)" $(DTRACE) -C -G -s $< -o $@
BUILT_SOURCES = probes.h probes.lo
-libbts_la_LDADD = probes.lo
+# FIXME: libbts is not a libtool archive (*.la)
+# libbts_la_LIBADD = probes.lo
endif
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/33533
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I829c63988fd51b481cb9f13a81dfaf5e78beb1b8
Gerrit-Change-Number: 33533
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newchange