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 8: Code-Review+2 (3 comments) > I'm fine with having lower layer APIs, but let's add low layer APIs which > re-use data structures we have instead of having to pass parameters to them. It looks like we have different opinions on what is low level and what is high level. To me, your new stateful API is high level compared to the old code. Again, I am not against keeping all state parameters in a single record - this is definitely nice for those complex test cases you're mostly working on. Meanwhile, I am mostly writing simple test cases sending just a couple of messages to the IUT, so I don't really want/need any additional abstraction like MS object in my code. I am not asking you to keep using the old API for complex test cases, while you insist that everybody should use your new API. Let's make everybody equally happy, and keep at least some part of the old code, in particular those three functions. And finally, let's get this change merged. https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/8/pcu/GPRS_Components.ttcn File pcu/GPRS_Components.ttcn: https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/8/pcu/GPRS_Components.ttcn@361 PS8, Line 361: f_pcuif_tx_data_ind(data, ms.lqual_cb, fn); > So you are the one hating extra abstraction layers and you are re-adding one > here. Well, I would remove this function too. I was just afraid that your pending test cases (in a private branch, or on the local host) might be using it. In your original change, f_pcuif_tx_data_ind() was just renamed and moved here - no functional changes. I kept the original function for small and simple test cases, where I don't want to dance with GprsMS just because I need to send a DATA.ind. https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/8/pcu/GPRS_Components.ttcn@561 PS8, Line 561: function f_establish_tbf(out GsmRrMessage rr_imm_ass, > So now we'll have 2 APIs doing similart stuff (establishing a UL TBF), > f_establish_tbf and f_ms_esta […] We should just rename it to f_pcuif_tx_rach_rx_imm_ass(), because that's exactly what it does. Your high-level f_ms_establish_ul_tbf() gives you a TBF object, this function is low-level because it sends one message and receives another in response with a bit of pattern matching. https://gerrit.osmocom.org/c/osmo-ttcn3-hacks/+/18370/8/pcu/GPRS_Components.ttcn@591 PS8, Line 591: function f_pcuif_tx_data_ind(octetstring data, int16_t lqual_cb := 0, uint32_t fn := 0) > I precisely wanted to get rid of these APIs where you stil labstract stuff > but need to keep passing […] Is it really that problematic to pass one (mandatory) parameter? I would agree with you if there were many, but definitely not in this case. The second one is used only in those test cases which test the link quality adaptation, while 'fn' is never passed explicitly in the current code. -- 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: 8 Gerrit-Owner: pespin <[email protected]> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: fixeria <[email protected]> Gerrit-Reviewer: laforge <[email protected]> Gerrit-Reviewer: pespin <[email protected]> Gerrit-Comment-Date: Thu, 28 May 2020 16:51:52 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: pespin <[email protected]> Gerrit-MessageType: comment
