Pau Espin Pedrol has posted comments on this change. ( https://gerrit.osmocom.org/14018 )
Change subject: add support for xtrx ...................................................................... Patch Set 1: (11 comments) Some comments after quick review. https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp File Transceiver52M/device/xtrx/XTRXDevice.cpp: https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@44 PS1, Line 44: LOG(INFO) << "creating XTRX device:" You should be using category DEV here, not MAIN (see other devices). https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@162 PS1, Line 162: return NORMAL; This NORMAL thing here makes no sense in the context of this function afaik. https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@167 PS1, Line 167: if (device) { Drop {} https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@175 PS1, Line 175: if (started) { Drop {} https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@212 PS1, Line 212: if (loopback) { Drop {} https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@231 PS1, Line 231: if (started) { early return: if (!started) reutrn false; https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@278 PS1, Line 278: LOG(NOTICE) << "Setting TX gain to " << dB << " dB."; LOGCHAN https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@282 PS1, Line 282: LOG(ERR) << "Error setting TX gain res: " << res; LOGCHAN. LOGCHAN everywhere where "chan" is passed as parameter in this file. https://gerrit.osmocom.org/#/c/14018/1/Transceiver52M/device/xtrx/XTRXDevice.cpp@319 PS1, Line 319: int res = xtrx_recv_sync_ex(device, &ri); Looks like we need to add smpl_buf usage in here (see my latest commits merged). https://gerrit.osmocom.org/#/c/14018/1/debian/osmo-trx-xtrx.install File debian/osmo-trx-xtrx.install: https://gerrit.osmocom.org/#/c/14018/1/debian/osmo-trx-xtrx.install@3 PS1, Line 3: /usr/bin/osmo-trx-xtrx why some strt with / and some doesn't? Unify style if possible. https://gerrit.osmocom.org/#/c/14018/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/14018/1/doc/examples/osmo-trx-xtrx/osmo-trx-xtrx.cfg@21 PS1, Line 21: tx-path BAND1 Are you sure this belongs here? looks copied from LMS file. -- To view, visit https://gerrit.osmocom.org/14018 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-trx Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Iad73e0611e7951d5bcfcc918063cc3778cb1dd8f Gerrit-Change-Number: 14018 Gerrit-PatchSet: 1 Gerrit-Owner: lynxis lazus <[email protected]> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-CC: Pau Espin Pedrol <[email protected]> Gerrit-Comment-Date: Mon, 13 May 2019 16:27:58 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: No
