pespin 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 <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <[email protected]> Gerrit-CC: fixeria <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-Comment-Date: Mon, 17 Aug 2020 13:20:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: laforge <[email protected]> Gerrit-MessageType: comment
