[U-Boot] [PATCH 3/3] tests: py: dfu: Provide functionality to set test and dummy files alt settings

[Stephen Warren]

On 04/11/2016 02:42 AM, Lukasz Majewski wrote:
> Hi Stephen,
>
>> On 04/08/2016 09:44 AM, Lukasz Majewski wrote:
>>> After concatenation of "dfu_alt_info" variable from "dfu_alt_boot"
>>> and "dfu_alt_system" it may happen that test and dummy files alt
>>> settings are different than default 0 and 1.
>>>
>>> This patch provides ability to set different values for them.
>>> It was the simplest possible solution - akin to the one from
>>> original bash dfu tests.
>>
>>> diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py
>>
>>> +        # - after concatenation dfu alt settings for test and
>>> dummy files are
>>> +        #   moved from 0 and 1 to other values
>>
>> Similar formatting comments to the previous patch. I'd also re-word
>> this to be much more generic, and simply state the it allows
>> different alt settings to be used, rather than tieing the description
>> to one possible reason why you might want to do that.
>
> Ok, I will rewrite the description.
>
>>
>>> +        "alt_num_test_file": "5",
>>> +        "alt_num_dummy_file": "6",
>>
>> This feels fragile. What if $dfu_alt_boot changes length? Does it
>> make more sense to:
>>
>> (a) Set alt_info_env_name to dfu_alt_boot instead, so that the
>> settings specified by the test are always at a known position in the
>> list, so we can always use alt setting 0 and 1.
>
> Unfortunately, dfu_alt_boot (which depends on boot medium) is
> immutable and set during boot time from CONFIG_DFU_ALT_BOOT_SD and
> CONFIG_DFU_ALT_BOOT_EMMC
>
> Only "dfu_alt_system" can be set by user.

I don't see anywhere in the code to enforce that. While dfu_alt_boot 
does appear to be set at boot based on CONFIG_DFU_ALT_BOOT_*, there 
doesn't appear to be anything that forces the variable to be read-only.

>> or:
>>
>> (b) Use names rather than numbers for the alt setting?
>
> I thought about this option.
>
> 1. One possible solution would be to define:
>
> "test_file_name": "file.bin"
> "dummy_file_name": "dummy.bin"
>
> at env__dfu_configs.
>
> Afterwards, I could automatically set the "alt_info" property int the
> same map:
>
> "alt_info" : "%s ext4 0 2; %s ext4 0 2" % (test_file_name,
> dummy_file_name)

I assume you're using the % operator here so that test_file_name can be 
shared with the other config options that define the alt setting 
names/IDs. That won't work exactly as you've written it, since 
"test_file_name" isn't a reference to the env__dfu_configs variable, and 
indeed that variable can't be accessed at that location in the code. I 
think you'd need something like:

test_file_name = "file.bin"
dummy_file_name = "dummy.bin"

env__dfu_configs = (
     {
         "fixture_id": "emmc",
...
         test_alt_id: test_file_name,
         dummy_alt_id: dummy_file_name,
         "alt_info" : "%s ext4 0 2; %s ext4 0 2" % (
             test_file_name, dummy_file_name)

     },
)

> As a result, I could use -a {test_file_name|dummy_file_name} to access
> proper alt setting.
>
>
>
> 2. Another option would be to set the "dfu_alt_system" env variable and
> then run "dfu-util -l" on host (or 'dfu 0 mmc 0 list') on target and
> grep output for test_file_name and dummy_file_name and extract alt
> setting numbers.
> This would require modification in the framework core code and hence I
> decided to manually specify the alt settings number (as I did in the
> bash version of those scripts).
>
> However, as I look now for it, I think that option from point 1) seems
> flexible enough. Stephen what do you think about it?

Option 1 does seem simplest.

>> Those should
>> be position-independent. Presumably this would require a slightly
>> large code change, since we'd need to move from %d to %s conversions
>> when constructing the dfu command string, but that should be very
>> easy.
>>
>> If you take this approach, I'd suggest making the configuration file
>> name (alt_num_*_file above) match the Python variable name
>> (alt_setting_*_file) for consistency.
>>
>> (c) Provide a way for the user to turn off the auto-concatenation
>> feature.
>
> This feature is already deployed and I would like to avoid changing it.

I wasn't suggesting removing it, rather making it read another 
environment variable that allows disabling the feature at run-time. 
That'd be something like the following at the start of the function that 
implements it:

if (getenv("dfu_alt_disable_auto_generation"))
     return;