Attention is currently required from: Hoernchen, pespin.
fixeria has posted comments on this change. (
https://gerrit.osmocom.org/c/osmo-trx/+/30416 )
Change subject: ms-trx support
......................................................................
Patch Set 3:
(21 comments)
File Transceiver52M/ms/ms_upper.h:
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/de8e74a6_3a259a08
PS3, Line 1:
ws
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/0a302518_73cc4d5c
PS3, Line 36: trx_if
do you really need/use TRXC/TRXD here?
File Transceiver52M/ms/ms_upper.cpp:
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/32a010d7_8a17baca
PS3, Line 61: #include <osmocom/core/gsmtap_util.h>
you're not using GSMTAP API here...
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/226cc199_31255475
PS3, Line 64: // #include <osmocom/core/application.h>
removeme
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/93fc1081_e6a4c799
PS3, Line 71: trx_if
do you really need/use TRXC/TRXD here?
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/fef1b6b3_90d5b9f3
PS3, Line 74: #include <osmocom/bb/l1sched/l1sched.h>
I don't see any l1sched_* API used in this file.
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/7aa85f32_4e0185b4
PS3, Line 177: return trash
can you clarify this a bit further? thrash?
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/8b6afa5b_a3a7daf3
PS3, Line 184: RSSI = 10
why 10?
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/3aeccd0b_dd0bd268
PS3, Line 223: timingOffset = (int)round(0);
TODO/FIXME?
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/164d3c60_0da5723e
PS3, Line 238: trx_data_rx_handler
removeme
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/3f5c1c4f_7ee4a937
PS3, Line 246: trxcon_phyif_handle_clock_ind
removeme
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/d43344cf_b38b007c
PS3, Line 304: case trxcon::TRXCON_PHYIF_CMDT_RESET
Better use value-string for that. Ideally such a value string should be part of libtrxcon,
so it's always in sync with the TRXCON_PHYIF_CMDT_* values.
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/2faadf5a_771fc6f7
PS3, Line 352: set_ta
Shouldn't this be done on receipt of TRXCON_PHYIF_CMDT_RESET instead?
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/86b41add_7e99a70e
PS3, Line 357: dbm = -80
please add a FIXME or TODO, so it's clear that the measurement logic is missing
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/3d17174c_41e5bdcd
PS3, Line 364: gsm_arfcn2freq10
careful, this function may return 0xffff
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/76523f1d_1714766d
PS3, Line 385: assert
OSMO_ASSERT?
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/32cb86c8_4b13670b
PS3, Line 425: trxcon
it's not trxcon app anymore
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/46c73659_9bc25e02
PS3, Line 429: 3
pass 0 to avoid confusion, we're not using fn_advance here
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/17f43833_0f042a41
PS3, Line 430: gsmtap = 0;
gsmtap is a pointer, so = NULL
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/f9efc989_1a4b0ef7
PS3, Line 431: 0x1234
What's this? Why not NULL?
https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/31d216f3_ce23d1ec
PS3, Line 433: trxc
I would avoid using 'trxc' here to avoid confusion with the TRXC protocol.
--
To view, visit
https://gerrit.osmocom.org/c/osmo-trx/+/30416
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-trx
Gerrit-Branch: master
Gerrit-Change-Id: I36c65a8c725c4da76dc70006cd96b0a2b6878e84
Gerrit-Change-Number: 30416
Gerrit-PatchSet: 3
Gerrit-Owner: Hoernchen <ewild(a)sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <vyanitskiy(a)sysmocom.de>
Gerrit-CC: pespin <pespin(a)sysmocom.de>
Gerrit-Attention: Hoernchen <ewild(a)sysmocom.de>
Gerrit-Attention: pespin <pespin(a)sysmocom.de>
Gerrit-Comment-Date: Thu, 01 Dec 2022 14:08:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment