<blockquote style="border-left: 1px solid #aaa; margin: 10px 0; padding: 0 10px;"><p style="white-space: pre-wrap; word-wrap: break-word;">Patch Set 2: Code-Review-1</p><p style="white-space: pre-wrap; word-wrap: break-word;">(2 comments)</p><p style="white-space: pre-wrap; word-wrap: break-word;">I think this needs improvement, also I think this may be partially redundant to the generic SIM/USIM support for pySim-shell.py, which I am currently working on.</p><p style="white-space: pre-wrap; word-wrap: break-word;">First of all we consciously decide the APDU parameters for each card and I think it should be kept that way. All those classes have autodetect methods which essentially go by the ATR. I plan to have generic classes SimCard, IsimCard and UsimCard. Those will have autodetect methods that will check weather it is a UICC or not. If it is a UICC it will further check if it is an USIM or ISIM by looking at the app-ids in EF.DIR</p><p style="white-space: pre-wrap; word-wrap: break-word;">I think it makes sense to use detect_card_type() for the first step (distinguishing from UICC), but I think the code is much more readable when the parameter change takes place in the autodetect method.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Once the detection is inside the base classes things will be simpler the code will only call card_detect() and get either a specific card or a generic SimCard, IsimCard or UsimCard back and the APDU parameters will also be adjusted already.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Its not very far yet, but here is the change-id: Id6527254a7305516f2d00876815e46a3c71a81b6</p><p style="white-space: pre-wrap; word-wrap: break-word;">regards.<br>Philipp</p></blockquote><p style="white-space: pre-wrap; word-wrap: break-word;">Thanks for the feedback, didn't want to be too invasive.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I think your suggestion is the way to go and actually will get obsolete if you add your changes.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Actually, I was working on the same issue - here ist my change-id: I862c42f2c3805886f18214d8b4c39da55a735473</p><p style="white-space: pre-wrap; word-wrap: break-word;">But I think, it makes more sense to make those function non-classmethods as you did.</p><p style="white-space: pre-wrap; word-wrap: break-word;">Nevertheless, I suggest to (optionally) support multiple different detection methods, in case ATR is not available in transport layer (e.g. AT commands).</p><p><a href="https://gerrit.osmocom.org/c/pysim/+/24160">View Change</a></p><ul style="list-style: none; padding: 0;"></ul><p>To view, visit <a href="https://gerrit.osmocom.org/c/pysim/+/24160">change 24160</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/c/pysim/+/24160"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: pysim </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-Change-Id: I224bd6c91cdaf444e2a94117f64030c024a776a7 </div>
<div style="display:none"> Gerrit-Change-Number: 24160 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Falkenber9 <robert.falkenberg@tu-dortmund.de> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder </div>
<div style="display:none"> Gerrit-Reviewer: dexter <pmaier@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Thu, 06 May 2021 11:25:37 +0000 </div>
<div style="display:none"> Gerrit-HasComments: No </div>
<div style="display:none"> Gerrit-Has-Labels: No </div>
<div style="display:none"> Gerrit-MessageType: comment </div>