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=... : 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=... : 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=... : 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=... : 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=... : 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=... : PS4, Line 115: /**
trailing whitespace
Removed trailing whitespace.
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/3176a15a_9aec5aa0?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. […]
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=... : 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=... : 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=... : 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=... : 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=... : 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=... : 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=... : 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=... : 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=... : 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=... : PS4, Line 260: if (dev) {
early return: […]
Implemented as suggested.
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/920a6e31_3e2cd58a?usp=... : 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=... : PS4, Line 296: return !started;
return true;
Implemented as suggested.
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/43edea18_c727c50a?usp=... : 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=... : 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=... : PS4, Line 392: TIMESTAMP timestamp, bool *underrun)
wrong indentation
Fixed the indentation.
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/ae31405e_b30f2fab?usp=... : 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=... : PS4, Line 421: bool *underrun, TIMESTAMP timestamp)
wrong indentation
Fixed
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/374673e2_814771bb?usp=... : 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=... : 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=... : 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=... : 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=... : 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=... : 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.