Attention is currently required from: Timur Davydov.
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 3: Code-Review-1
(16 comments)
Patchset:
PS3: 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:
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/af0bce02_b674c263?usp=... : PS3, 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?
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/8b925d17_1efc289a?usp=... : PS3, 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).
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/20940c5f_f75af47f?usp=... : PS3, Line 63: TIMESTAMP ts_last_recv = 0; wrong indentation
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/12705b92_a891e300?usp=... : PS3, 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.
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/4574676b_9f77251b?usp=... : PS3, Line 100: TIMESTAMP timestamp = 0xffffffff, bool *underrun = NULL); wrong indentation here and below.
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/abf0fde4_f08d4af3?usp=... : PS3, Line 121: Pleasr get rid of trailing whitespace.
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/460c17d8_fb1f28b7?usp=... : PS3, Line 132: TIMESTAMP initialReadTimestamp(void) { return ts_initial;} Please, fix indentation in the whole file.
File Transceiver52M/device/usdr/USDRDevice.cpp:
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/3ce342ee_589c89f5?usp=... : PS3, 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?
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/5d746f23_0af4c7af?usp=... : PS3, Line 52: enum { I don't see why this should be an enum. Looks more like 2 defines or 2 const unsigned int.
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/6f8ee7e3_0dd68be8?usp=... : PS3, 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.
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/b6291fd4_84385229?usp=... : PS3, Line 180: dev = NULL; Please fix indentation in the whole file. Better use tabs everywhere.
File debian/osmo-trx-usdr.postinst:
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/165e1499_770932f1?usp=... : PS3, 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:
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/fad6a46c_77479109?usp=... : PS3, 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?
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/6486201c_860cc643?usp=... : PS3, 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:
https://gerrit.osmocom.org/c/osmo-trx/+/42198/comment/08cb6d24_d39ad6ef?usp=... : PS3, 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.