<p style="white-space: pre-wrap; word-wrap: break-word;">Thanks for the review, updated the patch.</p><p style="white-space: pre-wrap; word-wrap: break-word;">One thing I am not sure about is this line from 3GPP TS 44.060 11.2.47:</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">This message may be segmented across more than two RLC/MAC control blocks by using extended RLC/MAC control message segmentation.</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Does this mean, we need to support splitting long messages into multiple msgb buffers (and sending multiple buffers to each UE then)?</p><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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">but nobody ever reads/checks it</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">It is used in gprs_rlcmac_sched.cpp, function sched_app_info(): after decrementing, if it is zero, free bts_data->app_info and log that the message was sent to all MS with active TBF.</p><p style="white-space: pre-wrap; word-wrap: break-word;">If you like, I can describe this in the surrounding comments. (I've left it out, because the commenting seems to be quite verbose already for the two variables.)</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">why an extra struct around a single member?</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Otherwise I would have needed to add a setter and getter. As I understood from the IRC discussion, it is preferred to not introduce more C++ style code in osmo-pcu where possible. This ms_data is now similar to bts_data in bts.h.</p><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><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></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Yes, that is a better name! Renamed it. Also I've renamed bts_data->app_info_todo to bts_data->app_info_pending, as it is the count of pending messages.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">I would have suggested to use the osmocom/core/bitvec.h infrastructure for constructing the payload. […]</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done.</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><blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;">style</blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">Done</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: Wed, 11 Sep 2019 07:59:03 +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: laforge <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-MessageType: comment </div>