pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-trx/+/15685 )
Change subject: Initial XTRX support ...................................................................... Patch Set 3: (5 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@49 PS1, Line 49: << " rx_path(0): " << (rx_paths.size() ? rx_paths[0] : "<>") > Looks like it can be 0 if we don't specify rx/tx-path in the config file. […] in trx_start() (osmo-trx.cpp): /* Generate vector of rx/tx_path: */ for (i = 0; i < trx->cfg.num_chans; i++) { rx_paths.push_back(charp2str(trx->cfg.chans[i].rx_path)); tx_paths.push_back(charp2str(trx->cfg.chans[i].tx_path)); } So it cannot be size()==0 unless there's cnum_chans=0, which shouldn't happen. IMHO having this check here is unnecesary and confusing for people reading the code. https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@52 PS1, Line 52: txsps = tx_sps; > Yes, but it looks cleaner here and doesn't affect performance. I disagree, but it's bike-shed talking anyway, so keep it this way if you prefer, np. https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@177 PS1, Line 177: return false; > What do you mean by "other devices" here? Other device implementations. For instance UHDDevice does: if (started) { LOGC(DDEV, ERROR) << "Device already started"; return false; } So fine here :) Maybe wanna add a LOGC like UHDDevice. 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); > I'll ask Sergey why is it done this way but I'm sure there is a reason for > that. Then please document so. https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@345 PS1, Line 345: if (!started) > Does it hurt to check, just to be sure? It adds confusing about how the system is expected to work, and makes it difficult to refactor later (because looks like different devices expect the generic framework to work in different ways). For instance UHDDevice doesn't do this kind of check. -- 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: 3 Gerrit-Owner: rauf.gyulal...@fairwaves.co Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: ipse <alexander.cheme...@gmail.com> Gerrit-Reviewer: pespin <pes...@sysmocom.de> Gerrit-Comment-Date: Wed, 09 Oct 2019 11:43:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pes...@sysmocom.de> Comment-In-Reply-To: ipse <alexander.cheme...@gmail.com> Gerrit-MessageType: comment
