Attention is currently required from: laforge. neels has posted comments on this change by neels. ( https://gerrit.osmocom.org/c/pysim/+/39744?usp=email ) Change subject: [4/6] personalization: refactor Pin, Adm ...................................................................... Patch Set 3: (2 comments) File pySim/esim/saip/personalization.py: https://gerrit.osmocom.org/c/pysim/+/39744/comment/3f2c4e54_a9a4b3ed?usp=em… : PS3, Line 408: return (pe for pe in l if pe.type == wanted_type) No! First, apply_val() looks up ~~~ pes.pes_by_naa['mf'] ~~~ The result looks like this: ~~~ [[<pySim.esim.saip.ProfileElementMF object at 0x7fcd2285e3c0>, <pySim.esim.saip.ProfileElementPuk object at 0x7fcd2285e900>, <pySim.esim.saip.ProfileElementPin object at 0x7fcd2285ea50>, <pySim.esim.saip.ProfileElementTelecom object at 0x7fcd2285eba0>, <pySim.esim.saip.ProfileElementGFM object at 0x7fcd2285ee40>, <pySim.esim.saip.ProfileElementPin object at 0x7fcd22856850>, <pySim.esim.saip.ProfileElementGFM object at 0x7fcd22856990>]] ~~~ It then picks the first element [0] which is now a ~~~ <class 'list'> ~~~ So now we have just a py list of various ProfileElement subclass instances. Not a ProfileElementSequence. I can do ~~~ [<pySim.esim.saip.ProfileElementPin object at 0x7fcd2285ea50>, <pySim.esim.saip.ProfileElementPin object at 0x7fcd22856850>, <pySim.esim.saip.ProfileElementPin object at 0x7fcd22856ad0>, <pySim.esim.saip.ProfileElementPin object at 0x7fcd22845cd0>, <pySim.esim.saip.ProfileElementPin object at 0x7fcd22845940>] ~~~ But the result is different from the code I found, which looks up only the 'mf' naa: ~~~ [<pySim.esim.saip.ProfileElementPin object at 0x7fcd2285ea50>, <pySim.esim.saip.ProfileElementPin object at 0x7fcd22856850>] ~~~ There are two questions for me: 1. Is it correct to limit to 'mf'? Until now I didn't know there were other pinCodes in other naas too. (I did not create this code, just working with code I got) 2. the pySim API: it seems slightly quirky design to have a ProfileElementSequence class, that in turn contains plain lists. That way those lookup members of ProfileElementSequence are not available in inner list levels. Would it make sense to use ProfileElementSequence instances on all inner lists, too? I think not, because it falls out of the asn1 decoding like this, right? Does it maybe make sense to have *only* global lookup functions that work on both ProfileElementSequence and list(ProfileElement) to avoid code dup? that would then defy those lookup dicts. Then again, are those dicts important, like, do they dramatically decrease lookup time? If we answer 1. with yes, limit to 'mf', and 2 with no, it's fine just as it is, then this patch is also fine as it is -- besides maybe moving the new function to a more prominent place. What are your thoughts? Thanks! https://gerrit.osmocom.org/c/pysim/+/39744/comment/7d1ea426_9bf78ee5?usp=em… : PS3, Line 452: def _apply_pinvalue(pe: ProfileElement, keyReference, val_bytes): it is indeed a List[ProfileElement] as i found out now .. related to my long comment. closing this speech bubble so we have one place to discuss. -- To view, visit https://gerrit.osmocom.org/c/pysim/+/39744?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: I54aef10b6d4309398d4b779a3740a7d706d68603 Gerrit-Change-Number: 39744 Gerrit-PatchSet: 3 Gerrit-Owner: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Attention: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Comment-Date: Thu, 13 Mar 2025 21:15:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels &lt;nhofmeyr(a)sysmocom.de&gt; Comment-In-Reply-To: laforge &lt;laforge(a)osmocom.org&gt;