<p><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370">View Change</a></p><p>16 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/+/18370/5/pcu/GPRS_Components.ttcn">File pcu/GPRS_Components.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/+/18370/5/pcu/GPRS_Components.ttcn@78">Patch Set #5, Line 78:</a> <code style="font-family:monospace,monospace">uint8_t</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">boolean? Ideally, we should use something like "bitstring length (8..11)" or a union of BIT8 abd BIT11 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/+/18370/5/pcu/GPRS_Components.ttcn@79">Patch Set #5, Line 79:</a> <code style="font-family:monospace,monospace">PCUIF_BurstType</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Not sure if access burst type should be stored here, as you don't really need to match it in the assignment message.</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/+/18370/5/pcu/GPRS_Components.ttcn@160">Patch Set #5, Line 160:</a> <code style="font-family:monospace,monospace">dynamic</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It also makes sense to parse both 'usf' and 'usf_granularity' here. Can be done later though.</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/+/18370/5/pcu/GPRS_Components.ttcn@169">Patch Set #5, Line 169:</a> <code style="font-family:monospace,monospace">dynamic</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">It also makes sense to parse both 'usf' and 'usf_granularity' here. Can be done later though.</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/+/18370/5/pcu/GPRS_Components.ttcn@204">Patch Set #5, Line 204:</a> <code style="font-family:monospace,monospace">dl_ass</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This template is not used in this function...</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/+/18370/5/pcu/GPRS_Components.ttcn@249">Patch Set #5, Line 249:</a> <code style="font-family:monospace,monospace">ms.burst_type := BURST_TYPE_0;</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This should be kept as a parameter of the function that actually sends RACH.ind to the IUT.</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/+/18370/5/pcu/GPRS_Components.ttcn@294">Patch Set #5, Line 294:</a> <code style="font-family:monospace,monospace">tr_RLCMAC_DL_PACKET_ASS</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">So by default, this function would match DL Assignment, while it can also handle UL Assignment. Maybe using '?' by default would make more sense?</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/+/18370/5/pcu/GPRS_Components.ttcn@294">Patch Set #5, Line 294:</a> <code style="font-family:monospace,monospace">imm_ass_rlcmac</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This name is confusing as there is no RLC/MAC version of the RR Immediate Assignment.</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/+/18370/5/pcu/GPRS_Components.ttcn@299">Patch Set #5, Line 299:</a> <code style="font-family:monospace,monospace">f_rx_rlcmac_dl_block</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please add a TODO/FIXME here stating that we need an alt statement 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/+/18370/5/pcu/GPRS_Components.ttcn@310">Patch Set #5, Line 310:</a> <code style="font-family:monospace,monospace">Wrong TLLI</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This probably means that received assignment is not for us, and we should wait for another one? At least a comment is needed 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/+/18370/5/pcu/GPRS_Components.ttcn@327">Patch Set #5, Line 327:</a> <code style="font-family:monospace,monospace">uint16_t</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Why 16 bits? Should be 8 I believe.</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/+/18370/5/pcu/GPRS_Components.ttcn@334">Patch Set #5, Line 334:</a> <code style="font-family:monospace,monospace">871</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">TODO (separate change idea): use mp_trx0_arfcn 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/+/18370/5/pcu/GPRS_Components.ttcn@353">Patch Set #5, Line 353:</a> <code style="font-family:monospace,monospace">establish</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">This name is confusing. Without looking at the code, I would think that it somehow establishes a Downlink TBF, but actually we *expect* DL TBF assignment 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/+/18370/5/pcu/GPRS_Components.ttcn@362">Patch Set #5, Line 362:</a> <code style="font-family:monospace,monospace">uint32_t fn := 0</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">template uint32_t fn := ?</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/+/18370/5/pcu/GPRS_Components.ttcn@381">Patch Set #5, Line 381:</a> <code style="font-family:monospace,monospace">data := f_pad_oct(data, 23, '00'O); /* CS-1 */</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">There must be a comment that padding only works for CS-1. A payload of more than 23 octets would be sent unaligned.</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/+/18370/5/pcu/GPRS_Components.ttcn@405">Patch Set #5, Line 405:</a> <code style="font-family:monospace,monospace">f_ms_tx_ul_data_block_multi</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Maybe call it f_ms_tx_rand_ul_data_blocks()? The point is that the function generates payload itself, so it should be reflected in its name.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370">change 18370</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/+/18370"/><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: Ib3fee37580f0ea0530a659dec83656799bf57288 </div>
<div style="display:none"> Gerrit-Change-Number: 18370 </div>
<div style="display:none"> Gerrit-PatchSet: 5 </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 <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: laforge <laforge@osmocom.org> </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 21 May 2020 18:21:40 +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>