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: Code-Review-1 (16 comments) File Makefile.am: https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/b149085f_86bef51a PS3, Line 37: osmocom-bb/src/host/trxcon \ Ack. Most likely he forgot to remove this line. File Transceiver52M/Makefile.am: https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/d979a6e9_746c2026 PS3, Line 26: AM_CPPFLAGS '-I' stuff should be in CPPFLAGS only, it makes no sense to have it in CFLAGS/CXXFLAGS. https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/c6880f67_88b3bfc7 PS3, Line 77: LIBOSMOCODING_LIBS Having LIBOSMOCODING_LIBS in COMMON_LDADD makes all osmo-trx-* binaries depend on libosmocoding, while it's only needed for osmo-trx-ms-* and osmo-prbs-tool. https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/83a2d0fc_dd0a1d18 PS3, Line 90: noinst_HEADERS This should be done conditionally too, but not critical I guess. File Transceiver52M/ms/itrq.h: https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/16aab7e0_8f217ba5 PS3, Line 34: /* weird comment formatting, please make it consistent with the copyright header File Transceiver52M/ms/l1ctl_server_cb.cpp: https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/e0c228dc_35756759 PS3, Line 65: osmocom_l2 TODO: make path configurable https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/2921f8c4_7e180c4a PS3, Line 74: return so the app keeps running if l1ctl_server_alloc() fails? File Transceiver52M/ms/logging.cpp: https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/d82e3853_8e6bb5aa PS3, Line 54: loglevel = LOGL_NOTICE spacing issues https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/8293fbce_b4ab0313 PS3, Line 57: DTRXC not used by libtrxcon nor needed https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/98faf6d8_0b0b4d10 PS3, Line 64: DTRXD not used by libtrxcon nor needed https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/9ac236f1_1832c8bf PS3, Line 75: .loglevel = LOGL_NOTICE, spacing issues https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/577062e9_90eb9294 PS3, Line 76: enabled = 0, why disabled? https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/40fa09ba_dd2d7c09 PS3, Line 82: .loglevel = LOGL_NOTICE, spacing issues File configure.ac: https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/668f1798_05ce8f1c PS3, Line 330: test -d osmocom-bb A proper approach would be making this configurable: --enable-ms or --with-ms. https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/552f7b86_a9ccfcd7 PS3, Line 333: AC_MSG_NOTICE(["Enabling ms-trx..."]) spacing issues https://gerrit.osmocom.org/c/osmo-trx/+/30416/comment/fd88649c_69bebf3b PS3, Line 364: doc/manuals/Makefile \ Ack, it's one of my fixups: https://cgit.osmocom.org/osmo-trx/commit/?h=fixeria/ms&id=8ed86879c4ea9… -- 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 13:26:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: pespin <pespin(a)sysmocom.de> Gerrit-MessageType: comment