<p><a href="https://gerrit.osmocom.org/c/osmo-pcu/+/15459">View Change</a></p><p>4 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15459/3/src/bts.h">File src/bts.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/15459/3/src/bts.h@171">Patch Set #3, Line 171:</a> <code style="font-family:monospace,monospace">uint32_t app_info_todo;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm not quite sure I understand the purpose of this. It seems we firt count the number of UEs with active TBF, then print that number, then decrement it but nobody ever reads/checks it. So why not simply iterate only once over all UEs and have a local variable that counts how many we sent (and then print that)?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15459/3/src/gprs_ms.h">File src/gprs_ms.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/15459/3/src/gprs_ms.h@43">Patch Set #3, Line 43:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">struct gprs_rlcmac_ms {<br> bool app_info_send;<br>};<br></pre></blockquote></p><ul><li>why an extra struct around a single member?</li><li>i guess it should be called 'sent' if it wants to indicate whether transmission already happened or not.</li></ul><p style="white-space: pre-wrap; word-wrap: break-word;">Update: I now understand it actually determines if the app_info still has to be sent. Let's call it app_info_pending?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15459/3/src/gprs_rlcmac.cpp">File src/gprs_rlcmac.cpp:</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/15459/3/src/gprs_rlcmac.cpp@59">Patch Set #3, Line 59:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">word = 0x00; /* 0-1: page mode: normal */<br> word |= (0x0F & req->application_type) << 6; /* 2-5: application type */<br> word |= (0xC0 & req->data[0]) >> 6; /* 6-7: first two data bits */<br> msgb_put_u8(msg, word);<br><br> for (i=0; i < req->len - 1; i++) {<br> word = req->data[i] << 2; /* 0-6: last six data bits from current byte */<br> word |= (0xC0 & req->data[i + 1]) >> 6; /* 7-8: first two data bits from next byte */<br> msgb_put_u8(msg, word);<br> }<br><br> word = (0xC0 & req->data[req->len -1]) << 2; /* 0-6: last six data bits from last byte (rest is padding) */<br> msgb_put_u8(msg, word);<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">I would have suggested to use the osmocom/core/bitvec.h infrastructure for constructing the payload. See e.g. http://git.osmocom.org/osmo-bts/commit/?id=f53fde91a36eff2601df9811fddee97b8f89d6ee how it's done in the BTS.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/15459/3/src/gprs_rlcmac_sched.cpp">File src/gprs_rlcmac_sched.cpp:</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/15459/3/src/gprs_rlcmac_sched.cpp@140">Patch Set #3, Line 140:</a> </p><p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><pre style="font-family: monospace,monospace; white-space: pre-wrap;">}<br> else<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">style</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-pcu/+/15459">change 15459</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-pcu/+/15459"/><meta itemprop="name" content="View Change"/></div></div>
<div style="display:none"> Gerrit-Project: osmo-pcu </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: Ie35959f833f46bde5f2126314b6f96763f863b36 </div>
<div style="display:none"> Gerrit-Change-Number: 15459 </div>
<div style="display:none"> Gerrit-PatchSet: 3 </div>
<div style="display:none"> Gerrit-Owner: osmith <osmith@sysmocom.de> </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: osmith <osmith@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 10 Sep 2019 12:39:34 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>