<p><a href="https://gerrit.osmocom.org/c/osmo-bts/+/14611">View Change</a></p><p>5 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/14611/5/src/osmo-bts-trx/l1_if.h">File src/osmo-bts-trx/l1_if.h:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14611/5/src/osmo-bts-trx/l1_if.h@11">Patch Set #5, Line 11:</a> <code style="font-family:monospace,monospace">       int                     setformat_sent;</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">bool?</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/14611/5/src/osmo-bts-trx/l1_if.c">File src/osmo-bts-trx/l1_if.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14611/5/src/osmo-bts-trx/l1_if.c@214">Patch Set #5, Line 214:</a> <code style="font-family:monospace,monospace">                 l1h->config.setformat_sent = 1;</code></p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">move them to be in the same place</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">ACK.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/14611/5/src/osmo-bts-trx/trx_if.c">File src/osmo-bts-trx/trx_if.c:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14611/5/src/osmo-bts-trx/trx_if.c@446">Patch Set #5, Line 446:</a> <code style="font-family:monospace,monospace"></code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Seems we want to add code here to match the version parameter in SETFORMAT.</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">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.</p><p style="white-space: pre-wrap; word-wrap: break-word;">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?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14611/5/src/osmo-bts-trx/trx_if.c@486">Patch Set #5, Line 486:</a> <code style="font-family:monospace,monospace">       if (rsp->status < 0 || rsp->status > TRX_DATA_FORMAT_VER) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">You should check here if rsp->status > l1h->config. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">ACK.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14611/5/src/osmo-bts-trx/trx_if.c@529">Patch Set #5, Line 529:</a> <code style="font-family:monospace,monospace">    } else if (strcmp(tcm->cmd, "SETFORMAT") == 0) {</code></p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">Add a comment here on why tcm->cmd is used instead of rsp->cmd (due to RSP ERR 1 being answered).</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">ACK!</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-bts/+/14611">change 14611</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-bts/+/14611"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-bts </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I8afe950bd1ec2afaf3347ff848ee46e69c4f5011 </div>
<div style="display:none"> Gerrit-Change-Number: 14611 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </div>
<div style="display:none"> Gerrit-Owner: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-CC: ipse <Alexander.Chemeris@gmail.com> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 04 Jul 2019 15:41:49 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Comment-In-Reply-To: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>