Change in ...osmo-bts[master]: osmo-bts-trx/trx_if.c: request the newest TRXD header version

[fixeria]

fixeria has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-bts/+/14611 )

Change subject: osmo-bts-trx/trx_if.c: request the newest TRXD header version
......................................................................


Patch Set 5:

(5 comments)

https://gerrit.osmocom.org/#/c/14611/5/src/osmo-bts-trx/l1_if.h 
File src/osmo-bts-trx/l1_if.h:

https://gerrit.osmocom.org/#/c/14611/5/src/osmo-bts-trx/l1_if.h@11 
PS5, Line 11: 	int			setformat_sent;
> bool?
Other *_sent members are using int. Feel free to fix this in a separate change, I don't think it's critical and deserves that much attention.


https://gerrit.osmocom.org/#/c/14611/5/src/osmo-bts-trx/l1_if.c 
File src/osmo-bts-trx/l1_if.c:

https://gerrit.osmocom.org/#/c/14611/5/src/osmo-bts-trx/l1_if.c@214 
PS5, Line 214: 			l1h->config.setformat_sent = 1;
> move them to be in the same place

ACK.


https://gerrit.osmocom.org/#/c/14611/5/src/osmo-bts-trx/trx_if.c 
File src/osmo-bts-trx/trx_if.c:

https://gerrit.osmocom.org/#/c/14611/5/src/osmo-bts-trx/trx_if.c@446 
PS5, Line 446: 
> Seems we want to add code here to match the version parameter in SETFORMAT.
Huh, I thought we're doing this already for all commands. And that's why we decided to keep the requested version in the response... As it turns out, we do match the command only.

For sure, I can add SETFORMAT here, but what about other commands? What if we send 'CMD TXTUNE 890000', but receive 'RSP TXTUNE 000000'? Why this matching should be implemented for every separate command individually?


https://gerrit.osmocom.org/#/c/14611/5/src/osmo-bts-trx/trx_if.c@486 
PS5, Line 486: 	if (rsp->status < 0 || rsp->status > TRX_DATA_FORMAT_VER) {
> You should check here if rsp->status > l1h->config. […]
ACK.


https://gerrit.osmocom.org/#/c/14611/5/src/osmo-bts-trx/trx_if.c@529 
PS5, Line 529: 	} else if (strcmp(tcm->cmd, "SETFORMAT") == 0) {
> Add a comment here on why tcm->cmd is used instead of rsp->cmd (due to RSP ERR 1 being answered).
ACK!



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

Gerrit-Project: osmo-bts
Gerrit-Branch: master
Gerrit-Change-Id: I8afe950bd1ec2afaf3347ff848ee46e69c4f5011
Gerrit-Change-Number: 14611
Gerrit-PatchSet: 5
Gerrit-Owner: fixeria <axilirator at gmail.com>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <axilirator at gmail.com>
Gerrit-Reviewer: laforge <laforge at gnumonks.org>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-CC: ipse <Alexander.Chemeris at gmail.com>
Gerrit-Comment-Date: Thu, 04 Jul 2019 15:41:49 +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/20190704/55aa68fe/attachment.htm>