ipse has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-trx/+/15685 )
Change subject: add XTRX support ...................................................................... Patch Set 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 Good point. 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 […] Looks like it can be 0 if we don't specify rx/tx-path in the config file. Plus, does it really hurt to have here? Esp if no one remembers is it can be zero or not? 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) […] Yes, but it looks cleaner here and doesn't affect performance. https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@173 PS1, Line 173: bool XTRXDevice::start() > trailing whitespace Ack 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. What do you mean by "other devices" 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? Ack https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@230 PS1, Line 230: bool XTRXDevice::stop() > trailing whitespace Ack https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@265 PS1, Line 265: } > trailing whitespace Ack 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 ? I'll ask Sergey why is it done this way but I'm sure there is a reason for that. 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!). Ack 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. Ack 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 c […] Does it hurt to check, just to be sure? https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@361 PS1, Line 361: if (*underrun) { > No need for brackets here. Ack 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. Ack https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@401 PS1, Line 401: bool XTRXDevice::updateAlignment(TIMESTAMP timestamp) > trailing whitespace Ack 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? Correct. But it's an abstract virtual function and thus must be implemented. https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@419 PS1, Line 419: << " actual freq: " << actual << std::endl; > Fix indent. Ack 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. […] Ack 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. Ok, I think we should remove these since we're ignoring them anyway. We're not specifying them for UmTRX as well since the driver knows better which path to use, based on frequency. -- 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-CC: ipse <alexander.cheme...@gmail.com> Gerrit-Comment-Date: Tue, 08 Oct 2019 22:52:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pes...@sysmocom.de> Gerrit-MessageType: comment
