Change in ...osmo-trx[master]: add XTRX support

[pespin]

pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-trx/+/15685 )

Change subject: add XTRX support
......................................................................


Patch Set 1: Code-Review-1

(19 comments)

https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp 
File Transceiver52M/device/xtrx/XTRXDevice.cpp:

https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@37 
PS1, Line 37: static int time_tx_corr = 60; //20+20+20+20+20;
20+20+20+20+20 is not 60 lol


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@49 
PS1, Line 49: 			  << " rx_path(0): " << (rx_paths.size() ? rx_paths[0] : "<>")
I don't remember now how tx_paths/rx_pathsa re generated, but I bet it's always populated to be size() == chans, and I doubt chans can be zero (int that case we should exit way before).


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@52 
PS1, Line 52: 	txsps = tx_sps;
You can probably set those around line 42 out of the body of the function (constructor initializers).


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@173 
PS1, Line 173: bool XTRXDevice::start() 
trailing whitespace


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@177 
PS1, Line 177: 		return false;
would be great checking other devices to see if we actually return true here.


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@194 
PS1, Line 194: 	xtrx_set_antenna(device, XTRX_TX_AUTO);
so rx_path/tx_path VTY params are not really used yet?


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@230 
PS1, Line 230: bool XTRXDevice::stop() 
trailing whitespace


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@265 
PS1, Line 265: } 
trailing whitespace


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@280 
PS1, Line 280: 	int res = xtrx_set_gain(device, XTRX_CH_AB, XTRX_TX_PAD_GAIN, dB - 30, &txGain);
db - 30 ?


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@311 
PS1, Line 311: 							TIMESTAMP timestamp, bool *underrun, unsigned *RSSI)
Please fix indenting for params, they should be under the "(" character. ( in all functions!).


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@322 
PS1, Line 322: 	int res = xtrx_recv_sync_ex(device, &ri);
You probably need to add code to use smpl_buf in here later on, not needed for now though.


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@345 
PS1, Line 345: 	if (!started)
I think this should never happen, because Transceiver stops threads calling this function on those conditions.


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@361 
PS1, Line 361: 	if (*underrun) {
No need for brackets here.


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@370 
PS1, Line 370: 	LOG(ALERT) << "CH" << chan << ": RX ANTENNA: " << ant.c_str();
probably want to add "(Not implemented)" here.


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@401 
PS1, Line 401: bool XTRXDevice::updateAlignment(TIMESTAMP timestamp) 
trailing whitespace


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@403 
PS1, Line 403: 	LOG(ALERT) << "Update Aligment " << timestamp;
Since requiresRadioAlign() returns false, this should never be called, right?


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@419 
PS1, Line 419: 				  << "    actual freq: " << actual << std::endl;
Fix indent.


https://gerrit.osmocom.org/#/c/15685/1/contrib/systemd/Makefile.am 
File contrib/systemd/Makefile.am:

https://gerrit.osmocom.org/#/c/15685/1/contrib/systemd/Makefile.am@25 
PS1, Line 25: EXTRA_DIST = $(SYSTEMD_SERVICES)
That's wrong I'd say. You want to keep all files in EXTRA_DIST, because someone else may decide to compile with different TRX device support from the archive later on, so all of themm need to be available.


https://gerrit.osmocom.org/#/c/15685/1/doc/examples/osmo-trx-xtrx/osmo-trx-xtrx.cfg 
File doc/examples/osmo-trx-xtrx/osmo-trx-xtrx.cfg:

https://gerrit.osmocom.org/#/c/15685/1/doc/examples/osmo-trx-xtrx/osmo-trx-xtrx.cfg@21 
PS1, Line 21:   tx-path BAND1
Do you use same names for tx/rx paths as LimeSUite? Otherwise this is wrong.



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-trx/+/15685
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-trx
Gerrit-Branch: master
Gerrit-Change-Id: I1067dfef53aa2669cc7c189cccae10074c674390
Gerrit-Change-Number: 15685
Gerrit-PatchSet: 1
Gerrit-Owner: rauf.gyulaliev at fairwaves.co
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Mon, 07 Oct 2019 18:29:15 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20191007/f1df6743/attachment.htm>