osmo-gsm-tester[master]: ofono_client: Implement network registration during connect()

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
Wed May 31 21:00:12 UTC 2017


Patch Set 6:

(19 comments)

https://gerrit.osmocom.org/#/c/2779/3/src/osmo_gsm_tester/ofono_client.py
File src/osmo_gsm_tester/ofono_client.py:

Line 39: 
> maybe also have the NETREG_ prefix for this value?
Done


Line 346:             'Type': 'hardware',
> it's IMHO nice to have this, but maybe in a separate patch because it is te
ACK


Line 351:     def set_powered(self, powered=True):
> Rather catch the specific exception you are expecting here. KeyError?
Done


Line 353: 
> why store netreg_status in self?
I'll remove it.


Line 355:         return self.dbus.set_online(online=online)
> do we need this?
Yes, afaik we need to check it in is_connected() in the case another mnc is used I guess. It doesn't harm and may be useful later if we want to test roaming related stuff.


Line 365: 
> self.raise_exn('Failed to find Network Operator', mcc=self.mcc, mnc=self.mn
Done


https://gerrit.osmocom.org/#/c/2779/6/src/osmo_gsm_tester/ofono_client.py
File src/osmo_gsm_tester/ofono_client.py:

Line 83: def __async_result_handler(obj, result, user_data):
> leading double underscores should be avoided, use one underscore
ACK


Line 107: def dbus_async_call(proxymethod, instance, *args, **kwargs):
> I would prefer writing out the kwargs timeout, result_handler, error_handle
ACK


Line 401:         # waiting for that. Make it async and try to regsiter when the scan is
> (typo regsiter)
ACK


Line 407:         self.raise_exn()
> no message? Let's make it an explicit scan_error_cb and have a meaningful m
ACK


Line 409:     def scan_result_cb(self, obj, result, user_data):
> 'result' should have a meaningful name. scanned_operators?
I prefer to leave it as result here to understand better that this is the result of the dbus method, then can easily be seen on register() it's the "scanned_operators". I'll change the one in register()


Line 412:     def register(self, ops):
> 'ops' should be more descriptive: scanned_operators?
ACK


Line 414:         if self.mcc is None or self.mnc is None:
> better: self.mcc_mnc is None
is the following true? None == (None, None)

If it's true we can change it, otherwise we need to keep it like this


Line 415:             if not self.is_connected():
> shouldn't this check be independent of whether mcc+mnc were passed or not?
1- No, because here we only check whether we are connected to ANY network, which means it's fine as we didn't specify one and we are fine as long as we are connected to one no matter which.

2- I think it's better here. As explained in point 1-, this check only applies for one of the 2 cases (default registering) and the code around the scanning is generic, otherwise I'd need to check there too whether we want to connect to default network or to a specific one. As now the scanning is done async, doesn't seem like a big issue anymore.


Line 427:                     self.log('Already registered with the network')
> should we also check this before we start scanning?
No, we need to keep this here to avoid re-registering in case the modem got registered at some point between pre-scan and here, which can happen afaiu (remember the modem attempts to connect once it becomes Online, which means it may connect at some point in between).

We could duplicate the check, do it before the scan too, but that would mean adding code again to find the Operator path, getting the Properties, etc. so lots of new code and complexity for not much gain.

I agree, like it is now we may call Scan() some time which is not needed, but doesn't look like a big issue anymore now that the call is async. Give it a try, it's quite quick now, I think even faster than before sometimes, I think when we hit the case in which we call Register explicitly instead of just waiting for the modem to decide to register at some point in time.


Line 454:         self.mcc, self.mnc = mcc_mnc
> why not also store as self.mcc_mnc
because I need to check the separately (I think unless the following is true: None == (None, None))
Also because at some point anyway I need to use them separately to check for equality against ofono properties.


Line 455:         if (self.mcc is None and self.mnc is not None) or (self.mcc is not None and self.mnc is None):
> len(mcc_mnc) != 2 or None in mcc_mnc
imho input validation should be done even before, when checking/reading the config.

Still some stuff I don't get here. Is the following true?
len('123', None) == 1
len(None, None) == 0


https://gerrit.osmocom.org/#/c/2779/6/suites/debug/interactive.py
File suites/debug/interactive.py:

Line 30:          wait(m.is_connected)
> we could reverse the order, i.e. first wait for the modems to be connected.
As far as I know, the registration confirmation comes from MSC/BSC and goes downlink, so I think it still make sense to check following the order going downstream, so first check MSC/BSC, then MS.

No strong opinion though, at the end it should be the same. Unless something fails and you need to debug, in which case makes more sense to first pass through the MSC/BSC check then wait for the modem. If the traceback comes from wait(is_connected), then you know the issue is at some point later than the app-level of MSC/BSC.

What do you think? Shall I change it or keep it like this? I'd prefer keeping it like this.


https://gerrit.osmocom.org/#/c/2779/6/suites/netreg/register.py
File suites/netreg/register.py:

Line 16: ms.connect(nitb.mcc_mnc())
> wait(is_connected)?
It's at the end of the file, line 21


-- 
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: 6
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



More information about the gerrit-log mailing list