Attention is currently required from: Hoernchen, fixeria, laforge, pespin. Timur Davydov 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 5: (30 comments) File Transceiver52M/device/usdr/USDRDevice.h: https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/d686e44c_b4e80738?usp=email : PS4, Line 12: This program is distributed in the hope that it will be useful, > please keep with the "* " at the start of the line. Added the `*` prefix. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/31d6048c_a4a4a89c?usp=email : PS4, Line 46: struct DeviceParameters { > Can we use "struct dev_desc" like in all other devices I'm seeing? Yes, replaced with `struct dev_desc` for consistency with the other devices. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/fc8aab80_928d031a?usp=email : PS4, Line 48: * time correction for TX timestamp, in seconds, to align it with RX timestamp, which is needed to achieve correct > This still look like >120 chars? Fixed, wrapped to stay under 120 chars. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/a1467606_bcf1f64c?usp=email : PS4, Line 54: using DeviceParametersMap = std::map<std::string, DeviceParameters>; > Can we use dev_map_t like all other devices I'm seeing? Yes, replaced with `dev_map_t` for consistency with the other devices. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/971f01a9_f8c3ac66?usp=email : PS4, Line 86: public: > Keep order in all other device classes, where public goes first. Reordered the class members to match the structure used in the other device classes (public section first). https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/695fe674_e665d97f?usp=email : PS4, Line 115: /** > trailing whitespace Removed trailing whitespace. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/3176a15a_9aec5aa0?usp=email : PS4, Line 116: * @brief Read samples from the USDR. > imho no need to re-add all the API documentation here for an implemenetation > of a virtual function. […] Ack. I’ve removed the duplicated documentation from the implementation and submitted a separate patch improving the API documentation in `radioDevice.h` https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/5bd3f95d_c0921444?usp=email : PS4, Line 126: bool *underrun = nullptr); > align to firs tchar in "(" Aligned as suggested. File Transceiver52M/device/usdr/USDRDevice.cpp: https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/93114d84_aadb7c43?usp=email : PS4, Line 12: This program is free software: you can redistribute it and/or modify > Please keep "* " at the start and same indentation as above. Added the `*` prefix. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/c5c368ad_97b29b50?usp=email : PS4, Line 84: if (res) { > tbh I don't like exceptions being added here for no good reason. […] Removed the exceptions, switched to return code checking for consistency with other devices. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/7749812b_4102ad38?usp=email : PS4, Line 191: const int loglevel = parse_config(args, "loglevel", 3); > So AFAIU "loglevel" is not something one really the low level library […] I’ve added a logging callback similar to the other backends. However, I’d prefer not to enable full debug logging inside the low-level library and then filter everything in the callback, as this would unnecessarily spam the logging system. For now, I kept the driver parameter loglevel, so the library only emits messages up to the configured level. If VTY-based control is required, loglevel can be set to 9. Alternatively, I can implement updating the driver loglevel dynamically from VTY (if that’s feasible in this context). https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/52da6059_d49ab232?usp=email : PS4, Line 199: const double rate = 0.27083333333e6 * tx_sps; > where does this magic number come from? Can we have it in a define with some > comment? It comes from 1083333.33333 sps / 4. In fact, this corresponds to the GSM symbol rate, and I already found a `GSMRATE` constant defined. I replaced the magic number with `GSMRATE` https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/3e69f7cb_576b7bb0?usp=email : PS4, Line 203: uint64_t val; > Try putting all variable declarations at the top of the function. I've moved the variable declarations to the top of the function, except for const ones in some functions. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/8cb3406d_dc3cceb3?usp=email : PS4, Line 221: usdr_dme_set_uint_exception(dev, "/dm/sdr/0/tx/gain", 0); > Sounds like we may want to have all these paths applied as defines and macros > instead of hardcoding […] Added the suggested macros and replaced the hardcoded paths accordingly. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/2c3fa475_27e25b22?usp=email : PS4, Line 228: const char* fmt = "ci16"; > this looks like a candaidate for a define too. or a const to be put at the > start of the file. Moved it to a #define at the top of the file. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/98218450_2defb621?usp=email : PS4, Line 246: usdr_dms_sync_exception(dev, "rx", ped.size(), ped.data()); > Same, we should imho avoid all these hardcoded strings. Replaced the remaining hardcoded strings with defines. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/5c4d9331_3e37cd73?usp=email : PS4, Line 260: if (dev) { > early return: […] Implemented as suggested. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/920a6e31_3e2cd58a?usp=email : PS4, Line 269: return false; > LOGC(DDEV, ERR) << "Device already started"; Added an error message. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/429df755_eaaa6db0?usp=email : PS4, Line 296: return !started; > return true; Implemented as suggested. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/43edea18_c727c50a?usp=email : PS4, Line 327: double USDRDevice::setTxGain(double dB, size_t chan) > This function afacit is not used anywhere not required by base class? […] Removed all unused functions. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/c0efb466_353bd4bb?usp=email : PS4, Line 382: try { > I'd say better to do as other backends are doing: […] Reworked it to follow the other backends: values are now cached per channel in arrays (rx_gains, tx_gains, rx_freqs, tx_freqs, etc.). https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/d7e4dd74_21a7f0a4?usp=email : PS4, Line 392: TIMESTAMP timestamp, bool *underrun) > wrong indentation Fixed the indentation. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/ae31405e_b30f2fab?usp=email : PS4, Line 394: if (!started) > Please look at other backends and try to mimic what they are doing, eg. > checking […] Replaced the check with the same bufs.size() != chans validation as used in the other backends, for consistency. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/11759752_811e7ff2?usp=email : PS4, Line 421: bool *underrun, TIMESTAMP timestamp) > wrong indentation Fixed https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/374673e2_814771bb?usp=email : PS4, Line 475: std::string USDRDevice::getTxAntenna(size_t chan ) > extra space at the end. Removed https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/ab9b85b5_48dbb42b?usp=email : PS4, Line 492: return GSM::Time(6,7); > missing space between "," and "7". Added space https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/7fc27f08_a2f528bf?usp=email : PS4, Line 610: return new USDRDevice(iface, cfg); > Do extra checks here for this device in provided cfg, see LMSDevice. […] I don’t think additional device-specific checks are needed here. I’ve only added a basic validity check for the pointer to ensure it’s not null. File debian/osmo-trx-usdr.install: https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/a100251b_a999f647?usp=email : PS4, Line 3: /usr/bin/osmo-trx-usdr > why do some start with / and some not? It surprised me as well. I followed the existing pattern used in other Debian install files in this repository. For example: - debian/osmo-trx-uhd.install - debian/osmo-trx-lms.install - debian/osmo-trx-usrp1.install - debian/osmo-trx-ipc.install File doc/manuals/chapters/trx-backends.adoc: https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/88cf39db_2ca0cc80?usp=email : PS3, Line 86: where multiple small SDRs are combined to present one or more TRX instances > > Multi-TRX deployments are achieved by running multiple osmo-trx-usdr > > instances, typically one per […] Updated the documentation according to your remarks. File doc/manuals/chapters/trx-devices.adoc: https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/c9688264_bc0ce831?usp=email : PS4, Line 127: instances. The current _osmo-trx-usdr_ backend operates in single-channel mode. > missing new line at the end of file. Added the missing newline at the end of the file. -- 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: 5 Gerrit-Owner: Timur Davydov <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <[email protected]> Gerrit-CC: Hoernchen <[email protected]> Gerrit-CC: fixeria <[email protected]> Gerrit-CC: laforge <[email protected]> Gerrit-CC: osmith <[email protected]> Gerrit-Attention: Hoernchen <[email protected]> Gerrit-Attention: laforge <[email protected]> Gerrit-Attention: pespin <[email protected]> Gerrit-Attention: fixeria <[email protected]> Gerrit-Comment-Date: Sat, 28 Feb 2026 21:19:46 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <[email protected]> Comment-In-Reply-To: Timur Davydov <[email protected]>
