Attention is currently required from: JPM, dexter.
fixeria has posted comments on this change by JPM. ( https://gerrit.osmocom.org/c/pysim/+/38993?usp=email )
Change subject: Modem related fixes. ......................................................................
Patch Set 1: Code-Review-1
(5 comments)
Patchset:
PS1: This patch should be split into smaller ones, ideally with some description and motivation in their COMMIT_MSGs.
File pySim/cards.py:
https://gerrit.osmocom.org/c/pysim/+/38993/comment/cf65b68a_87491944?usp=ema... : PS1, Line 43: return [] #return empty ATR This change looks suspicious. The type hint for this function is `Optional[Hexstr]`, indicating that it returns either a `Hexstr` or `None`. Now you're changing it to return a `list` instead of `None`. Why? What's the goal?
File pySim/transport/modem_atcmd.py:
https://gerrit.osmocom.org/c/pysim/+/38993/comment/afe707b3_cd79d3ee?usp=ema... : PS1, Line 59: timeout=1.5, patience=0.1 This should be proposed in a separate patch with some explanation why the values need to be increased.
https://gerrit.osmocom.org/c/pysim/+/38993/comment/94c922c9_75f0d08a?usp=ema... : PS1, Line 111: def get_atr(self) -> Hexstr: This also looks like a separate patch. I see we also have `get_atr()` in `PcscSimLink` and `SerialSimLink`, but not in `CalypsoSimLink`. Neither this method is defined in `LinkBase` (the abstract class). I don't remember seeing this method when writing this module...
https://gerrit.osmocom.org/c/pysim/+/38993/comment/8643752e_9d3cd0b3?usp=ema... : PS1, Line 137: Rebooting the modem to force card reset AFAICS, you're reading the current `AT+CFUN` (operational) mode and then sending `AT+CFUN` with exactly that same mode value that was read. Could you explain and/or give a spec. reference on how is that supposed to reset the card?
I would expect doing `AT+CFUN=0` followed by `AT+CFUN=1` to reset the card, since the modem would likely power off the card to ensure that the "minimum power is drawn".