<p style="white-space: pre-wrap; word-wrap: break-word;">Some comments after quick review.</p><p><a href="https://gerrit.osmocom.org/14018">View Change</a></p><p>11 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/14018/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/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@44">Patch Set #1, Line 44:</a> <code style="font-family:monospace,monospace"> LOG(INFO) << "creating XTRX device:"</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You should be using category DEV here, not MAIN (see other devices).</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@162">Patch Set #1, Line 162:</a> <code style="font-family:monospace,monospace"> return NORMAL;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This NORMAL thing here makes no sense in the context of this function afaik.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@167">Patch Set #1, Line 167:</a> <code style="font-family:monospace,monospace"> if (device) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Drop {}</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@175">Patch Set #1, Line 175:</a> <code style="font-family:monospace,monospace"> if (started) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Drop {}</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@212">Patch Set #1, Line 212:</a> <code style="font-family:monospace,monospace"> if (loopback) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Drop {}</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@231">Patch Set #1, Line 231:</a> <code style="font-family:monospace,monospace"> if (started) {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">early return: if (!started) reutrn false;</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@278">Patch Set #1, Line 278:</a> <code style="font-family:monospace,monospace"> LOG(NOTICE) << "Setting TX gain to " << dB << " dB.";</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">LOGCHAN</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@282">Patch Set #1, Line 282:</a> <code style="font-family:monospace,monospace"> LOG(ERR) << "Error setting TX gain res: " << res;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">LOGCHAN. LOGCHAN everywhere where "chan" is passed as parameter in this file.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@319">Patch Set #1, Line 319:</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;">Looks like we need to add smpl_buf usage in here (see my latest commits merged).</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/14018/1/debian/osmo-trx-xtrx.install">File debian/osmo-trx-xtrx.install:</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/14018/1/debian/osmo-trx-xtrx.install@3">Patch Set #1, Line 3:</a> <code style="font-family:monospace,monospace">/usr/bin/osmo-trx-xtrx</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">why some strt with / and some doesn't? Unify style if possible.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/14018/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/14018/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;">Are you sure this belongs here? looks copied from LMS file.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/14018">change 14018</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/14018"/><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-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Iad73e0611e7951d5bcfcc918063cc3778cb1dd8f </div>
<div style="display:none"> Gerrit-Change-Number: 14018 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: lynxis lazus <lynxis@fe80.eu> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-CC: Pau Espin Pedrol <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 13 May 2019 16:27:58 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: No </div>