Attention is currently required from: Hoernchen, laforge, fixeria.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-trx/+/33926 )
Change subject: ms: drop the tx burst padding
......................................................................
Patch Set 3: Code-Review+1
(1 comment)
Patchset:
PS3:
> well it has been useless for quite some time but there was no reason to change it because everything […]
Have in mind also that others may not feel like reviewing your patches if they find not enough explanation/context!
--
To view, visit https://gerrit.osmocom.org/c/osmo-trx/+/33926
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-trx
Gerrit-Branch: master
Gerrit-Change-Id: Ied5c3ab5dde975e11b0ef6d9cbc86be19173c4e8
Gerrit-Change-Number: 33926
Gerrit-PatchSet: 3
Gerrit-Owner: Hoernchen <ewild(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: Hoernchen <ewild(a)sysmocom.de>
Gerrit-Attention: laforge <laforge(a)osmocom.org>
Gerrit-Attention: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 27 Jul 2023 21:20:31 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: Hoernchen <ewild(a)sysmocom.de>
Comment-In-Reply-To: laforge <laforge(a)osmocom.org>
Gerrit-MessageType: comment
Attention is currently required from: jolly.
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-msc/+/33965 )
Change subject: ASCI: Add debugging and error logging to VGCS/VBS call control
......................................................................
Patch Set 1: Code-Review+1
(1 comment)
Patchset:
PS1:
we should be removing all those dots before \n :)
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/33965
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: I3fbce71e47e9670f58e46e7c2a0f26a1bbc1a46e
Gerrit-Change-Number: 33965
Gerrit-PatchSet: 1
Gerrit-Owner: jolly <andreas(a)eversberg.eu>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: jolly <andreas(a)eversberg.eu>
Gerrit-Comment-Date: Thu, 27 Jul 2023 21:16:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Hoernchen, pespin.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-trx/+/33944 )
Change subject: ms: restructure the va code to add rach support
......................................................................
Patch Set 6:
(6 comments)
File Transceiver52M/grgsm_vitac/grgsm_vitac.h:
https://gerrit.osmocom.org/c/osmo-trx/+/33944/comment/402d44d8_32e18d97
PS6, Line 61: MULTI_VER_TARGET_ATTR_CLANGONLY
Why removing this? If on purpose, please explain in the commit message.
https://gerrit.osmocom.org/c/osmo-trx/+/33944/comment/bfde6848_a3b770d8
PS6, Line 62: detect_burst_access_burst
`detect_burst_ab()` for consistency with `detect_burst_nb()` then?
File Transceiver52M/grgsm_vitac/grgsm_vitac.cpp:
https://gerrit.osmocom.org/c/osmo-trx/+/33944/comment/5e2af26d_b3af6876
PS6, Line 77: MULTI_VER_TARGET_ATTR NO_UBSAN
Why removing this? If on purpose, please explain in the commit message.
https://gerrit.osmocom.org/c/osmo-trx/+/33944/comment/03a0ecb3_d2d28bf6
PS6, Line 42: d_sync_training_seq[N_ACCESS_BITS]; ///<encoded training sequence of a SCH burst
I find this confusing: the comment mentions SCH, the symbol name contains `_sync_`, and the `N_ACCESS_BITS` is perhaps from the context of access bursts? Looks like the comment is copy-pasted? If it's for access bursts, maybe call it `d_acc_training_seq`?
https://gerrit.osmocom.org/c/osmo-trx/+/33944/comment/9c0c697b_0e8ea44b
PS6, Line 71: gr_complex startpoint = train_seq[i][0] == 0 ? gr_complex(1.0, 0.0) : gr_complex(-1.0, 0.0);
Unrelated spacing changes. I am find with that in general, but it makes the patch harder to read, so ideally should be done separately.
https://gerrit.osmocom.org/c/osmo-trx/+/33944/comment/0ffd49f1_528eceb7
PS6, Line 139: gmsk_output[i] = j * gr_complex(encoded_symbol, 0.0) * gmsk_output[i - 1];
Again, I am fine with proper formatting, but it makes your patch harder to read.
--
To view, visit https://gerrit.osmocom.org/c/osmo-trx/+/33944
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-trx
Gerrit-Branch: master
Gerrit-Change-Id: I4a5cedc8c9a3289c75ce7b914eac286e601ebed0
Gerrit-Change-Number: 33944
Gerrit-PatchSet: 6
Gerrit-Owner: Hoernchen <ewild(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-Reviewer: pespin <pespin(a)sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: Hoernchen <ewild(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 27 Jul 2023 20:58:53 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Hoernchen.
fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-trx/+/33927 )
Change subject: ms: update osmocom-bb
......................................................................
Patch Set 4:
(2 comments)
Patchset:
PS4:
Please also add a dedicated logging category for l1gprs (`TRXCON_LOGC_GPRS`/`DGPRS`), like it's done in osmocom-bb.git [1]. Otherwise all GPRS related logging would be using `DLGLOBAL`.
[1] https://cgit.osmocom.org/osmocom-bb/tree/src/host/trxcon/src/logging.c#n89
File osmocom-bb:
https://gerrit.osmocom.org/c/osmo-trx/+/33927/comment/e4f56b53_4965e9d9
PS4, Line 1: 60215bc051c96d87c552ece42d594be7f7388c4f
I recommend upgrading to even more recent version (`3f409eb94eac9ffa67a7528f29f58275f0b836b8`), which has several important fixes for the scheduler (TCH handling).
--
To view, visit https://gerrit.osmocom.org/c/osmo-trx/+/33927
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-trx
Gerrit-Branch: master
Gerrit-Change-Id: I6f9fca7a4d6a02e82bf406fd136c5bde96bb93af
Gerrit-Change-Number: 33927
Gerrit-PatchSet: 4
Gerrit-Owner: Hoernchen <ewild(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge(a)osmocom.org>
Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-Attention: Hoernchen <ewild(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 27 Jul 2023 20:39:44 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment