Change in osmo-ttcn3-hacks[master]: pcu: Refactor GPRS infrastructure to keep state and simplify tests

[fixeria]

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

Change subject: pcu: Refactor GPRS infrastructure to keep state and simplify tests
......................................................................


Patch Set 5:

(8 comments)

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn 
File pcu/GPRS_Components.ttcn:

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@78 
PS5, Line 78: uint8_t
> I iitially used a boolean but then had issues passing it to some lower layer template in RAW_PCUIF o […]
Ah, yeah, TITAN does not support implicit boolean-integer conversion. Neither there are functions for that like int2bool() and bool2int(). We should just change the field type in PCUFI_Types to boolean with FIELDLENGTH(8). I am fine with doing it later, in a separate change.

On the other hand, using bitstring would simplify the things - it's easy to convert it to what PCUIF needs:

  type bitstring GsmRa length(8..11) with { variant "" };

  const GsmRa chan_req_def := '01111000'B;

  // Just before sending RACH.ind
  var uint16_t ra := bit2int(chan_req_def);
  var uint8_t is_11bit;

  if (lengthof(chan_req_def) == 11) { is_11bit := 1 } else { is_11bit := 0 }


https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@79 
PS5, Line 79: PCUIF_BurstType
> Ack, will simply apply it when needed.
Thanks!


https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@249 
PS5, Line 249: ms.burst_type := BURST_TYPE_0;
> I will simply do this check before sending RACH.ind to send the correct BURST_TYPE.
Just let the caller some freedom, there is also BURST_TYPE_2 (and even more in the specs).


https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@327 
PS5, Line 327: uint16_t
> will check the template.
Actually, template wants type integer TimingAdvance (0..219).


https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@362 
PS5, Line 362: uint32_t fn := 0
> no, fn=0 is fine here, iirc it's a special value meaning "next one".
Ah, right. Nevermind.


https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn 
File pcu/PCU_Tests.ttcn:

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@269 
PS5, Line 269: f_ms_establish_ul_tbf
> it was already calling f_establish_tbf, there's no much new overhead.

Well, f_establish_tbf() has a bit inaccurate name. Under the hood there is no TBF establishment, it simply sends RACH.ind, receives DATA.req on AGCH, matches it against tr_IMM_TBF_ASS and returns to the caller. This is exactly what's needed here. So there is overhead. Please keep low level API for small and simple test cases.


https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@584 
PS5, Line 584: if (match(dl_block, tr_RLCMAC_DUMMY_CTRL())) {
             : 			continue;
             : 		}
             : 		if (not match(dl_block, tr_RLCMAC_UL_ACK_NACK_GPRS(ul_tfi := ?)) and
             : 		    not match(dl_block, tr_RLCMAC_UL_ACK_NACK_EGPRS(ul_tfi := ?))) {
             : 			setverdict(fail, "Failed to match Packet Uplink ACK / NACK:", dl_block);
             : 			f_shutdown(__BFILE__, __LINE__);
             : 		}
> Not really, I'm fixing the test since it was not behaving correctly as an MS would. […]
I would prefer this to be done in a separate change, but if it's really needed here - let's keep it.


https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@609 
PS5, Line 609: f_shutdown
> I think this is fine. You want me to remove it? ok, I will remove it.
The proble is that I don't see how it's related to this patch. Feel free to do in a separate change with some description of its purpose.



-- 
To view, visit https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370
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: Ib3fee37580f0ea0530a659dec83656799bf57288
Gerrit-Change-Number: 18370
Gerrit-PatchSet: 5
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: fixeria <axilirator at gmail.com>
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Fri, 22 May 2020 05:18:12 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Comment-In-Reply-To: fixeria <axilirator at gmail.com>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20200522/a73788f1/attachment.htm>