Change in ...osmo-ttcn3-hacks[master]: BTS: add some dynamic power control tests

laforge gerrit-no-reply at lists.osmocom.org
Tue Jun 11 14:53:26 UTC 2019


laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/14417 )

Change subject: BTS: add some dynamic power control tests
......................................................................


Patch Set 2:

(6 comments)

https://gerrit.osmocom.org/#/c/14417/2/bts/BTS_Tests.ttcn 
File bts/BTS_Tests.ttcn:

https://gerrit.osmocom.org/#/c/14417/2/bts/BTS_Tests.ttcn@1974 
PS2, Line 1974: 	pp := valueof(ts_RSL_IE_MS_Power_Parameters('aabbcc'O));
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.


https://gerrit.osmocom.org/#/c/14417/2/bts/BTS_Tests.ttcn@2028 
PS2, Line 2028: 
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...


https://gerrit.osmocom.org/#/c/14417/2/bts/BTS_Tests.ttcn@2062 
PS2, Line 2062: 
              : 	var RSL_IE_MS_Power ms_power;
              : 	ms_power := valueof(ts_RSL_IE_MS_Power(pwr_var));
              : 	var RSL_IE pwr;
              : 	pwr  := valueof(t_RSL_IE(RSL_IE_MS_POWER, RSL_IE_Body:{ms_power := ms_power}));
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) ...


https://gerrit.osmocom.org/#/c/14417/2/bts/BTS_Tests.ttcn@2165 
PS2, Line 2165: 	if((band == "GSM450")
if if were a function, one wouldn't have spaces before the parenthesis :P


https://gerrit.osmocom.org/#/c/14417/2/bts/BTS_Tests.ttcn@2166 
PS2, Line 2166: 	or (band == "GSM480")
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) { ... }

But once again, just a stylistic comment, no absolute need to change the code.


https://gerrit.osmocom.org/#/c/14417/2/bts/BTS_Tests.ttcn@2264 
PS2, Line 2264: 
unrelated whitespace



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/14417
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-ttcn3-hacks
Gerrit-Branch: master
Gerrit-Change-Id: I57489ba22542d859ced767e856634f9060c060f0
Gerrit-Change-Number: 14417
Gerrit-PatchSet: 2
Gerrit-Owner: Hoernchen <ewild at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: laforge <laforge at gnumonks.org>
Gerrit-Comment-Date: Tue, 11 Jun 2019 14:53:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190611/6a1c7daa/attachment.html>


More information about the gerrit-log mailing list