Attention is currently required from: Hoernchen, pespin.
Patch set 3:Code-Review -1
16 comments:
File Makefile.am:
Patch Set #3, Line 37: osmocom-bb/src/host/trxcon \
osmocom-bb/src/host/trxcon is added twice? here and above
Ack. Most likely he forgot to remove this line.
File Transceiver52M/Makefile.am:
Patch Set #3, Line 26: AM_CPPFLAGS
'-I' stuff should be in CPPFLAGS only, it makes no sense to have it in CFLAGS/CXXFLAGS.
Patch Set #3, 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.
Patch Set #3, Line 90: noinst_HEADERS
This should be done conditionally too, but not critical I guess.
File Transceiver52M/ms/itrq.h:
weird comment formatting, please make it consistent with the copyright header
File Transceiver52M/ms/l1ctl_server_cb.cpp:
Patch Set #3, Line 65: osmocom_l2
TODO: make path configurable
Patch Set #3, Line 74: return
so the app keeps running if l1ctl_server_alloc() fails?
File Transceiver52M/ms/logging.cpp:
Patch Set #3, Line 54: loglevel = LOGL_NOTICE
spacing issues
Patch Set #3, Line 57: DTRXC
not used by libtrxcon nor needed
Patch Set #3, Line 64: DTRXD
not used by libtrxcon nor needed
Patch Set #3, Line 75: .loglevel = LOGL_NOTICE,
spacing issues
Patch Set #3, Line 76: enabled = 0,
why disabled?
Patch Set #3, Line 82: .loglevel = LOGL_NOTICE,
spacing issues
File configure.ac:
Patch Set #3, Line 330: test -d osmocom-bb
A proper approach would be making this configurable: --enable-ms or --with-ms.
Patch Set #3, Line 333: AC_MSG_NOTICE(["Enabling ms-trx..."])
spacing issues
Patch Set #3, Line 364: doc/manuals/Makefile \
This looks like a separate patch.
Ack, it's one of my fixups: https://cgit.osmocom.org/osmo-trx/commit/?h=fixeria/ms&id=8ed86879c4ea9a0ed487896b1092bc8c94d5f173
To view, visit change 30416. To unsubscribe, or for help writing mail filters, visit settings.