<p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><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 style="white-space: pre-wrap; word-wrap: break-word;">20+20+20+20+20 is not 60 lol</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 style="white-space: pre-wrap; word-wrap: break-word;">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).</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 style="white-space: pre-wrap; word-wrap: break-word;">You can probably set those around line 42 out of the body of the function (constructor initializers).</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 style="white-space: pre-wrap; word-wrap: break-word;">trailing whitespace</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 style="white-space: pre-wrap; word-wrap: break-word;">would be great checking other devices to see if we actually return true 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 style="white-space: pre-wrap; word-wrap: break-word;">so rx_path/tx_path VTY params are not really used yet?</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 style="white-space: pre-wrap; word-wrap: break-word;">trailing whitespace</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 style="white-space: pre-wrap; word-wrap: break-word;">trailing whitespace</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 style="white-space: pre-wrap; word-wrap: break-word;">db - 30 ?</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 style="white-space: pre-wrap; word-wrap: break-word;">Please fix indenting for params, they should be under the "(" character. ( in all functions!).</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 style="white-space: pre-wrap; word-wrap: break-word;">You probably need to add code to use smpl_buf in here later on, not needed for now though.</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 style="white-space: pre-wrap; word-wrap: break-word;">I think this should never happen, because Transceiver stops threads calling this function on those conditions.</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 style="white-space: pre-wrap; word-wrap: break-word;">No need for brackets 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@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 style="white-space: pre-wrap; word-wrap: break-word;">probably want to add "(Not implemented)" 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@401">Patch Set #1, Line 401:</a> <code style="font-family:monospace,monospace">bool XTRXDevice::updateAlignment(TIMESTAMP timestamp) </code></p><p style="white-space: pre-wrap; word-wrap: break-word;">trailing whitespace</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 style="white-space: pre-wrap; word-wrap: break-word;">Since requiresRadioAlign() returns false, this should never be called, right?</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 style="white-space: pre-wrap; word-wrap: break-word;">Fix indent.</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 style="white-space: pre-wrap; word-wrap: break-word;">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.</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 style="white-space: pre-wrap; word-wrap: break-word;">Do you use same names for tx/rx paths as LimeSUite? Otherwise this is wrong.</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-Comment-Date: Mon, 07 Oct 2019 18:29:15 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>