Patch Set 3: (6 comments)
> please open a bug report in pydbus If you look at the issue linked to in this commit log you will notice that I have done so on wednesday. https://github.com/LEW21/pydbus/issues/56 https://gerrit.osmocom.org/#/c/2733/3/src/osmo_gsm_tester/ofono_client.py File src/osmo_gsm_tester/ofono_client.py: Line 161: self._dbus_obj = None > So, if I understand correctly the old object in maintained alive by python I infer so, because signal subscriptions remain active. Python GC should clean it up once the signal subscription is not referenced anymore. Line 236: def is_online(self, *args, **kwargs): > args and kwargs not needed here copy-pasto :) Line 260: def set_powered(self, *args, **kwargs): > why all this args and kwargs in these functions? looks wrong to me. The intention was to redirect and not have to keep code in sync. Agreed, for the sake of providing a clear API to test scripts it makes more sense to have explicit args. Line 292: event_loop.wait(self, lambda: not self.dbus.has_interface(I_NETREG, I_SMS), timeout=10) > is this really needed? I'd better wait(lambda: not is_powered) the is_powered returns the desired value immediately, while the signals that disable interfaces keep coming in after that. So I want to make sure that we are not setting powered to true again before those other signals aren't done and processed. IOW, yes, unfortunately necessary. Line 295: event_loop.wait(self, self.dbus.has_interface, I_NETREG, I_SMS, timeout=10) > Same here, wait(is_online) I am adding a requirement for features 'sms' and 'net' to be present, but keeping as is otherwise. I want to pre-shadow use of I_NETREG for your patch (kind of verifying that it works), in the same way that the code did before this patch. I am assuming that all of our modems do provide SMS functionality. After all, any modem that doesn't is utterly useless in the osmo-gsm-tester. I also want to check I_NETREG because so far all of our modems do provide it (and I expect all future ones to do so as well), but our code always ended up saying that there wasn't one. I am really fed up with nonsense like this, we have better things to do than keep working around this problem. We can talk about this again when there are modems to be added that don't provide some of these interfaces. Line 302: if not self.dbus.has_interface(I_SMS): > As statedaboce , better first check "sms" feature,then if it appears wait() I'll actually remove this condition here, it was a previous kind of workaround for the useless 'keyError' exception. After startup, I want to know that all features we use are definitely available, and no more conditions in the rest of the code. -- To view, visit https://gerrit.osmocom.org/2733 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia36b881c25976d7e69dbb587317dd139169ce3d9 Gerrit-PatchSet: 3 Gerrit-Project: osmo-gsm-tester Gerrit-Branch: master Gerrit-Owner: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: Neels Hofmeyr <nhofm...@sysmocom.de> Gerrit-Reviewer: Pau Espin Pedrol <pes...@sysmocom.de> Gerrit-HasComments: Yes
