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

[pespin]

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

Change subject: Initial XTRX support
......................................................................


Patch Set 3:

(5 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@49 
PS1, Line 49: 			  << " rx_path(0): " << (rx_paths.size() ? rx_paths[0] : "<>")
> Looks like it can be 0 if we don't specify rx/tx-path in the config file. […]
in trx_start() (osmo-trx.cpp):

	/* Generate vector of rx/tx_path: */
	for (i = 0; i < trx->cfg.num_chans; i++) {
		rx_paths.push_back(charp2str(trx->cfg.chans[i].rx_path));
		tx_paths.push_back(charp2str(trx->cfg.chans[i].tx_path));
	}

So it cannot be size()==0 unless there's cnum_chans=0, which shouldn't happen.

IMHO having this check here is unnecesary and confusing for people reading the code.


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@52 
PS1, Line 52: 	txsps = tx_sps;
> Yes, but it looks cleaner here and doesn't affect performance.
I disagree, but it's bike-shed talking anyway, so keep it this way if you prefer, np.


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@177 
PS1, Line 177: 		return false;
> What do you mean by "other devices" here?
Other device implementations. For instance UHDDevice does:

	if (started) {
		LOGC(DDEV, ERROR) << "Device already started";
		return false;
	}

So fine here :) Maybe wanna add a LOGC like UHDDevice.


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


https://gerrit.osmocom.org/#/c/15685/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@345 
PS1, Line 345: 	if (!started)
> Does it hurt to check, just to be sure?
It adds confusing about how the system is expected to work, and makes it difficult to refactor later (because looks like different devices expect the generic framework to work in different ways). For instance UHDDevice doesn't do this kind of check.



-- 
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: 3
Gerrit-Owner: rauf.gyulaliev at fairwaves.co
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: ipse <Alexander.Chemeris at gmail.com>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Wed, 09 Oct 2019 11:43:41 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Comment-In-Reply-To: ipse <Alexander.Chemeris at gmail.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20191009/75c85dc1/attachment.htm>