<p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/15685">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp">File Transceiver52M/device/xtrx/XTRXDevice.cpp:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@49">Patch Set #1, Line 49:</a> <code style="font-family:monospace,monospace"> << " rx_path(0): " << (rx_paths.size() ? rx_paths[0] : "<>")</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Looks like it can be 0 if we don't specify rx/tx-path in the config file. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">in trx_start() (osmo-trx.cpp):</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> /* Generate vector of rx/tx_path: */<br> for (i = 0; i < trx->cfg.num_chans; i++) {<br> rx_paths.push_back(charp2str(trx->cfg.chans[i].rx_path));<br> tx_paths.push_back(charp2str(trx->cfg.chans[i].tx_path));<br> }</pre><p style="white-space: pre-wrap; word-wrap: break-word;">So it cannot be size()==0 unless there's cnum_chans=0, which shouldn't happen.</p><p style="white-space: pre-wrap; word-wrap: break-word;">IMHO having this check here is unnecesary and confusing for people reading the code.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@52">Patch Set #1, Line 52:</a> <code style="font-family:monospace,monospace"> txsps = tx_sps;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Yes, but it looks cleaner here and doesn't affect performance.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I disagree, but it's bike-shed talking anyway, so keep it this way if you prefer, np.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@177">Patch Set #1, Line 177:</a> <code style="font-family:monospace,monospace"> return false;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">What do you mean by "other devices" here?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Other device implementations. For instance UHDDevice does:</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;"> if (started) {<br> LOGC(DDEV, ERROR) << "Device already started";<br> return false;<br> }</pre><p style="white-space: pre-wrap; word-wrap: break-word;">So fine here :) Maybe wanna add a LOGC like UHDDevice.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@280">Patch Set #1, Line 280:</a> <code style="font-family:monospace,monospace"> int res = xtrx_set_gain(device, XTRX_CH_AB, XTRX_TX_PAD_GAIN, dB - 30, &txGain);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I'll ask Sergey why is it done this way but I'm sure there is a reason for that.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Then please document so.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@345">Patch Set #1, Line 345:</a> <code style="font-family:monospace,monospace"> if (!started)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Does it hurt to check, just to be sure?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-trx/+/15685">change 15685</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-trx/+/15685"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-trx </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I1067dfef53aa2669cc7c189cccae10074c674390 </div>
<div style="display:none"> Gerrit-Change-Number: 15685 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: rauf.gyulaliev@fairwaves.co </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: ipse <Alexander.Chemeris@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Wed, 09 Oct 2019 11:43:41 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Comment-In-Reply-To: ipse <Alexander.Chemeris@gmail.com> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>