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

historical

pespin 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:

(24 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
> boolean? Ideally, we should use something like "bitstring length (8.. […]
I iitially used a boolean but then had issues passing it to some lower layer template in RAW_PCUIF or alike because it also expected an uint8_t, so I left is as uint8_t. It can be moved to boolean together with lower layers later if we want.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@79 
PS5, Line 79: PCUIF_BurstType
> Not sure if access burst type should be stored here, as you don't really need to match it in the ass […]
Ack, will simply apply it when needed.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@160 
PS5, Line 160: dynamic
> It also makes sense to parse both 'usf' and 'usf_granularity' here. Can be done later though.
it's done for usf in later commit in the patchset. usf_granularity is TBH for later.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@169 
PS5, Line 169: dynamic
> It also makes sense to parse both 'usf' and 'usf_granularity' here. Can be done later though.
Same as above.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@204 
PS5, Line 204: dl_ass
> This template is not used in this function...
ack, will check it.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@249 
PS5, Line 249: ms.burst_type := BURST_TYPE_0;
> This should be kept as a parameter of the function that actually sends RACH.ind to the IUT.
I will simply do this check before sending RACH.ind to send the correct BURST_TYPE.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@294 
PS5, Line 294: tr_RLCMAC_DL_PACKET_ASS
> So by default, this function would match DL Assignment, while it can also handle UL Assignment. […]
ack

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@294 
PS5, Line 294: imm_ass_rlcmac
> This name is confusing as there is no RLC/MAC version of the RR Immediate Assignment.
ack, should be named f_ms_rc_ass_pacch

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@299 
PS5, Line 299: f_rx_rlcmac_dl_block
> Please add a TODO/FIXME here stating that we need an alt statement here.
for what exactly? I could start adding TODOs everywhere, but I won't add it unless there's something specific which makes sense here.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@310 
PS5, Line 310: Wrong TLLI
> This probably means that received assignment is not for us, and we should wait for another one? At l […]
Right now we are only supporting 1 MS and checking for 1 TLLI, so the message is fine.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@327 
PS5, Line 327: uint16_t
> Why 16 bits? Should be 8 I believe.
will check the template.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@353 
PS5, Line 353: establish
> This name is confusing. […]
ok will change the name,

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

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@381 
PS5, Line 381: data := f_pad_oct(data, 23, '00'O); /* CS-1 */
> There must be a comment that padding only works for CS-1. […]
Indeed I know it only works for CS1, but it was always the case. I will add a TODO.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/GPRS_Components.ttcn@405 
PS5, Line 405: f_ms_tx_ul_data_block_multi
> Maybe call it f_ms_tx_rand_ul_data_blocks()? The point is that the function generates payload itself […]
ACK

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@a811 
PS5, Line 811: bsn := 0;
> Are you sure?
Yes it's fine, I changed the test a bit so it makes more sense from a real MS point of view.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@269 
PS5, Line 269: f_ms_establish_ul_tbf
> Uhhhh, such an overhead... […]
Come on, it was already calling f_establish_tbf, there's no much new overhead.
Let's use same infra so people can easily understand other tests once you have seen one.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@310 
PS5, Line 310: f_ms_rx_imm_ass_ccch
> Same problem here, what would be the failure reason? Right, "failed to match Immediate Assignment". […]
So what? the test is more complete, more stuff is checked for free in the same scenario. Really, let's push forward.

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__);
             : 		}
> This goes behind the scope of the test case.
Not really, I'm fixing the test since it was not behaving correctly as an MS would. I stated in the commit I also fixed/improved some tests while moving to the new API.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@609 
PS5, Line 609: f_shutdown
> Unrelated change. AFAIR, I intentionally did not break here.
I think this is fine. You want me to remove it? ok, I will remove it.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@685 
PS5, Line 685: f_shutdown
> Again unrelated. […]
Again, I think it's fine since we always want to terminate immediately after the error, having afterwards calling f_shutdown with final := true is confusing.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@730 
PS5, Line 730: f_shutdown
> Same.
Same.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@747 
PS5, Line 747: ms := g_ms[0]; /* We only use first MS in this test */
> While it could be just one line: var GprsMS ms := valueof(t_GprsMS_def) or so.
Yes for these tests, not for future ones.

https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/5/pcu/PCU_Tests.ttcn@1511 
PS5, Line 1511: f_TC_egprs_pkt_chan_req
> Same here, we don't really need MS/TBF/... abstraction here.
Same, it makes sense using same set of tools so people can easily understand tests and develop them.
The old APIs will be eventually dropped.

-- 
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: Thu, 21 May 2020 22:21:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
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/20200521/10f57c11/attachment.htm>