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@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> Gerrit-CC: pespin <pespin@sysmocom.de> Gerrit-Attention: Hoernchen <ewild@sysmocom.de> Gerrit-Attention: pespin <pespin@sysmocom.de> Gerrit-Comment-Date: Thu, 01 Dec 2022 14:08:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment