Attention is currently required from: fixeria.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/32727 )
Change subject: osmo-bts-trx: implement CSD scheduling support
......................................................................
Patch Set 16: Code-Review+1
(1 comment)
Patchset:
PS16:
not super critical, but there still are two unresolved comments regarding magic numbers
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/32727
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I08ffbf8e79ce76a586d61f5463890c6e72a6d9b9
Gerrit-Change-Number: 32727
Gerrit-PatchSet: 16
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: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 12 Jul 2023 07:38:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: fixeria.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/33703 )
Change subject: osmo-bts-trx: pull the AMR header in tch_dl_dequeue()
......................................................................
Patch Set 2: Code-Review+1
--
To view, visit https://gerrit.osmocom.org/c/osmo-bts/+/33703
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I15318e497b236128f779769e4fa99f307ea431ea
Gerrit-Change-Number: 33703
Gerrit-PatchSet: 2
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 12 Jul 2023 07:37:07 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: fixeria, dexter.
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/pysim/+/33704 )
Change subject: cdma_ruim: Fix unit tests and actually enable them
......................................................................
Patch Set 1:
(1 comment)
File pySim/cdma_ruim.py:
https://gerrit.osmocom.org/c/pysim/+/33704/comment/cd64abae_c14f6d65
PS1, Line 136: HexAdapter
> Then we should be using `HexAdapter` for the `rfu` field too, because it's also `bytes`?
yes, nice catch!
--
To view, visit https://gerrit.osmocom.org/c/pysim/+/33704
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: pysim
Gerrit-Branch: master
Gerrit-Change-Id: Icdf4621eb68d05a4948ae9efeb81a007d48e1bb7
Gerrit-Change-Number: 33704
Gerrit-PatchSet: 1
Gerrit-Owner: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: dexter <pmaier(a)sysmocom.de>
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: dexter <pmaier(a)sysmocom.de>
Gerrit-Comment-Date: Wed, 12 Jul 2023 07:36:37 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Comment-In-Reply-To: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: comment
Attention is currently required from: daniel.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/33686 )
Change subject: osmo_io: Add function to change the maximum length of the tx_queue
......................................................................
Patch Set 2: Code-Review+1
(2 comments)
File src/core/osmo_io.c:
https://gerrit.osmocom.org/c/libosmocore/+/33686/comment/6f1c5773_11b19393
PS2, Line 545: g
(end in a dot for doxygen AUTO_BRIEF)
https://gerrit.osmocom.org/c/libosmocore/+/33686/comment/f5292165_0abbe2ec
PS2, Line 549: void osmo_iofd_set_txqueue_max_length(struct osmo_io_fd *iofd, unsigned int size)
(i find mildly curious mixing of "max_length" and "size" and "maximum size" terms, would rather stick to the identical name 'max_length' also for the function arg)
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/33686
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: If3d1de8bffe1123002515878655ea9e02b482888
Gerrit-Change-Number: 33686
Gerrit-PatchSet: 2
Gerrit-Owner: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 11 Jul 2023 21:01:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Jenkins Builder has posted comments on this change. ( https://gerrit.osmocom.org/c/osmocom-bb/+/33708 )
Change subject: trxcon/l1sched: rework dequeueing of Tx prims
......................................................................
Patch Set 1:
(1 comment)
File src/host/trxcon/src/sched_lchan_tchf.c:
Robot Comment from checkpatch (run ID jenkins-gerrit-lint-9518):
https://gerrit.osmocom.org/c/osmocom-bb/+/33708/comment/bdace033_6a685848
PS1, Line 234: } else {
else is not generally useful after a break or return
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/33708
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: I3d6c2136ff1855ab0aa9062b20b2a64fd0e5fe28
Gerrit-Change-Number: 33708
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: Jenkins Builder
Gerrit-Comment-Date: Tue, 11 Jul 2023 20:54:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: daniel.
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/33685 )
Change subject: osmo_io: Document expectation that segmentation_cb() can modify msgb
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
File include/osmocom/core/osmo_io.h:
https://gerrit.osmocom.org/c/libosmocore/+/33685/comment/a79d5a25_9e72b138
PS1, Line 50: will be trimmed to size
i find it ambiguous: is this something that always happens before/after calling segmentation_cb(), or is segmentation_cb() expected to implement it?
Possibly I'm just not familiar enough with segmentation, so feel free to ignore this review if appropriate. (I also don't understand whether segmentation_cb() splits up or joins messages from this doc...)
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/33685
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Idd2115baae98a7818aabb26232d4423d2d48fb5c
Gerrit-Change-Number: 33685
Gerrit-PatchSet: 1
Gerrit-Owner: daniel <dwillmann(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr(a)sysmocom.de>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: daniel <dwillmann(a)sysmocom.de>
Gerrit-Comment-Date: Tue, 11 Jul 2023 20:54:43 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
fixeria has uploaded this change for review. ( https://gerrit.osmocom.org/c/osmocom-bb/+/33709 )
Change subject: trxcon/l1sched: peoperly prioritize FACCH/H over TCH
......................................................................
trxcon/l1sched: peoperly prioritize FACCH/H over TCH
Unlike FACCH/F, which steals one TCH frame, FACCH/H steals two TCH
frames. This is what prim_dequeue_tchh() aims to implement, but
the current implementation is not 100% correct.
The problem is that we're attempting to dequeue and drop two TCH frames
in one go, whenever we get a FACCH/H frame. Most likely, there will be
no 2nd TCH frame in the Tx queue at that time, so it will never be
dropped and will clog the queue.
Let's replicate what osmo-bts-trx does:
* dequeue and drop the 1st TCH frame when sending 1st/6 burst of FACCH,
* dequeue and drop the 2nd TCH frame when sending 3rd/6 burst of FACCH.
Change-Id: I513d6805ddf97783c002be285fb3ca7893e42377
Related: OS#4396, OS#1572
---
M src/host/trxcon/src/sched_lchan_tchh.c
1 file changed, 41 insertions(+), 27 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmocom-bb refs/changes/09/33709/1
diff --git a/src/host/trxcon/src/sched_lchan_tchh.c b/src/host/trxcon/src/sched_lchan_tchh.c
index 6affbc5..67e2132 100644
--- a/src/host/trxcon/src/sched_lchan_tchh.c
+++ b/src/host/trxcon/src/sched_lchan_tchh.c
@@ -414,36 +414,23 @@
static struct msgb *prim_dequeue_tchh(struct l1sched_lchan_state *lchan, uint32_t fn)
{
- struct msgb *facch;
- struct msgb *tch;
- bool facch_now;
+ struct msgb *msg_facch;
+ struct msgb *msg_tch;
- /* Can we initiate an UL FACCH/H frame transmission at this Fn? */
- facch_now = l1sched_tchh_facch_start(lchan->type, fn, true);
- if (!facch_now)
- goto no_facch;
+ /* dequeue a pair of TCH and FACCH frames */
+ msg_tch = l1sched_lchan_prim_dequeue_tch(lchan, false);
+ if (l1sched_tchh_facch_start(lchan->type, fn, true))
+ msg_facch = l1sched_lchan_prim_dequeue_tch(lchan, true);
+ else
+ msg_facch = NULL;
- /* If there are no FACCH/H prims in the queue */
- facch = l1sched_lchan_prim_dequeue_tch(lchan, true);
- if (!facch) /* Just dequeue a TCH/H prim */
- goto no_facch;
-
- /* FACCH/H prim replaces two TCH/F prims */
- tch = l1sched_lchan_prim_dequeue_tch(lchan, false);
- if (tch) {
- /* At least one TCH/H prim is dropped */
- msgb_free(tch);
-
- /* Attempt to find another */
- tch = l1sched_lchan_prim_dequeue_tch(lchan, false);
- if (tch) /* Drop the second TCH/H prim */
- msgb_free(tch);
+ /* prioritize FACCH over TCH */
+ if (msg_facch != NULL) {
+ msgb_free(msg_tch); /* drop 1st TCH/HS block */
+ return msg_facch;
}
- return facch;
-
-no_facch:
- return l1sched_lchan_prim_dequeue_tch(lchan, false);
+ return msg_tch;
}
int tx_tchh_fn(struct l1sched_lchan_state *lchan,
@@ -478,8 +465,11 @@
*mask = *mask << 2;
/* If FACCH/H blocks are still pending */
- if (lchan->ul_facch_blocks > 2)
+ if (lchan->ul_facch_blocks > 2) {
+ struct msgb *msg = l1sched_lchan_prim_dequeue_tch(lchan, false);
+ msgb_free(msg); /* drop 2nd TCH/HS block */
goto send_burst;
+ }
lchan->prim = prim_dequeue_tchh(lchan, br->fn);
if (lchan->prim == NULL) {
--
To view, visit https://gerrit.osmocom.org/c/osmocom-bb/+/33709
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmocom-bb
Gerrit-Branch: master
Gerrit-Change-Id: I513d6805ddf97783c002be285fb3ca7893e42377
Gerrit-Change-Number: 33709
Gerrit-PatchSet: 1
Gerrit-Owner: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-MessageType: newchange