Attention is currently required from: fixeria, laforge.

View Change

1 comment:

• File pySim/esim/saip/batch.py:

  • Patch Set #11, Line 143: {'001010000000023'}

    My point is: There are multiple constellations in which there could be multiple IMSIs. […]

    Ok, I wasn't aware of the possibility...

    An exception is not a good way to solve this, because it interrupts the audit. The audit should, without imposing constraints, show what it can find. The exception would happen in the code that evaluates the audit result.

    
    This is also used in ConfigurableParameters tests: on error, I get a missing/empty or mismatching value for a given parameter name, which the test then complains about after the entire audit is done.

    Multiple ADF.USIM opens a profound discussion on how the ConfigurableParameter API works:
    So far the idea is that there is a single value in a PES per ConfigurableParameter (that can occur multiple times).

    For ADF.USIM + ADF.ISIM, my first idea was to keep this current Imsi() ConfigurableParameter for USIM, and add a new ImsiIsim() for IMSIs kept in ADF-ISIM, but..

    But you also mention multiple USIM entries, where each USIM would reflect separate data from identical PE structures. This is beyond the scope of the current API design: our ConfigurableParameters are not able to distinguish between different ADF-USIM.

    
    To support that, we need to refactor/enhance the ConfigurableParameter API to support selectors for different entries of the same name (like an ADF index?).
    My first idea is to pre-select subsections of a PES before applying ConfigurableParameter:

    For example, currently we have a flat list of params:

        BatchPersonalization().add_param_and_src(Imsi, IncDigitSource(...))

    we could change it to a tree where each Imsi() has a reduced scope:

        BatchPersonalization().add_param_and_src_limited(
            limit_to={"adf":"usim", "index": 0},
            Imsi, IncDigitSource(first_usim_imsi...))
            
        BatchPersonalization().add_param_and_src_limited(
            limit_to={"adf":"usim", "index": 1},
            Imsi, IncDigitSource(second_usim_imsi...))

        BatchPersonalization().add_param_and_src_limited(
            limit_to={"adf":"isim"},
            Imsi, IncDigitSource(isim_imsi...))

    
    So that we pre-select the scope before applying the ConfigurableParameter instances, so that the ConfigurableParameter API does not need to know the difference.

    For the audit part, calling code could also pre-select a scope in which to perform the audit, then run two separate audits: one for USIM and one for ISIM.

    
    Simpler for audit: run one Imsi() across all ADF and expect a set of exactly the two IMSIs, without checking in detail whether each was in the expected place.

    
    (How to handle this would be up to the calling code)

    More complex idea: the audit could indicate in detail where it found the values, by some sort of xpath: instead of a set, return a dict like

    
    ~~~~
       {
         'IMSI': {
           'usim[0]/ef-imsi': '098765432123456',
           'usim[1]/ef-imsi': '098765432123457',
           'isim[0]/ef-imsi': '987654345678765',
           },
         'IMSI-ACC': {
           'usim[0]/ef-acc': '5',
           'usim[1]/ef-acc': '5',
           'isim[0]/ef-acc': '7',
           }
       }
    ~~~~

    The caller can then still form a set() on the dict values to still follow the current logic, and can decide when to implement distinguishing by location.
    However, it would be much simpler for callers to first constrain to one "usim" and then run on the current logic that expects a single value.

    
    This design discussion is a bit out of place in a CR comment, because it has a larger scope than this patch.
    How important is the use case of multiple USIM in the same PES? -- all profiles I've seen so far have one ADF-USIM.
    When we continue this, I'll make a redmine issue and we should probably have a planning meeting first.

    
    Given that this code is in active use, my "tactical" decision would be to merge this as-is, accepting the constraints that we do not yet support multiple ADF.USIM, as simple fact of the initial ConfigurableParameter design that has been in pysim before I started. I would create a redmine issue to enhance the API as soon as we encounter the need to add support for multiple distinct ADF.USIM in practice. Does that make sense to CRers?

To view, visit change 40208. To unsubscribe, or for help writing mail filters, visit settings.