<p>Patch set 1:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/18535">View Change</a></p><p>3 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/18535/1/src/osmo_gsm_tester/obj/ms_srs.py">File src/osmo_gsm_tester/obj/ms_srs.py:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/18535/1/src/osmo_gsm_tester/obj/ms_srs.py@314">Patch Set #1, Line 314:</a> <code style="font-family:monospace,monospace">        return pos_connected > pos_released</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">heh nice way of checking order.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/18535/1/src/osmo_gsm_tester/obj/ms_srs.py@319">Patch Set #1, Line 319:</a> <code style="font-family:monospace,monospace">    def is_emm_registered(self):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please keep the is_connected() and is_attached() APIs to it also matches the ones in other MS types (like the ofono one), so that we have a common interface as much as possible. The idea is to control a device/UE in a simialr way no matter if it's 2g/3G/4G so we can reuse as much as possible.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm fine with renaming "is_connected" to "is_registered" if you want, but then do it in all suite-tests and MS object classes.</p><p style="white-space: pre-wrap; word-wrap: break-word;">We have 2 APIs because in 2G there's difference between being CircuitSwitch attached and PacketSwitch Attached (in our osmocom implementations you first get CS registered, then request PS attachment). In your case if it doesn't make sense, return the same in both.</p><p style="white-space: pre-wrap; word-wrap: break-word;">(I know they should be mrked as abstract methods in the abstract class and they aren't, but we should at some point :P)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/18535/1/src/osmo_gsm_tester/obj/ms_srs.py@322">Patch Set #1, Line 322:</a> <code style="font-family:monospace,monospace">    def get_ipv4_addr(self):</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Please add this API as abstract method in superclass ms.py, and leave it unimplemented in ms_ofono.py:<br>raise log.Error('API not implemented!')</p><pre style="font-family: monospace,monospace; white-space: pre-wrap;">BTW, let's better use same API for both IPv4 and IPv6 addr:<br>def get_assigned_addr(self, ipv6=False):<br>    if ipv6:<br>      .... return ipv6 addr or raise not implemented<br>    else:<br>      .... return ipv4</pre></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/18535">change 18535</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/osmo-gsm-tester/+/18535"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-gsm-tester </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I6cd057e34b4df6a1a73695355dd6406d7e039546 </div>
<div style="display:none"> Gerrit-Change-Number: 18535 </div>
<div style="display:none"> Gerrit-PatchSet: 1 </div>
<div style="display:none"> Gerrit-Owner: srs_andre <andre@softwareradiosystems.com> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: pespin <pespin@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 28 May 2020 11:27:03 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-Has-Labels: Yes </div>
<div style="display:none"> Gerrit-MessageType: comment </div>