Attention is currently required from: fixeria, pespin.
37 comments:
File Makefile.am:
Patch Set #3, Line 37: osmocom-bb/src/host/trxcon \
Ack. Most likely he forgot to remove this line.
Done
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.
Done
Patch Set #3, Line 77: LIBOSMOCODING_LIBS
Having LIBOSMOCODING_LIBS in COMMON_LDADD makes all osmo-trx-* binaries depend on libosmocoding, whi […]
Done
Patch Set #3, Line 90: noinst_HEADERS
This should be done conditionally too, but not critical I guess.
Done
File Transceiver52M/ms/itrq.h:
weird comment formatting, please make it consistent with the copyright header
Done
File Transceiver52M/ms/l1ctl_server_cb.cpp:
Patch Set #3, Line 65: osmocom_l2
TODO: make path configurable
Done
Patch Set #3, Line 74: return
so the app keeps running if l1ctl_server_alloc() fails?
Done
File Transceiver52M/ms/logging.cpp:
Patch Set #3, Line 54: loglevel = LOGL_NOTICE
spacing issues
Done
Patch Set #3, Line 57: DTRXC
not used by libtrxcon nor needed
Done
Patch Set #3, Line 64: DTRXD
not used by libtrxcon nor needed
Done
Patch Set #3, Line 75: .loglevel = LOGL_NOTICE,
spacing issues
Done
Patch Set #3, Line 76: enabled = 0,
why disabled?
too nuch unbounded log spam impacts timing.
Patch Set #3, Line 82: .loglevel = LOGL_NOTICE,
spacing issues
Done
File Transceiver52M/ms/ms_upper.h:
ws
Done
Patch Set #3, Line 36: trx_if
do you really need/use TRXC/TRXD here?
Done
File Transceiver52M/ms/ms_upper.cpp:
Patch Set #3, Line 61: #include <osmocom/core/gsmtap_util.h>
you're not using GSMTAP API here...
Done
Patch Set #3, Line 64: // #include <osmocom/core/application.h>
removeme
Done
Patch Set #3, Line 71: trx_if
do you really need/use TRXC/TRXD here?
Done
Patch Set #3, Line 74: #include <osmocom/bb/l1sched/l1sched.h>
I don't see any l1sched_* API used in this file.
Done
Patch Set #3, Line 177: return trash
can you clarify this a bit further? thrash?
fcch bursts contain "nothing", but the old trxcon needed "something" to tickle the scheduler. There are only 3 burst types, so explictily listing all of those is fine, even if the new scheduler allows skipping those and the function returns early.
Patch Set #3, Line 184: RSSI = 10
why 10?
random reasonable value, the SCH burst is only demodulated once in the timekeeping part because doing it twice introduces massive latency spikes comapared to NB that are only handled in the upper layer, so there is no opportunity here to measure anything because only the demodded bits are passed to the upper layer in the SCH case.
Patch Set #3, Line 223: timingOffset = (int)round(0);
TODO/FIXME?
Done
In practice the alignment does not matter because the lower loop continuously tracks and updates it anyway so the only offset available would be the short term offset until it gets updated and reset to 0.
Patch Set #3, Line 238: trx_data_rx_handler
removeme
Done
Patch Set #3, Line 246: trxcon_phyif_handle_clock_ind
removeme
Done
Patch Set #3, Line 304: case trxcon::TRXCON_PHYIF_CMDT_RESET
Better use value-string for that. […]
This mostly exists for debugging the order of actions here and should not really be used for anything else. It's just easier to use than signaltap or other custom markers. In practice for general purpose logging trxcon/the l1ctl handler knows what is going on anyway.
Patch Set #3, Line 352: set_ta
Shouldn't this be done on receipt of TRXCON_PHYIF_CMDT_RESET instead?
Done
Patch Set #3, Line 357: dbm = -80
please add a FIXME or TODO, so it's clear that the measurement logic is missing
Done
Patch Set #3, Line 364: gsm_arfcn2freq10
careful, this function may return 0xffff
In practice we mostly want to use different frequencies anyway, but for current testing and usage we just stick to "a" arfcn and that's it. If wrong arfcns are passed that are not part of the gsm ranges everything is broken anyway...
Patch Set #3, Line 385: assert
OSMO_ASSERT?
Done
Patch Set #3, Line 425: trxcon
it's not trxcon app anymore
but this "is" the global trxcon context used for all trxcon stuff and this is only being used by the "main loop" upper thread doing trxcon things...
pass 0 to avoid confusion, we're not using fn_advance here
Done
Patch Set #3, Line 430: gsmtap = 0;
gsmtap is a pointer, so = NULL
Done
Patch Set #3, Line 431: 0x1234
What's this? Why not NULL?
Done
I would avoid using 'trxc' here to avoid confusion with the TRXC protocol.
Done
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.
Done
Patch Set #3, Line 333: AC_MSG_NOTICE(["Enabling ms-trx..."])
spacing issues
Done
Patch Set #3, Line 364: doc/manuals/Makefile \
Ack, it's one of my fixups: https://cgit.osmocom. […]
Done
To view, visit change 30416. To unsubscribe, or for help writing mail filters, visit settings.