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/.
pespin gerrit-no-reply at lists.osmocom.orgpespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217 )
Change subject: Modems: Introduce Android UEs
......................................................................
Patch Set 1: Code-Review-1
(30 comments)
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1//COMMIT_MSG
Commit Message:
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1//COMMIT_MSG@9
PS1, Line 9: To expand the test capacities we would like to introduce
I'm really lacking here some description on the setup to understand how the android phones are used, how are they hooked to OGT.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/process.py
File src/osmo_gsm_tester/core/process.py:
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/process.py@391
PS1, Line 391: self.remote_port = remote_port
Please add the port param as a different patch prior to the android one (this patch).
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/process.py@400
PS1, Line 400: self.remote_user = 'root'
All of this needs to get out from here. In the end is simply setting the remote_user to root which can be passed as an argument...
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/process.py@419
PS1, Line 419: if is_ue_cmd:
this is not really UE/android specific, please introduce the port feature generically.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/process.py@464
PS1, Line 464: run_dir = run_dir.new_dir(name)
This is a separate patch.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/process.py@476
PS1, Line 476: run_dir = run_dir.new_dir(name)
This too, in the same patch as line 464.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/process.py@480
PS1, Line 480:
Different patch.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/remote.py
File src/osmo_gsm_tester/core/remote.py:
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/core/remote.py@31
PS1, Line 31: def __init__(self, run_dir, remote_user = 'root', remote_host = 'localhost', remote_cwd=None, remote_port=None):
This all goes into the generic "add port" patch.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/enb_srs.py
File src/osmo_gsm_tester/obj/enb_srs.py:
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/enb_srs.py@117
PS1, Line 117: MainLoop.sleep(3)
What's this?
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/iperf3.py
File src/osmo_gsm_tester/obj/iperf3.py:
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/iperf3.py@269
PS1, Line 269: if self._run_node.is_local() and ue.ue_serial:
This most probably needs to go into a subclass of RemoteProcess or Process or alike.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/iperf3.py@291
PS1, Line 291: None, self._run_node.ssh_port())
This goes into the "add port patch".
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py
File src/osmo_gsm_tester/obj/ms_android.py:
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@1
PS1, Line 1: # Author: Nils Fürste <nils.fuerste at softwareradiosystems.com>
Please add a Copyright header with license.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@38
PS1, Line 38: class AndroidUE(MS, srslte_common):
Please split all the methods in this class according to their scope. I know it's not done perfectly in lots of places but let's do it here since there's lots of code. Use the following categories, see obj/bts.py as an example:
##############
# PROTECTED
##############
########################
# PUBLIC - INTERNAL API
########################
###################
# PUBLIC (test API included)
###################
You can get an idea of what is what by looking at other ms classes and which methods are used by tests, or by other classes inside obj/ or core/.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@75
PS1, Line 75:
Drop space
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@80
PS1, Line 80: if self.enable_pcap:
use this boolean when calling this method, not here. Or at least use early return.
if not self.enable_pcap:
return
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@85
PS1, Line 85: diag_parser_proc.launch()
tis launch needs to go after the if/else. Actually.... you don't need to scp back if you are running locally... so you can drop the if and the else condition altogether..
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@97
PS1, Line 97: time.sleep(2)
Mainloop.sleep
You have actually Mainloop.wait(), you should use it here instead of this selfmade timeout + sleep you use here
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@103
PS1, Line 103: if not self._run_node.is_local():
Again, if you are scping the file it's because you are running remotely, so makes no sense to have this check here.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@116
PS1, Line 116: self.rx_monitor_proc.terminate()
You shouldn't need this if you make OGT "remember" to have it killed. You need that to make sure the process doesn't end up dangling when the test is done successfuly or fails.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@122
PS1, Line 122: if self.diag_monitor_proc:
It probably makes sense to have DiagMonitor as a different class either in this same file or a separate file. See for instance the pcap_recorder.py, or AbisIpFind, etc.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@134
PS1, Line 134: self.scp_back_pcap()
if not is local and pcap enabled, then self.scp_back_pcap()
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@152
PS1, Line 152: popen_args_clear_run_dir = ['sudo', 'rm', '-rf', '-v', str(self.remote_run_dir) + '/*', '||', 'true']
Please describe somewhere why is that needed but I'm not liking having a sudo rm -rf /* here ;)
Feel free to contact me and explain me your needs so we can work out the best way.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@167
PS1, Line 167: self.run_netns_wait("kill-adb", ['sudo', 'adb', 'kill-server'])
why is this run on a netns? I lack a description of the setup in the commit message.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@182
PS1, Line 182: emm_conn_chk_proc.launch()
launch below if/else?
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@218
PS1, Line 218: self.rx_monitor_proc.launch()
All these processes you launch should be monitored by OGT using self.testenv.remember_to_stop(self.process).
And please, rather split some code into different classes if possible.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/ms_android.py@227
PS1, Line 227: DIAG_PARSER = 'osmo-gsm-tester_androidue_diag_parser.sh'
QcDiag class?
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/run_node.py
File src/osmo_gsm_tester/obj/run_node.py:
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/src/osmo_gsm_tester/obj/run_node.py@33
PS1, Line 33: def __init__(self, type=None, run_addr=None, ssh_user=None, ssh_addr=None, run_label=None, remote_port=None):
All of this goe sto the "add port" commit.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/sysmocom/default-suites.conf
File sysmocom/default-suites.conf:
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/sysmocom/default-suites.conf@175
PS1, Line 175: - 4g:androidue at mi5g+srsenb-rftype@uhd+mod-enb-nprb at 25+mod-enb-txmode@1
That's definetly going to fail in our setup ;)
Better simply describe it as an example in the commit description.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/sysmocom/scenarios/androidue%40.conf
File sysmocom/scenarios/androidue at .conf:
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/sysmocom/scenarios/androidue%40.conf@3
PS1, Line 3: - label: ${param1}
name of the scenario doesn't make much sense, since the "label" is generic.
Rather name it "ms-label at .conf" or similar.
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/utils/bin/osmo-gsm-tester_androidue_conn_chk.sh
File utils/bin/osmo-gsm-tester_androidue_conn_chk.sh:
https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217/1/utils/bin/osmo-gsm-tester_androidue_conn_chk.sh@2
PS1, Line 2: while true; do
Is this some monitoring tool? Please describe what is it used for here.
--
To view, visit https://gerrit.osmocom.org/c/osmo-gsm-tester/+/21217
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-gsm-tester
Gerrit-Branch: master
Gerrit-Change-Id: Iab37a0de59d6643d21bced34ddca06ff40fa62df
Gerrit-Change-Number: 21217
Gerrit-PatchSet: 1
Gerrit-Owner: srs_andre <andre at softwareradiosystems.com>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Nils <nils.fuerste at softwareradiosystems.com>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Tue, 17 Nov 2020 17:59:57 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20201117/9f9ebbb8/attachment.htm>