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 7:
(16 comments)
https://gerrit.osmocom.org/#/c/2779/7//COMMIT_MSG
Commit Message:
Line 10:
> you could elaborate a bit on what is going on in this commit. Some say to h
ACK
https://gerrit.osmocom.org/#/c/2779/7/src/osmo_gsm_tester/ofono_client.py
File src/osmo_gsm_tester/ofono_client.py:
Line 90: etype, e = sys.exc_info()[:2]
> are you using etype? why not
ACK
Line 108: *proxymethod_args,
> indent?
ACK
Line 113: #def dbus_async_call(proxymethod, instance, *args, **kwargs):
> cruft?
oops, my bad, didn't see it.
Line 402: nr = self.dbus.interface(I_NETREG)
> (cosmetic: I read 'nr' as 'number'. netreg?)
ACK
Line 410: def scan_error_cb(self, obj, e, user_data):
> interesting question here: is scan_error_cb() called asyncly? Should it rat
AFAIK it's called by the glib mainloop, which means it's on the same thread and will be called by wait()->poll_glib()->glib_main_ctx.iteration()
Line 418: if not self.is_connected():
> is_connected() again checks "self.mcc_mnc is None" and so forth. Does it ma
I could check for both by doing self.is_connected(self.mcc_mnc) but I still need all the code afterwards for the 2nd case (manual register), and in there I can do the same check without doing any dbus call, just by doing the following check:
if op_prop.get('Status') == 'current':
self.log('Already registered with the network')
For the manual register, as I already have the scanned_operators and I can do the same check with those, there is no sense in querying again all the operators to check this here. I will add a comment saying it's checked afterwards.
Line 423: self.log('Already registered with the network')
> Osmo style: always early-exit instead of if-cascade:
ACK
Line 433: self.log('Already registered with the network')
> seems to me to duplicate the is_connected() semantics ... follows above com
As I said, I prefer having different code paths for both cases. I could even move the automatic code path to a sub-method register_automatic(), but not needed.
Line 434: # return and not try to re-connect, otherwise dbus method returns a fail
> "otherwise" meaning what? not return?? :)
It only tries to say that, but one could expect in the future when looking/modifying the code that it should be OK calling register if we are already regsitered resulting in a NO-OP, but it's not true, so I think it's still important to give some hint here.
Line 448: self.dbg('Powering on')
> (this dbg is a bit confusing. If it is already powered, we will in fact pow
I'll move it afterwards with an else:
Line 450: self.dbg('is powered')
> (log says "Powering on", "is powered" -- but instead the facts are: modem w
ACK
Line 459: 'Connect to MCC+MNC from NITB config'
> rather don't mention the NITB config at all.
ACK
Line 460: if mcc_mnc is not None and (len(mcc_mnc) != 2 or None in mcc_mnc):
> (personal preference: braces around each sub-condition)
ACK
Line 464: self.log('connect to', self.mcc_mnc if self.mcc_mnc else 'default network')
> this log could go in the 'else:' below? In the sense of unbloating the log:
ACK
https://gerrit.osmocom.org/#/c/2779/7/src/osmo_gsm_tester/osmo_msc.py
File src/osmo_gsm_tester/osmo_msc.py:
Line 40: self.config = None
> for things that are None I generally put them above in the, what is it call
ACK
--
To view, visit https://gerrit.osmocom.org/2779
To unsubscribe, visit https://gerrit.osmocom.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d9eb47eac1044550d3885adb55105c304b0c15c
Gerrit-PatchSet: 7
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Owner: Pau Espin Pedrol <pespin 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