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/.
Hoernchen gerrit-no-reply at lists.osmocom.orgHoernchen has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-trx/+/19641 )
Change subject: osmo-trx-ipc
......................................................................
Patch Set 9:
(39 comments)
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?
Done
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/IPCDevice.cpp@82
PS7, Line 82: //unsigned int i;
> Drop it
Done
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?)
We do, Tom introduced c++11 in 2017 and no one complained about it for 3 years.
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?
This is how it currently works and there is no way to change it because the other ipc side implemented it this way and we can't really do much about it for the uhd test tool since the actual sample rate depends on the device and other settings and is dynamically looked up.
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.
Done
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
Done
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
Done
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 […]
Because they are different after all and depending on a test tool implementation for internal implementation details does not make sense.
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
Done
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.
No, because it can't be async, because we can't process messages out of order, so we could only asynchronously not handle any events while waiting.
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
Done
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 on […]
This tool is a hack for a b210, what do I care about devices no one owns that might or might not work and happen to have more than 8 distinct channels?!
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.
Done
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.
Done
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc-driver-test.c@79
PS7, Line 79: [DDEV] = {
> Wrong indent.
Done
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?
This file was not always a standaloe tool, but used to be included in a cpp file and rand as a thread so it allowed replacing direct function calls to the uhddevice with ipc channels during initial development.
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: […]
Ack
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.
Done
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
Done
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?
Done
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?
Done
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
> Agree with all log related comments from laforge.
done.
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
Done
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?
Done
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?
Done
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.
Done
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.
Ack
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/ipc_shm.c@67
PS7, Line 67: fprintf
> likewise here in this function, please use the osmocom logging framework. […]
Done
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.
Done
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?
Done
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
Done
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?
Done
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.
Ack
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/shm.c
File Transceiver52M/device/ipc/shm.c:
https://gerrit.osmocom.org/c/osmo-trx/+/19641/7/Transceiver52M/device/ipc/shm.c@40
PS7, Line 40: #ifdef ENCDECDEBUG
: fprintf(stderr, "decode: smpl_buf %d at offset %u\n", i, stream_raw->buffer_offset[i]);
: #endif
> would make sense to #define a DEBUGENCDEC() macro to avoid #ifdef/endif around every line of related […]
Done
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.
No. They serve as as reminder that you can coveniently wrap start and stop here to debug stuff.
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... […]
No, this code is used by the uhd ipc test backend, which does not have a vty or anthing else, that is why it fails and that is why it is part of this patchset.
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. […]
This is a related change because it defaults to uhd which means you can't quite build the ipc backend without uhd, even though is has no deps on uhd.
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.
done
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 ;)
yep.
--
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: 9
Gerrit-Owner: Hoernchen <ewild at sysmocom.de>
Gerrit-Reviewer: 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 15:21:05 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
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/5141d05f/attachment.htm>