This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
pespin gerrit-no-reply at lists.osmocom.orgpespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-trx/+/19641 )
Change subject: osmo-trx-ipc
......................................................................
Patch Set 8: Code-Review-1
(37 comments)
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.
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp
File Transceiver52M/device/ipc/IPCDevice.cpp:
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@66
PS7, Line 66: //m_IPC_stream_rx.resize(chans);
Can be dropped?
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@82
PS7, Line 82: //unsigned int i;
Drop it
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@94
PS7, Line 94: for (auto i : shm_io_rx_streams)
That sounds like c++11, do we want to depend on it? (do we already depend on it?)
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@210
PS7, Line 210: /* FIXME: this is actually the sps value, not the sample rate!
What about this?
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@227
PS7, Line 227: /* FIXME: clock ref part of config, not open */
ACK, can be fixed later.
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@375
PS7, Line 375: // shm_io_tx_streams.push_back(ipc_shm_init_producer(shm_dec->channels[i]->dl_stream));
Can be dropped
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@510
PS7, Line 510: //struct ipc_sk_if *ipc_prim = (struct ipc_sk_if *) msg->data;
Drop
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@653
PS7, Line 653: int IPCDevice::ipc_sock_write(struct osmo_fd *bfd)
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?
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@735
PS7, Line 735: /* _after_ we send it, we can deueue */
And third typo: dequeue
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@837
PS7, Line 837: void IPCDevice::manually_poll_sock_fds()
We should really make all this async later in the future.
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@1163
PS7, Line 1163: //expect_smpls = len - avail_smpls;
Can be dropped
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.h
File Transceiver52M/device/ipc/ipc-driver-test.h:
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.h@23
PS7, Line 23: /* 8 channels are plenty */
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).
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.h@30
PS7, Line 30: struct osmo_fd conn_bfd; /* fd for connection to lcr */
lcr? that's probably some leftover from some old code.
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c
File Transceiver52M/device/ipc/ipc-driver-test.c:
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@70
PS7, Line 70: //enum { DMAIN,
Can br dropped.
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@79
PS7, Line 79: [DDEV] = {
Wrong indent.
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@91
PS7, Line 91: #ifdef __cplusplus
Why is this here? Shouldn't it be a lot higher?
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@199
PS7, Line 199: /* FIXME: dynamc chan limit, currently 8 */
typo dynamic:
We are using "8" as limit in several places, may be worth adding a define for it.
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@254
PS7, Line 254: volatile int ul_running = 0;
better use bool with true/false for boolean, it's clearer then that it's a boolean variable.
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@261
PS7, Line 261: pthread_setname_np(pthread_self(), "uplink rx");
I'd avoid spaces in thread names, it's confusing and you save chars (only 15 available): UplinkRx
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@418
PS7, Line 418: //osmo_signal_register_handler(SS_GLOBAL, IPC_if_signal_cb, NULL);
Can be dropped?
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@496
PS7, Line 496: //ipc_sock_close()
Can be uncommented?
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_chan.c
File Transceiver52M/device/ipc/ipc_chan.c:
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_chan.c@52
PS7, Line 52: fprintf
> can we migrate all of those fprintf/printf statements to the osmocom logging API, please?
Agree with all log related comments from laforge.
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_chan.c@193
PS7, Line 193: /* _after_ we send it, we can deueue */
typo: dequeue
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_chan.c@265
PS7, Line 265: //IPC_tx_info_ind();
what about this?
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_shm.h
File Transceiver52M/device/ipc/ipc_shm.h:
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_shm.h@31
PS7, Line 31: volatile struct ipc_shm_raw_stream *this_stream; // plus num_buffers at end
I don't see num_buffers anywhere nearby?
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_shm.c
File Transceiver52M/device/ipc/ipc_shm.c:
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_shm.c@34
PS7, Line 34: #define SAMPLE_SIZE_BYTE sizeof(uint16_t) * 2
better adding () around here.
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_shm.c@36
PS7, Line 36: struct ipc_shm_io *ipc_shm_init_consumer(struct ipc_shm_stream *s)
A bit of documentation in functions would be welcome at some point.
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_shm.c@120
PS7, Line 120: if (r->buf_ptrs)
free already checks for NULL, no need to check.
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_sock.c
File Transceiver52M/device/ipc/ipc_sock.c:
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_sock.c@70
PS7, Line 70: //struct ipc_sk_if *ipc_prim = (struct ipc_sk_if *) msg->data;
This line can be dropped?
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_sock.c@200
PS7, Line 200: /* _after_ we send it, we can deueue */
typo: dequeue
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_sock.c@270
PS7, Line 270: //IPC_tx_info_ind();
Can be dropped?
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/shm.h
File Transceiver52M/device/ipc/shm.h:
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/shm.h@33
PS7, Line 33: pthread_cond_t cf;
May be worth documenting all these conditional variables.
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/uhdwrap.h
File Transceiver52M/device/ipc/uhdwrap.h:
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/uhdwrap.h@46
PS7, Line 46: // bool start() override;
Drop all the commented lines.
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/uhd/UHDDevice.cpp
File Transceiver52M/device/uhd/UHDDevice.cpp:
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/uhd/UHDDevice.cpp@155
PS7, Line 155: osmo_cpu_sched_vty_apply_localthread();
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.
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/configure.ac
File configure.ac:
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/configure.ac@246
PS7, Line 246: AM_CONDITIONAL(DEVICE_UHD, [test "x$with_uhd" == "xyes"])
This change looks non related, please submit a separate patch. BTW, why "==" while others use "="?
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/contrib/osmo-trx.spec.in
File contrib/osmo-trx.spec.in:
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/contrib/osmo-trx.spec.in@239
PS7, Line 239: # FIXME: missing: osmo-trx-ipc.cfg
Please add the file to the commit instead of adding a FIXME here.
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/debian/osmo-trx-ipc.install
File debian/osmo-trx-ipc.install:
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/debian/osmo-trx-ipc.install@4
PS7, Line 4: /usr/share/doc/osmo-trx/examples/osmo-trx-ipc/osmo-trx-ipc.cfg /usr/share/doc/osmo-trx/examples/osmo-trx-ipc/
So the cfg file actually exists in the commit, then drop the FIXME line I commented on ;)
--
To view, visit https://gerrit.osmocom.org/c/osmo-trx/+/19641
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-trx
Gerrit-Branch: master
Gerrit-Change-Id: Ice63d3499026293ade8aad675ff7a883bcdd5756
Gerrit-Change-Number: 19641
Gerrit-PatchSet: 8
Gerrit-Owner: Hoernchen <ewild at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-CC: fixeria <vyanitskiy at sysmocom.de>
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-Comment-Date: Mon, 17 Aug 2020 13:20:59 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Comment-In-Reply-To: laforge <laforge at osmocom.org>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200817/7ae799e7/attachment.htm>