Attention is currently required from: Hoernchen, Timur Davydov, fixeria, laforge. 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 4: (31 comments) Patchset: PS4: In general, I urge you to look at other existing backends like LMSDevice, UHDDevice and blade_device and try to mimic what they are doing, eg. parameter checking, lifecycle, logging, etc. Let's try to unify them as much as possible to improving something in one backend makes it possible to improve it in other easily too. File Transceiver52M/device/usdr/USDRDevice.h: https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/bbfac51e_d91e74a2?usp=... : 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. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/9e82342d_64c3c7e5?usp=... : PS4, Line 46: struct DeviceParameters { Can we use "struct dev_desc" like in all other devices I'm seeing? https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/8f72abbe_1f5cf2bf?usp=... : 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? https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/5666b94e_9102dd6b?usp=... : PS4, Line 54: using DeviceParametersMap = std::map<std::string, DeviceParameters>; Can we use dev_map_t like all other devices I'm seeing? https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/69dd1797_4c72ab51?usp=... : PS4, Line 86: public: Keep order in all other device classes, where public goes first. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/6c48220d_db8f093a?usp=... : PS4, Line 115: /** trailing whitespace https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/4d38b381_f3ad5bc4?usp=... : 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. I'd say remove them here and submit a separate patch improving documentation in radioDevice.h for those virtual functions. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/06e63029_8a06b71b?usp=... : PS4, Line 126: bool *underrun = nullptr); align to firs tchar in "(" File Transceiver52M/device/usdr/USDRDevice.cpp: https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/bc8a6933_38b033db?usp=... : 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. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/de1b6b46_552349cf?usp=... : PS4, Line 84: if (res) { tbh I don't like exceptions being added here for no good reason. All other devices use return code checking just fine, and actually the usdr API seems to be returning that kind of errors too. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/a856e591_fce55d86?usp=... : PS4, Line 191: const int loglevel = parse_config(args, "loglevel", 3); So AFAIU "loglevel" is not something one really the low level library ingests, but something you are using here to program the library. IMHO this should be controlled by vty config setting log level of DEVDRV as done in other backends, eg "log level devdrv info". Can't you actually get a callback function to be called when logging is to happen inside the library? see how it's done for instance in LMSDevice.cpp lms_log_callback(). https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/7e7e1d0c_5ab0fc62?usp=... : 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? https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/20e60e46_480f4eef?usp=... : PS4, Line 203: uint64_t val; Try putting all variable declarations at the top of the function. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/774dcff0_75523811?usp=... : 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 them N times. eg: #define DM_SDR(idx) "/dm/sdr/" OSMO_STRINGIFY_VAL(idx) #define DM_SDR_TX_GAIN(idx) DM_SDR(idx) "/tx/gain" #define DM_SDR_RX_GAIN(idx) DM_SDR(idx) "/rx/gain" #define DM_SDR_TX_BW(idx) DM_SDR(idx) "/tx/bandwidth" #define DM_SDR_RX_BW(idx) DM_SDR(idx) "/rx/bandwidth" #define LL_DEVICE_NAME "/ll/device/name" #define LL_STX(idx) "/ll/stx/" OSMO_STRINGIFY_VAL(idx) #define LL_SRX(idx) "/ll/srx/" OSMO_STRINGIFY_VAL(idx) https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/46749fb3_75d524e4?usp=... : 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. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/bc277d19_8652f704?usp=... : PS4, Line 246: usdr_dms_sync_exception(dev, "rx", ped.size(), ped.data()); Same, we should imho avoid all these hardcoded strings. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/2487f410_a49bed6a?usp=... : PS4, Line 260: if (dev) { early return: if (!dev) return; https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/69e35f29_017473a0?usp=... : PS4, Line 269: return false; LOGC(DDEV, ERR) << "Device already started"; https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/f3ce2912_b2f5b2ad?usp=... : PS4, Line 296: return !started; return true; https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/beffed8d_7f4fb397?usp=... : PS4, Line 327: double USDRDevice::setTxGain(double dB, size_t chan) This function afacit is not used anywhere not required by base class? Leave it under "#if 0" if that's the case. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/9136098b_664bd3ec?usp=... : PS4, Line 382: try { I'd say better to do as other backends are doing: Cache the values into an array per chan in eg. rx_gains, tx_gains, rx_freqs, tx_freqs, etc. Let's try to keep all backends as similar as posibble to ease maintenance and future feature development. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/7cee77c2_deb93f59?usp=... : PS4, Line 392: TIMESTAMP timestamp, bool *underrun) wrong indentation https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/0bbc2246_b0a98687?usp=... : PS4, Line 394: if (!started) Please look at other backends and try to mimic what they are doing, eg. checking if (bufs.size() != chans) { LOGC(DDEV, ALERT) << "Invalid channel combination " << bufs.size(); return -1; } instead of what you check here, unless there's a good reason. Let's try to keep all backends similar so it's easy to maintain. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/86e63165_9cd81e09?usp=... : PS4, Line 421: bool *underrun, TIMESTAMP timestamp) wrong indentation https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/51e84e4e_6e80974d?usp=... : PS4, Line 475: std::string USDRDevice::getTxAntenna(size_t chan ) extra space at the end. https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/e295efb1_39dc6a41?usp=... : PS4, Line 492: return GSM::Time(6,7); missing space between "," and "7". https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/205827a9_ecce79a8?usp=... : PS4, Line 610: return new USDRDevice(iface, cfg); Do extra checks here for this device in provided cfg, see LMSDevice.cpp File debian/osmo-trx-usdr.install: https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/ae98245f_98e51337?usp=... : PS4, Line 3: /usr/bin/osmo-trx-usdr why do some start with / and some not? File doc/manuals/chapters/trx-backends.adoc: https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/874b912d_a11d740f?usp=... : 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 device.

AFAIK this is not supported by osmo-bts-trx, one osmo-trx instance = 1 bts. Am I missing or not remembering something with regards to this topic?

the Osmocom OsmoTRX architecture operates with one TRX per osmo-trx process.

No, that's not correct. OsmoTRX supports serving multiple TRX. We also support serving multiple TRX from 1 OsmoTRX in osmo-bts-trx. What we don't support is serving multiple TRX from multiple OsmoTRX in osmo-bts-trx. CC: @ewild@sysmocom.de @vyanitskiy@sysmocom.de File doc/manuals/chapters/trx-devices.adoc: https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/4b60f924_e4656939?usp=... : PS4, Line 127: instances. The current _osmo-trx-usdr_ backend operates in single-channel mode. missing new line at the end of 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: 4 Gerrit-Owner: Timur Davydov <dtv.comp@gmail.com> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin <pespin@sysmocom.de> Gerrit-CC: Hoernchen <ewild@sysmocom.de> Gerrit-CC: fixeria <vyanitskiy@sysmocom.de> Gerrit-CC: laforge <laforge@osmocom.org> Gerrit-CC: osmith <osmith@sysmocom.de> Gerrit-Attention: Hoernchen <ewild@sysmocom.de> Gerrit-Attention: laforge <laforge@osmocom.org> Gerrit-Attention: fixeria <vyanitskiy@sysmocom.de> Gerrit-Attention: Timur Davydov <dtv.comp@gmail.com> Gerrit-Comment-Date: Fri, 27 Feb 2026 10:41:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin <pespin@sysmocom.de> Comment-In-Reply-To: Timur Davydov <dtv.comp@gmail.com>