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>