Attention is currently required from: Timur Davydov.
Patch set 3:Code-Review -1
16 comments:
Patchset:
This was not a full review, I will review USDRDevice.cpp/.h again once you fix all the formatting there.
File Transceiver52M/device/usdr/USDRDevice.h:
Patch Set #3, Line 54: int txsps; ///< samples count per GSM symbol for Tx
unsigned?
We actually have "size_t tx_sps, rx_sps;" in RadioDevice, why not reusing them?
Patch Set #3, Line 60: int timeTxCorr = 0; ///< time correction for TX timestamp, in samples, to align it with RX timestamp, which is needed to achieve correct timing of the transmitted signal. This is a device-specific parameter that can be configured via dev_args and defaults to 0 if not specified.
Change all these comments using /* */ and properly split into acceptable width (<=120 chars).
Patch Set #3, Line 63: TIMESTAMP ts_last_recv = 0;
wrong indentation
Patch Set #3, Line 90: Read samples from the USDR.
syntax for comment (at least indentation) is different here and in next block.
We usually tend to have "* yourtexthere" at the start of each comment line.
Patch Set #3, Line 100: TIMESTAMP timestamp = 0xffffffff, bool *underrun = NULL);
wrong indentation here and below.
Pleasr get rid of trailing whitespace.
Patch Set #3, Line 132: TIMESTAMP initialReadTimestamp(void) { return ts_initial;}
Please, fix indentation in the whole file.
File Transceiver52M/device/usdr/USDRDevice.cpp:
Patch Set #3, Line 49: {"limemini", {tx_offset: 86.769us}}, /* 94 samples at 1083333.33333 sps corresponds to 86.769 us */
are you sure this is correct? isn't it xsdr? maybe add an extra commit explaining?
Patch Set #3, Line 52: enum {
I don't see why this should be an enum.
Looks more like 2 defines or 2 const unsigned int.
Patch Set #3, Line 85: std::string msg = std::string("Error creating USDR device with connection string '") + connection_string + "' res: " + std::to_string(res) + "(" + strerror(res) + ")";
Fix all lines too long like this. Maximum is 120 chars.
Patch Set #3, Line 180: dev = NULL;
Please fix indentation in the whole file. Better use tabs everywhere.
File debian/osmo-trx-usdr.postinst:
Patch Set #3, Line 18: # Fix permissions of previous (root-owned) install (OS#4107)
This is AFAICT not needed here, because there's no previous root-owned install ;)
File doc/manuals/chapters/trx-backends.adoc:
Patch Set #3, Line 85: The backend supports single-channel SDR hardware and is designed for setups
But in the other .adoc (devices) you say xSDR supports multiple channels? Is this simply not implemented yet in osmo-trx?
Patch Set #3, Line 86: where multiple small SDRs are combined to present one or more TRX instances
You mean using multiple SDR devices controlled by one osmo-trx-usdr process?
Or multiple SDR devices with multiple osmo-trx-usdr processes each controlling 1 device?
The later is not supported and I can foresee clock sync issues. The former is not supported at least for other backends, dunno for USDR yet (still reviewing).
File doc/manuals/chapters/trx-devices.adoc:
Patch Set #3, Line 102: scale the number of TRX instances.
I'd welcome some extra documentation here regarding how "multi-unit setups to
scale the number of TRX instances." is accomplished, or at least some link to somewhere explaining it.
To view, visit change 42198. To unsubscribe, or for help writing mail filters, visit settings.