Attention is currently required from: Hoernchen, Timur Davydov, fixeria, laforge.
31 comments:
Patchset:
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:
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.
Patch Set #4, Line 46: struct DeviceParameters {
Can we use "struct dev_desc" like in all other devices I'm seeing?
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?
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?
Patch Set #4, Line 86: public:
Keep order in all other device classes, where public goes first.
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. I'd say remove them here and submit a separate patch improving documentation in radioDevice.h for those virtual functions.
Patch Set #4, Line 126: bool *underrun = nullptr);
align to firs tchar in "("
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.
Patch Set #4, 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.
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
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().
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?
Patch Set #4, Line 203: uint64_t val;
Try putting all variable declarations at the top of the function.
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 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)
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.
Patch Set #4, Line 246: usdr_dms_sync_exception(dev, "rx", ped.size(), ped.data());
Same, we should imho avoid all these hardcoded strings.
Patch Set #4, Line 260: if (dev) {
early return:
if (!dev)
return;
Patch Set #4, Line 269: return false;
LOGC(DDEV, ERR) << "Device already started";
Patch Set #4, Line 296: return !started;
return true;
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?
Leave it under "#if 0" if that's the case.
Patch Set #4, 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.
Patch Set #4, Line 392: TIMESTAMP timestamp, bool *underrun)
wrong indentation
Patch Set #4, 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.Patch Set #4, Line 421: bool *underrun, TIMESTAMP timestamp)
wrong indentation
Patch Set #4, Line 475: std::string USDRDevice::getTxAntenna(size_t chan )
extra space at the end.
Patch Set #4, Line 492: return GSM::Time(6,7);
missing space between "," and "7".
Patch Set #4, 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:
Patch Set #4, Line 3: /usr/bin/osmo-trx-usdr
why do some start with / and some not?
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 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:
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.
To view, visit change 42198. To unsubscribe, or for help writing mail filters, visit settings.