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 <ew...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-CC: laforge <lafo...@gnumonks.org> Gerrit-Comment-Date: Tue, 11 Jun 2019 14:53:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
