Re: ModemManager [PATCH 2/2] Take 3 of: Improvements to SIM PIN handling - Add EnabledFacilityLocks property

[Aleksander Morgado <aleksander lanedo com>]

Hey hey,

> I guess I don't have a problem with merging them, although strictly
> speaking they refer to different things - locks vs. the PINs needed to
> unlock those locks.
> 
Yeah, you're totally right... but at the end the PIN/PUK codes are
directly related to facilities. E.g "SIM facility", has "PIN code" and
"PUK code". But it really seems broken to use the enum of codes as index
for facilities, not the best idea.
> 
> Two other points to consider regarding lock/PIN handling in the new
> API:
>       * PIN, PUK, PIN2, and PUK2 are PINs for SIM locks. The rest are
>         device locks, so having them belong to the *.Sim interface
>         doesn't seem completely right. But 3gpp gloms them all
>         together, so maybe we should just live with this.

In the 0.6 API they are not handled in the SIM interface. UnlockRetries
and PinRetryCounts are handled in the Modem interface, along with
UnlockRequired; and the EnabledFacilityLocks one is handled in the 3GPP
interface. Anyway, I'm not very convinced yet, on why we do keep
EnabledFacilityLocks in one interface and
UnlockRetries/PinRetryCounts/UnlockRequired in another one. Shouldn't we
have all in the 3GPP-specific interface? Are there CDMA-only modems with
facility locks?

>       * Shouldn't we add an argument to EnablePin and ChangePin that
>         specifies the lock to which the operation should apply? This
>         would mirror the CLCK and CPWD AT commands, which take the
>         facility name as an argument. This would correct what seems to
>         be an oversight in the current API.

Not an oversight, just that the main use case is to be able to
Enable/Change the SIM PIN, and the idea was to keep it simple. But now
that we report if given facility locks are enabled or disabled, it could
make sense to also allow trying to enable/disable all reported ones. We
would need to move Enable() and Change() out from the SIM object and
back to some interface in the Modem object.

And now that you talk about EnablePin() and ChangePin(); the same logic
could be applied to SendPin() and SendPuk(). SendPin()/SendPuk() should
be used only when UnlockRequired says that there is something to unlock;
they are actually backed by the same CPIN command. So, UnlockRequired
may be saying "ph-sim-pin" (phone-specific PIN required), and we can
then use SendPin() to send that specific code to the modem. Therefore,
SendPin() is really not related to the SIM, but to UnlockRequired, and
so they ought to be in the same interface (in the 0.6 API, SendPin() and
SendPuk() are managed in the SIM object).


And a new brainstorm here... given that we're going to break the DBus
API, we can try to rework and consolidate all related properties.

We could assume that each facility has 2 codes (PIN, PUK), and that the
PUK code will only be required once all PIN attempts have been used
(didn't look at any documentation, but that seems to me always the
case).

If we do assume this, we could then setup a "FacilityLocks" dictionary
with signature "a{ubuu}" where:
 * The uint32 in the first position identifies the facility.
 * The boolean in the second position tells whether the facility has a
lock.
 * The uint32 in the third position has the PIN retry count.
 * The uint32 in the fourth position has the PUK retry count.

Then, we could have UnlockRequired be just a uint32 where we identify
the 'facility' locked (as opposed to identifying which pin code we
need).

So, if we get "SIM" facility in UnlockRequired, we could then check the
corresponding entry in the FacilityLocks dictionary and see how many
retry counts we have for PIN and PUK. If retries for PIN is 0; it means
we need the PUK code, unless PUK retries is also 0, which would mean we
need to buy a new SIM card. Instead of just the key of the dictionary
entry, we could also have the UnlockRequired property be a (ubuu); which
would contain the whole entry of the dictionary.

Thoughts?

-- 
Aleksander