Attention is currently required from: Timur Davydov. pespin has posted comments on this change by Timur Davydov. ( https://gerrit.osmocom.org/c/osmo-trx/+/42198?usp=email )
Change subject: feat(usdr): add USDR backend support (osmo-trx-usdr) ...................................................................... Patch Set 3: Code-Review-1 (16 comments) Patchset: PS3: This was not a full review, I will review USDRDevice.cpp/.h again once you fix all the formatting there. File Transceiver52M/device/usdr/USDRDevice.h: https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/af0bce02_b674c263?usp=email : PS3, Line 54: int txsps; ///< samples count per GSM symbol for Tx unsigned? We actually have "size_t tx_sps, rx_sps;" in RadioDevice, why not reusing them? https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/8b925d17_1efc289a?usp=email : PS3, Line 60: int timeTxCorr = 0; ///< time correction for TX timestamp, in samples, to align it with RX timestamp, which is needed to achieve correct timing of the transmitted signal. This is a device-specific parameter that can be configured via dev_args and defaults to 0 if not specified. Change all these comments using /* */ and properly split into acceptable width (<=120 chars). https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/20940c5f_f75af47f?usp=email : PS3, Line 63: TIMESTAMP ts_last_recv = 0; wrong indentation https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/12705b92_a891e300?usp=email : PS3, Line 90: Read samples from the USDR. syntax for comment (at least indentation) is different here and in next block. We usually tend to have "* yourtexthere" at the start of each comment line. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/4574676b_9f77251b?usp=email : PS3, Line 100: TIMESTAMP timestamp = 0xffffffff, bool *underrun = NULL); wrong indentation here and below. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/abf0fde4_f08d4af3?usp=email : PS3, Line 121: Pleasr get rid of trailing whitespace. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/460c17d8_fb1f28b7?usp=email : PS3, Line 132: TIMESTAMP initialReadTimestamp(void) { return ts_initial;} Please, fix indentation in the whole file. File Transceiver52M/device/usdr/USDRDevice.cpp: https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/3ce342ee_589c89f5?usp=email : PS3, Line 49: {"limemini", {tx_offset: 86.769us}}, /* 94 samples at 1083333.33333 sps corresponds to 86.769 us */ are you sure this is correct? isn't it xsdr? maybe add an extra commit explaining? https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/5d746f23_0af4c7af?usp=email : PS3, Line 52: enum { I don't see why this should be an enum. Looks more like 2 defines or 2 const unsigned int. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/6f8ee7e3_0dd68be8?usp=email : PS3, Line 85: std::string msg = std::string("Error creating USDR device with connection string '") + connection_string + "' res: " + std::to_string(res) + "(" + strerror(res) + ")"; Fix all lines too long like this. Maximum is 120 chars. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/b6291fd4_84385229?usp=email : PS3, Line 180: dev = NULL; Please fix indentation in the whole file. Better use tabs everywhere. File debian/osmo-trx-usdr.postinst: https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/165e1499_770932f1?usp=email : PS3, Line 18: # Fix permissions of previous (root-owned) install (OS#4107) This is AFAICT not needed here, because there's no previous root-owned install ;) File doc/manuals/chapters/trx-backends.adoc: https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/fad6a46c_77479109?usp=email : PS3, Line 85: The backend supports single-channel SDR hardware and is designed for setups But in the other .adoc (devices) you say xSDR supports multiple channels? Is this simply not implemented yet in osmo-trx? https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/6486201c_860cc643?usp=email : PS3, Line 86: where multiple small SDRs are combined to present one or more TRX instances You mean using multiple SDR devices controlled by one osmo-trx-usdr process? Or multiple SDR devices with multiple osmo-trx-usdr processes each controlling 1 device? The later is not supported and I can foresee clock sync issues. The former is not supported at least for other backends, dunno for USDR yet (still reviewing). File doc/manuals/chapters/trx-devices.adoc: https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/08cb6d24_d39ad6ef?usp=email : PS3, Line 102: scale the number of TRX instances. I'd welcome some extra documentation here regarding how "multi-unit setups to scale the number of TRX instances." is accomplished, or at least some link to somewhere explaining it. -- To view, visit https://gerrit.osmocom.org/c/osmo-trx/+/42198?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: osmo-trx Gerrit-Branch: master Gerrit-Change-Id: I5d1d25921514954c4929ae6e7352168b3ceb05df Gerrit-Change-Number: 42198 Gerrit-PatchSet: 3 Gerrit-Owner: Timur Davydov <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <[email protected]> Gerrit-Attention: Timur Davydov <[email protected]> Gerrit-Comment-Date: Wed, 25 Feb 2026 10:10:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes
