<p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/15685">View Change</a></p><p>19 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@37">Patch Set #1, Line 37:</a> <code style="font-family:monospace,monospace">static int time_tx_corr = 60; //20+20+20+20+20;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">20+20+20+20+20 is not 60 lol</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Good point.</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@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;">I don't remember now how tx_paths/rx_pathsa re generated, but I bet it's always populated to be size […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Looks like it can be 0 if we don't specify rx/tx-path in the config file.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Plus, does it really hurt to have here? Esp if no one remembers is it can be zero or not?</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;">You can probably set those around line 42 out of the body of the function (constructor initializers) […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Yes, but it looks cleaner here and doesn't affect performance.</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@173">Patch Set #1, Line 173:</a> <code style="font-family:monospace,monospace">bool XTRXDevice::start() </code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">trailing whitespace</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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;">would be great checking other devices to see if we actually return true here.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">What do you mean by "other devices" here?</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@194">Patch Set #1, Line 194:</a> <code style="font-family:monospace,monospace"> xtrx_set_antenna(device, XTRX_TX_AUTO);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">so rx_path/tx_path VTY params are not really used yet?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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@230">Patch Set #1, Line 230:</a> <code style="font-family:monospace,monospace">bool XTRXDevice::stop() </code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">trailing whitespace</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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@265">Patch Set #1, Line 265:</a> <code style="font-family:monospace,monospace">} </code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">trailing whitespace</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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;">db - 30 ?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'll ask Sergey why is it done this way but I'm sure there is a reason for that.</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@311">Patch Set #1, Line 311:</a> <code style="font-family:monospace,monospace"> TIMESTAMP timestamp, bool *underrun, unsigned *RSSI)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Please fix indenting for params, they should be under the "(" character. ( in all functions!).</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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@322">Patch Set #1, Line 322:</a> <code style="font-family:monospace,monospace"> int res = xtrx_recv_sync_ex(device, &ri);</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You probably need to add code to use smpl_buf in here later on, not needed for now though.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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;">I think this should never happen, because Transceiver stops threads calling this function on those c […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Does it hurt to check, just to be sure?</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@361">Patch Set #1, Line 361:</a> <code style="font-family:monospace,monospace"> if (*underrun) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">No need for brackets here.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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@370">Patch Set #1, Line 370:</a> <code style="font-family:monospace,monospace"> LOG(ALERT) << "CH" << chan << ": RX ANTENNA: " << ant.c_str();</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">probably want to add "(Not implemented)" here.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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@401">Patch Set #1, Line 401:</a> <code style="font-family:monospace,monospace">bool XTRXDevice::updateAlignment(TIMESTAMP timestamp) </code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">trailing whitespace</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</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@403">Patch Set #1, Line 403:</a> <code style="font-family:monospace,monospace"> LOG(ALERT) << "Update Aligment " << timestamp;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Since requiresRadioAlign() returns false, this should never be called, right?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Correct. But it's an abstract virtual function and thus must be implemented.</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@419">Patch Set #1, Line 419:</a> <code style="font-family:monospace,monospace"> << " actual freq: " << actual << std::endl;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Fix indent.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15685/1/contrib/systemd/Makefile.am">File contrib/systemd/Makefile.am:</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/contrib/systemd/Makefile.am@25">Patch Set #1, Line 25:</a> <code style="font-family:monospace,monospace">EXTRA_DIST = $(SYSTEMD_SERVICES)</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">That's wrong I'd say. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ack</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="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:</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/doc/examples/osmo-trx-xtrx/osmo-trx-xtrx.cfg@21">Patch Set #1, Line 21:</a> <code style="font-family:monospace,monospace"> tx-path BAND1</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Do you use same names for tx/rx paths as LimeSUite? Otherwise this is wrong.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</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: 1 </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: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: ipse <Alexander.Chemeris@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 08 Oct 2019 22:52:18 +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"> Gerrit-MessageType: comment </div>