pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-trx/+/15685 )
Change subject: add XTRX support ...................................................................... Patch Set 1: Code-Review-1 (19 comments) https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp File Transceiver52M/device/xtrx/XTRXDevice.cpp: https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@37 PS1, Line 37: static int time_tx_corr = 60; //20+20+20+20+20; 20+20+20+20+20 is not 60 lol https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@49 PS1, Line 49: << " rx_path(0): " << (rx_paths.size() ? rx_paths[0] : "<>") I don't remember now how tx_paths/rx_pathsa re generated, but I bet it's always populated to be size() == chans, and I doubt chans can be zero (int that case we should exit way before). https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@52 PS1, Line 52: txsps = tx_sps; You can probably set those around line 42 out of the body of the function (constructor initializers). https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@173 PS1, Line 173: bool XTRXDevice::start() trailing whitespace https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@177 PS1, Line 177: return false; would be great checking other devices to see if we actually return true here. https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@194 PS1, Line 194: xtrx_set_antenna(device, XTRX_TX_AUTO); so rx_path/tx_path VTY params are not really used yet? https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@230 PS1, Line 230: bool XTRXDevice::stop() trailing whitespace https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@265 PS1, Line 265: } trailing whitespace https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@280 PS1, Line 280: int res = xtrx_set_gain(device, XTRX_CH_AB, XTRX_TX_PAD_GAIN, dB - 30, &txGain); db - 30 ? https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@311 PS1, Line 311: TIMESTAMP timestamp, bool *underrun, unsigned *RSSI) Please fix indenting for params, they should be under the "(" character. ( in all functions!). https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@322 PS1, Line 322: int res = xtrx_recv_sync_ex(device, &ri); You probably need to add code to use smpl_buf in here later on, not needed for now though. https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@345 PS1, Line 345: if (!started) I think this should never happen, because Transceiver stops threads calling this function on those conditions. https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@361 PS1, Line 361: if (*underrun) { No need for brackets here. https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@370 PS1, Line 370: LOG(ALERT) << "CH" << chan << ": RX ANTENNA: " << ant.c_str(); probably want to add "(Not implemented)" here. https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@401 PS1, Line 401: bool XTRXDevice::updateAlignment(TIMESTAMP timestamp) trailing whitespace https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@403 PS1, Line 403: LOG(ALERT) << "Update Aligment " << timestamp; Since requiresRadioAlign() returns false, this should never be called, right? https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@419 PS1, Line 419: << " actual freq: " << actual << std::endl; Fix indent. https://gerrit.osmocom.org/#/c/15685/1/contrib/systemd/Makefile.am File contrib/systemd/Makefile.am: https://gerrit.osmocom.org/#/c/15685/1/contrib/systemd/Makefile.am@25 PS1, Line 25: EXTRA_DIST = $(SYSTEMD_SERVICES) That's wrong I'd say. You want to keep all files in EXTRA_DIST, because someone else may decide to compile with different TRX device support from the archive later on, so all of themm need to be available. https://gerrit.osmocom.org/#/c/15685/1/doc/examples/osmo-trx-xtrx/osmo-trx-xtrx.cfg File doc/examples/osmo-trx-xtrx/osmo-trx-xtrx.cfg: https://gerrit.osmocom.org/#/c/15685/1/doc/examples/osmo-trx-xtrx/osmo-trx-xtrx.cfg@21 PS1, Line 21: tx-path BAND1 Do you use same names for tx/rx paths as LimeSUite? Otherwise this is wrong. -- To view, visit https://gerrit.osmocom.org/c/osmo-trx/+/15685 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-trx Gerrit-Branch: master Gerrit-Change-Id: I1067dfef53aa2669cc7c189cccae10074c674390 Gerrit-Change-Number: 15685 Gerrit-PatchSet: 1 Gerrit-Owner: rauf.gyulal...@fairwaves.co Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Mon, 07 Oct 2019 18:29:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
