Attention is currently required from: dexter, laforge. fixeria has posted comments on this change by dexter. ( https://gerrit.osmocom.org/c/pysim/+/39199?usp=email ) Change subject: global_platform: add new command "install_cap" ...................................................................... Patch Set 7: (6 comments) File docs/cap-tutorial.rst: https://gerrit.osmocom.org/c/pysim/+/39199/comment/e70f3a56_000afc68?usp=em… : PS7, Line 5: support "do support ... support" https://gerrit.osmocom.org/c/pysim/+/39199/comment/3b844dd9_e0495a8a?usp=em… : PS7, Line 54: "maximum_length_of_data_field_in_command_message": 255 shouldn't there be an additional indention level? File pySim/global_platform/__init__.py: https://gerrit.osmocom.org/c/pysim/+/39199/comment/151e93f6_b121b29e?usp=em… : PS7, Line 868: install Parameters (GPC_SPE_034, section 11.5.2.3.7, table 11-49 looks like copy paste. Maybe clarify that it's one of the installation params setting the volatile memory quota? https://gerrit.osmocom.org/c/pysim/+/39199/comment/e771db3a_fe185277?usp=em… : PS7, Line 865: install_cap_parser.add_argument('--install-parameters', type=is_hexstr, default='', : help='install Parameters (GPC_SPE_034, section 11.5.2.3.7, table 11-49)') : install_cap_parser.add_argument('--install-parameters-volatile-memory-quota', type=int, default=None, : help='install Parameters (GPC_SPE_034, section 11.5.2.3.7, table 11-49)') : install_cap_parser.add_argument('--install-parameters-non-volatile-memory-quota', type=int, default=None, : help='Install Parameters (GPC_SPE_034, section 11.5.2.3.7, table 11-49)') : install_cap_parser.add_argument('--install-parameters-stk', type=is_hexstr, default=None, : Like in previous patch, define them as mutually exclusive params? https://docs.python.org/3/library/argparse.html#mutual-exclusion. File pySim/global_platform/install_param.py: https://gerrit.osmocom.org/c/pysim/+/39199/comment/525ddd5a_7316dc97?usp=em… : PS7, Line 20: from construct import Optional as COptional : fr none of these used? File tests/unittests/test_globalplatform.py: https://gerrit.osmocom.org/c/pysim/+/39199/comment/7cf3087c_1252dfb5?usp=em… : PS7, Line 24: * you only need `gen_install_parameters()`, so import it alone? Otherwise you're importing everything, including imports of the given module. -- To view, visit https://gerrit.osmocom.org/c/pysim/+/39199?usp=email To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: pysim Gerrit-Branch: master Gerrit-Change-Id: I6cbd37f0fad5579b20e83c27349bd5acc129e6d0 Gerrit-Change-Number: 39199 Gerrit-PatchSet: 7 Gerrit-Owner: dexter <pmaier(a)sysmocom.de> Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge <laforge(a)osmocom.org> Gerrit-CC: fixeria <vyanitskiy(a)sysmocom.de> Gerrit-Attention: laforge <laforge(a)osmocom.org> Gerrit-Attention: dexter <pmaier(a)sysmocom.de> Gerrit-Comment-Date: Mon, 20 Jan 2025 12:37:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No