<p style="white-space: pre-wrap; word-wrap: break-word;">Mostly few lines general cleanup and a couple things which should go in a seaprate commit. Should be quick to apply and then I'm fine with it.</p><p>Patch set 8:<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/+/19641">View Change</a></p><p>37 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp">File Transceiver52M/device/ipc/IPCDevice.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/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@66">Patch Set #7, Line 66:</a> <code style="font-family:monospace,monospace"> //m_IPC_stream_rx.resize(chans);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can be dropped?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@82">Patch Set #7, Line 82:</a> <code style="font-family:monospace,monospace"> //unsigned int i;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Drop it</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@94">Patch Set #7, Line 94:</a> <code style="font-family:monospace,monospace"> for (auto i : shm_io_rx_streams)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">That sounds like c++11, do we want to depend on it? (do we already depend on it?)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@210">Patch Set #7, Line 210:</a> <code style="font-family:monospace,monospace"> /* FIXME: this is actually the sps value, not the sample rate!</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">What about this?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@227">Patch Set #7, Line 227:</a> <code style="font-family:monospace,monospace"> /* FIXME: clock ref part of config, not open */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">ACK, can be fixed later.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@375">Patch Set #7, Line 375:</a> <code style="font-family:monospace,monospace"> // shm_io_tx_streams.push_back(ipc_shm_init_producer(shm_dec->channels[i]->dl_stream));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can be dropped</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@510">Patch Set #7, Line 510:</a> <code style="font-family:monospace,monospace"> //struct ipc_sk_if *ipc_prim = (struct ipc_sk_if *) msg->data;</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/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@653">Patch Set #7, Line 653:</a> <code style="font-family:monospace,monospace">int IPCDevice::ipc_sock_write(struct osmo_fd *bfd)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">So AFAIU we reimplement all the sock stuff here and we have almost same implementation used for the Driver in a seaprate C file? Why?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@735">Patch Set #7, Line 735:</a> <code style="font-family:monospace,monospace"> /* _after_ we send it, we can deueue */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">And third typo: dequeue</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@837">Patch Set #7, Line 837:</a> <code style="font-family:monospace,monospace">void IPCDevice::manually_poll_sock_fds()</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We should really make all this async later in the future.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@1163">Patch Set #7, Line 1163:</a> <code style="font-family:monospace,monospace"> //expect_smpls = len - avail_smpls;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can be dropped</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.h">File Transceiver52M/device/ipc/ipc-driver-test.h:</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/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.h@23">Patch Set #7, Line 23:</a> <code style="font-family:monospace,monospace">/* 8 channels are plenty */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Ugh that's one per TRX right? Cannot we have more with some devices? (I know, I may have been the one myself writing this line when doing the prototype a while ago).</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.h@30">Patch Set #7, Line 30:</a> <code style="font-family:monospace,monospace"> struct osmo_fd conn_bfd; /* fd for connection to lcr */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">lcr? that's probably some leftover from some old code.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c">File Transceiver52M/device/ipc/ipc-driver-test.c:</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/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@70">Patch Set #7, Line 70:</a> <code style="font-family:monospace,monospace">//enum { DMAIN,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can br dropped.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@79">Patch Set #7, Line 79:</a> <code style="font-family:monospace,monospace"> [DDEV] = {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Wrong indent.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@91">Patch Set #7, Line 91:</a> <code style="font-family:monospace,monospace">#ifdef __cplusplus</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why is this here? Shouldn't it be a lot higher?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@199">Patch Set #7, Line 199:</a> <code style="font-family:monospace,monospace"> /* FIXME: dynamc chan limit, currently 8 */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">typo dynamic:<br>We are using "8" as limit in several places, may be worth adding a define for it.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@254">Patch Set #7, Line 254:</a> <code style="font-family:monospace,monospace">volatile int ul_running = 0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">better use bool with true/false for boolean, it's clearer then that it's a boolean variable.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@261">Patch Set #7, Line 261:</a> <code style="font-family:monospace,monospace"> pthread_setname_np(pthread_self(), "uplink rx");</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'd avoid spaces in thread names, it's confusing and you save chars (only 15 available): UplinkRx</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@418">Patch Set #7, Line 418:</a> <code style="font-family:monospace,monospace"> //osmo_signal_register_handler(SS_GLOBAL, IPC_if_signal_cb, NULL);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can be dropped?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@496">Patch Set #7, Line 496:</a> <code style="font-family:monospace,monospace"> //ipc_sock_close()</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can be uncommented?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_chan.c">File Transceiver52M/device/ipc/ipc_chan.c:</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/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_chan.c@52">Patch Set #7, Line 52:</a> <code style="font-family:monospace,monospace">fprintf</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">can we migrate all of those fprintf/printf statements to the osmocom logging API, please?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Agree with all log related comments from laforge.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_chan.c@193">Patch Set #7, Line 193:</a> <code style="font-family:monospace,monospace"> /* _after_ we send it, we can deueue */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">typo: dequeue</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_chan.c@265">Patch Set #7, Line 265:</a> <code style="font-family:monospace,monospace"> //IPC_tx_info_ind();</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">what about this?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_shm.h">File Transceiver52M/device/ipc/ipc_shm.h:</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/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_shm.h@31">Patch Set #7, Line 31:</a> <code style="font-family:monospace,monospace"> volatile struct ipc_shm_raw_stream *this_stream; // plus num_buffers at end</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I don't see num_buffers anywhere nearby?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_shm.c">File Transceiver52M/device/ipc/ipc_shm.c:</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/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_shm.c@34">Patch Set #7, Line 34:</a> <code style="font-family:monospace,monospace">#define SAMPLE_SIZE_BYTE sizeof(uint16_t) * 2</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">better adding () around here.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_shm.c@36">Patch Set #7, Line 36:</a> <code style="font-family:monospace,monospace">struct ipc_shm_io *ipc_shm_init_consumer(struct ipc_shm_stream *s)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">A bit of documentation in functions would be welcome at some point.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_shm.c@120">Patch Set #7, Line 120:</a> <code style="font-family:monospace,monospace"> if (r->buf_ptrs)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">free already checks for NULL, no need to check.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_sock.c">File Transceiver52M/device/ipc/ipc_sock.c:</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/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_sock.c@70">Patch Set #7, Line 70:</a> <code style="font-family:monospace,monospace"> //struct ipc_sk_if *ipc_prim = (struct ipc_sk_if *) msg->data;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This line can be dropped?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_sock.c@200">Patch Set #7, Line 200:</a> <code style="font-family:monospace,monospace"> /* _after_ we send it, we can deueue */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">typo: dequeue</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_sock.c@270">Patch Set #7, Line 270:</a> <code style="font-family:monospace,monospace"> //IPC_tx_info_ind();</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Can be dropped?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/shm.h">File Transceiver52M/device/ipc/shm.h:</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/osmo-trx/+/19641/7/Transceiver52M/device/ipc/shm.h@33">Patch Set #7, Line 33:</a> <code style="font-family:monospace,monospace"> pthread_cond_t cf;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">May be worth documenting all these conditional variables.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/uhdwrap.h">File Transceiver52M/device/ipc/uhdwrap.h:</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/osmo-trx/+/19641/7/Transceiver52M/device/ipc/uhdwrap.h@46">Patch Set #7, Line 46:</a> <code style="font-family:monospace,monospace"> // bool start() override;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Drop all the commented lines.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/uhd/UHDDevice.cpp">File Transceiver52M/device/uhd/UHDDevice.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/osmo-trx/+/19641/7/Transceiver52M/device/uhd/UHDDevice.cpp@155">Patch Set #7, Line 155:</a> <code style="font-family:monospace,monospace"> osmo_cpu_sched_vty_apply_localthread();</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Non related, must be in a separate commit. BTW, why does it fail? It shouldn't... removing the assert doesn't look like the correct fix.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/configure.ac">File configure.ac:</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/osmo-trx/+/19641/7/configure.ac@246">Patch Set #7, Line 246:</a> <code style="font-family:monospace,monospace">AM_CONDITIONAL(DEVICE_UHD, [test "x$with_uhd" == "xyes"])</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This change looks non related, please submit a separate patch. BTW, why "==" while others use "="?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/contrib/osmo-trx.spec.in">File contrib/osmo-trx.spec.in:</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/osmo-trx/+/19641/7/contrib/osmo-trx.spec.in@239">Patch Set #7, Line 239:</a> <code style="font-family:monospace,monospace"># FIXME: missing: osmo-trx-ipc.cfg</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please add the file to the commit instead of adding a FIXME here.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/debian/osmo-trx-ipc.install">File debian/osmo-trx-ipc.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/osmo-trx/+/19641/7/debian/osmo-trx-ipc.install@4">Patch Set #7, Line 4:</a> <code style="font-family:monospace,monospace">/usr/share/doc/osmo-trx/examples/osmo-trx-ipc/osmo-trx-ipc.cfg /usr/share/doc/osmo-trx/examples/osmo-trx-ipc/</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">So the cfg file actually exists in the commit, then drop the FIXME line I commented on ;)</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-trx/+/19641">change 19641</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/+/19641"/><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: Ice63d3499026293ade8aad675ff7a883bcdd5756 </div>
<div style="display:none"> Gerrit-Change-Number: 19641 </div>
<div style="display:none"> Gerrit-PatchSet: 8 </div>
<div style="display:none"> Gerrit-Owner: Hoernchen <ewild@sysmocom.de> </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: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 17 Aug 2020 13:20:59 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Comment-In-Reply-To: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>