Attention is currently required from: Hoernchen, fixeria, laforge, pespin.
30 comments:
File Transceiver52M/device/usdr/USDRDevice.h:
Patch Set #4, 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.
Patch Set #4, 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.
Patch Set #4, 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.
Patch Set #4, 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.
Patch Set #4, 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).
trailing whitespace
Removed trailing whitespace.
Patch Set #4, 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`
Patch Set #4, Line 126: bool *underrun = nullptr);
align to firs tchar in "("
Aligned as suggested.
File Transceiver52M/device/usdr/USDRDevice.cpp:
Patch Set #4, 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.
Patch Set #4, 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.
Patch Set #4, 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).
Patch Set #4, 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`
Patch Set #4, 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.
Patch Set #4, 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.
Patch Set #4, 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.
Patch Set #4, 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.
Patch Set #4, Line 260: if (dev) {
early return: […]
Implemented as suggested.
Patch Set #4, Line 269: return false;
LOGC(DDEV, ERR) << "Device already started";
Added an error message.
Patch Set #4, Line 296: return !started;
return true;
Implemented as suggested.
Patch Set #4, 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.
Patch Set #4, 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.).
Patch Set #4, Line 392: TIMESTAMP timestamp, bool *underrun)
wrong indentation
Fixed the indentation.
Patch Set #4, 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.
Patch Set #4, Line 421: bool *underrun, TIMESTAMP timestamp)
wrong indentation
Fixed
Patch Set #4, Line 475: std::string USDRDevice::getTxAntenna(size_t chan )
extra space at the end.
Removed
Patch Set #4, Line 492: return GSM::Time(6,7);
missing space between "," and "7".
Added space
Patch Set #4, 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:
Patch Set #4, 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:
File doc/manuals/chapters/trx-backends.adoc:
Patch Set #3, 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:
Patch Set #4, 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 change 42198. To unsubscribe, or for help writing mail filters, visit settings.