This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
Pau Espin Pedrol gerrit-no-reply at lists.osmocom.org
Patch Set 3: Code-Review-1
(7 comments)
I really don't like playing like this wit the dbus proxy objects, but I understand it's most probably a bug (or feature missing) in pydbus and for now we need to workaround it.
This means, I'm OK with merging this but please open a bug report in pydbus explaining the situation and asking them to fix it, because all tis adds a lot of complexity which should be probably more easily fixed inside pydbus by introspecting the dbus object again inside the API in the case of KeyError. I'll feel happier once there is a proper fix for this in pydbus and we can depend on it and throw all this re-creation of dbus proxies.
I mark it with a -1 because some stuff needs fixing or clarification.
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 untill all signal tokens as disconnect()ed because they maintain a ref of the old dbus_obj?
Line 182: for subscription in self.connected_signals.get(interface_name, []):
for subscription in got
Line 236: def is_online(self, *args, **kwargs):
args and kwargs not needed here
Line 260: def set_powered(self, *args, **kwargs):
why all this args and kwargs in these functions? looks wrong to me.
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)
Line 295: event_loop.wait(self, self.dbus.has_interface, I_NETREG, I_SMS, timeout=10)
Same here, wait(is_online)
And in any case don't check for I_NETREG now as we don't use it. I have code in the latest netreg commit (not in gerrit now) to handle this.
On top, be careful because I_NETREG and I_SMS are not mandatory interfaces, which means we should first check the "sms" feature in Modem.GetProperties('Features'), then check if the iface is there. Also, for the feature to appear it can take some time, so I'd go for check that only when we want to use it, as I did with SimManaer on my modem-discovery patch. Wait for it with wait() with timeout of 5 seconds or similar, otherwise raise exception saying feature is not supported.
Line 302: if not self.dbus.has_interface(I_SMS):
As statedaboce , better first check "sms" feature,then if it appears wait() for long time until the iface is there.
But this can be done in a separate patch, I'd go remove the wait()s you added above or change them
--
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 <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-HasComments: Yes