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

This is merely a historical archive of years 2008-2021, before the migration to mailman3.

A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.

ipse gerrit-no-reply at lists.osmocom.org
Tue Oct 8 22:52:18 UTC 2019


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

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


Patch Set 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
Good point.


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 […]
Looks like it can be 0 if we don't specify rx/tx-path in the config file.

Plus, does it really hurt to have here? Esp if no one remembers is it can be zero or not?


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) […]
Yes, but it looks cleaner here and doesn't affect performance.


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


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.
What do you mean by "other devices" 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?
Ack


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


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


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 ?
I'll ask Sergey why is it done this way but I'm sure there is a reason for that.


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!).
Ack


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.
Ack


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 c […]
Does it hurt to check, just to be sure?


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


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.
Ack


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


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?
Correct. But it's an abstract virtual function and thus must be implemented.


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


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. […]
Ack


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.
Ok, I think we should remove these since we're ignoring them anyway. We're not specifying them for UmTRX as well since the driver knows better which path to use, based on frequency.



-- 
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-CC: ipse <Alexander.Chemeris at gmail.com>
Gerrit-Comment-Date: Tue, 08 Oct 2019 22:52:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20191008/84786ad0/attachment.htm>


More information about the gerrit-log mailing list