<p><a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/14417">View Change</a></p><p>6 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/14417/2/bts/BTS_Tests.ttcn">File bts/BTS_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/14417/2/bts/BTS_Tests.ttcn@1974">Patch Set #2, Line 1974:</a> <code style="font-family:monospace,monospace">     pp := valueof(ts_RSL_IE_MS_Power_Parameters('aabbcc'O));</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I would consider sending an empty octet string ''O to ensure we're not confusing the receiver.  Sure, at them moment OsmoBTs doesn't implement any parameters, but in the future we might.  Using an empty IE fulfills the requirement of having that element present (and hence enabling BTS-side power control), but at the same time has low risk of breaking something that actually might interpret aabbcc as actual parameters.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14417/2/bts/BTS_Tests.ttcn@2028">Patch Set #2, Line 2028:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I would avoid the 'rsl' variable here. As RSL.send() can take the send template directly, it also means you save the valueof().  Sure, it's just stylistic, not important...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14417/2/bts/BTS_Tests.ttcn@2062">Patch Set #2, Line 2062:</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>        var RSL_IE_MS_Power ms_power;<br> ms_power := valueof(ts_RSL_IE_MS_Power(pwr_var));<br>     var RSL_IE pwr;<br>       pwr  := valueof(t_RSL_IE(RSL_IE_MS_POWER, RSL_IE_Body:{ms_power := ms_power}));<br></pre></blockquote></p><p style="white-space: pre-wrap; word-wrap: break-word;">again here I'd typically try to avoid the local variables, as they force you to do valueof() all the time, which sort of makes templates less nice to use.  The alternative if you want the local variables would be to define them as template variables, so 'var template RSL_IE_MS_Power ms_power := ts_RSL_IE_MS_Power(pwr_var);' would do the trick, maybe even var template (value) ...</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14417/2/bts/BTS_Tests.ttcn@2165">Patch Set #2, Line 2165:</a> <code style="font-family:monospace,monospace">  if((band == "GSM450")</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">if if were a function, one wouldn't have spaces before the parenthesis :P</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14417/2/bts/BTS_Tests.ttcn@2166">Patch Set #2, Line 2166:</a> <code style="font-family:monospace,monospace">  or (band == "GSM480")</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I would typically have used a select/case construct here, not having to repeat "band ==" in every line.  The nice part about the select/case in TTCN3 is that every "case" is a template match.  So you can define a charstring template matching all those bands that equal in their treatment and then have a single line. case (t_my_bands) { ... }</p><p style="white-space: pre-wrap; word-wrap: break-word;">But once again, just a stylistic comment, no absolute need to change the code.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/14417/2/bts/BTS_Tests.ttcn@2264">Patch Set #2, Line 2264:</a> <code style="font-family:monospace,monospace"></code></p><p style="white-space: pre-wrap; word-wrap: break-word;">unrelated whitespace</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/14417">change 14417</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/+/14417"/><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: I57489ba22542d859ced767e856634f9060c060f0 </div>
<div style="display:none"> Gerrit-Change-Number: 14417 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Hoernchen <ewild@sysmocom.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-CC: laforge <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Comment-Date: Tue, 11 Jun 2019 14:53:26 +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>