<p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/26256">View Change</a></p><p>9 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/26256/1/library/RLCMAC_CSN1_Templates.ttcn">File library/RLCMAC_CSN1_Templates.ttcn:</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/osmo-ttcn3-hacks/+/26256/1/library/RLCMAC_CSN1_Templates.ttcn@23">Patch Set #1, Line 23:</a> <code style="font-family:monospace,monospace">tr_RlcMacDlCtrl_PKT_ACC_REJ_ID_TLLI</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not critical, but having 'RlcMacDlCtrl' in the name of this template looks a bit confusing to me, because it may look like a template of 'RlcmacDlCtrlMsg'. I would expect something like 'tr_PacketAccessRejectStruct_TLLI' or so.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/26256/1/library/RLCMAC_CSN1_Templates.ttcn@102">Patch Set #1, Line 102:</a> <code style="font-family:monospace,monospace">;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why are you adding this semicolon here (in this patch I mean)?</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/26256/1/library/RLCMAC_CSN1_Types.ttcn">File library/RLCMAC_CSN1_Types.ttcn:</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/osmo-ttcn3-hacks/+/26256/1/library/RLCMAC_CSN1_Types.ttcn@756">Patch Set #1, Line 756:</a> <code style="font-family:monospace,monospace">PacketAccessRejectStruct</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I had a quick look into the specs: the '< Reject struct >' actually contains an ID *and* the optional '< WAIT_INDICATION >', while in your definition only an ID is present. So I propose to move the '< WAIT_INDICATION >' stuff here. After that, we can easily implement the recursion '{ 1 < Reject struct > ** 0 }'.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/26256/1/library/RLCMAC_CSN1_Types.ttcn@764">Patch Set #1, Line 764:</a> <code style="font-family:monospace,monospace">               uint8_t                 wait_ind,</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Missing 'optional' keyword for both 'wait_ind' and 'wait_ind_size'.</p></li></ul></li><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/26256/1/pcu/PCU_Tests.ttcn">File pcu/PCU_Tests.ttcn:</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/osmo-ttcn3-hacks/+/26256/1/pcu/PCU_Tests.ttcn@2073">Patch Set #1, Line 2073:</a> <code style="font-family:monospace,monospace">    var EGPRSPktChRequest req := {</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Is there a reason why you're specifically using EGPRS Packet Channel Request here?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/26256/1/pcu/PCU_Tests.ttcn@2082">Patch Set #1, Line 2082:</a> <code style="font-family:monospace,monospace"> /* We send 7 requests, the IUT gives us all available USFs (0..6) */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">You could simply tell f_init_gprs_ms() to allocate 7 GprsMS instances, and then just call f_multi_ms_establish_tbf(). This would make the test case shorter and easier to follow/read. I mean, you were the one introducing multi-GprsMS capable API, but somehow you're not using it :P</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/26256/1/pcu/PCU_Tests.ttcn@2086">Patch Set #1, Line 2086:</a> <code style="font-family:monospace,monospace">     }</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">So if we send another RACH.ind here, the IUT would send the reject over the BCCH/AGCH in form of Immediate Assignment Reject. This message does contain the Wait Indication too, however as per 3GPP TS 44.018 section 9.1.20.3 it reflects the value of T3142, not T3172. And while writing this comment, I found out that we already test it implicitly in TC_egprs_pkt_chan_req_reject_exhaustion. Good.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/26256/1/pcu/PCU_Tests.ttcn@2101">Patch Set #1, Line 2101:</a> <code style="font-family:monospace,monospace"> /* Since all USF are taken, we should receive a Reject: */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Took me a while to understand what exactly causes the Reject. I looked above, read the comment 'ACK the DL block', and was like: what? How could an ACK/NACK message trigger the resource allocation? Then I noticed that it actually contains an optional ChannelReqDescription! I didn't know it was possible to request an Uplink resource allocation using the ACK/NACK messages. Thanks for giving me a chance to learn something new, really.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Now I am wondering if it would have been simpler to use one of the allocated Uplink TBFs to request additional resources instead of using the ACK/NACK message? Just wondering, not saying that it must be done this way.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/26256/1/pcu/PCU_Tests.ttcn@2127">Patch Set #1, Line 2127:</a> <code style="font-family:monospace,monospace">De</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Due</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/26256">change 26256</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-ttcn3-hacks/+/26256"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-ttcn3-hacks </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I3f4368c99b00453b471c3d741fecb8864ecdc628 </div>
<div style="display:none"> Gerrit-Change-Number: 26256 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: fixeria <vyanitskiy@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 16 Nov 2021 00:32:33 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>