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

[Stephen Warren]

On 04/20/2016 03:40 AM, Lukasz Majewski wrote:
> Hi Stephen,
>
>> On 04/19/2016 09:51 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 the ability to set different values for them.
>>
>>> @@ -122,6 +139,8 @@ def test_dfu(u_boot_console, env__usb_dev_port,
>>> env__dfu_config): Returns:
>>>                Nothing.
>>>            """
>>> +        global alt_setting_test_file
>>> +        global alt_setting_dummy_file
>>
>> There should be a blank line after the """ line. Although per the
>> comments below, you can simply drop this part of the diff completely.
>>
>>> @@ -132,6 +151,9 @@ def test_dfu(u_boot_console, env__usb_dev_port,
>>> env__dfu_config): u_boot_console.log.action(
>>>                'Starting long-running U-Boot dfu shell command')
>>>
>>> +        alt_setting_test_file =
>>> env__dfu_config.get('alt_id_test_file', '0')
>>> +        alt_setting_dummy_file =
>>> env__dfu_config.get('alt_id_dummy_file', '1')
>>
>> This always over-writes alt_setting_test_file, and changes the type
>> from integer (as specified by the current global assignment added in
>> patch 1) to string. You may as well simply remove the "global" lines
>> added in this patch, and the global assignment, since this patch
>> always assigns a value to those variables.
>>
>> Since the variable always contains a string now, you can remove the
>> str() call from run_dfu_util()'s assignment to cmd[].
>
> Frankly I'm a bit confused now.
> I'm not the python expert, but v2 design seems logical to me:
>
>
> test_dfu.py Python module (outermost scope):
>
> alt_setting_test_file = 0
> alt_setting_dummy_file = 1
>
> def test_dfu:
> 	
> 	def start_dfu():
> 		global alt_setting_test_file
> 		global alt_setting_dummy_file
>
> 		alt_setting_test_file=env__dfu_config.get('alt_id_test_file','0')
> 		alt_setting_dummy_file=env__dfu_config.get('alt_id_dummy_file','1')
>
> 	def dfu_write_read_check():
> 		...
> 		dfu_write(alt_setting_test_file, test_f.abs_fn)
> 		...
> 		dfu_write(alt_setting_dummy_file, dummy_f.abs_fn)
> 		...
> 		dfu_write(alt_setting_test_file, dummy_f.abs_fn)
>
>
> So I'm setting up alt_setting_{test|dummy}_file global variables at
> start_dfu() function. Afterwards, I reuse them at
> dfu_write_read_check().
>
> Such approach is similar to one used for "first_usb_dev_port" global
> variable in the same file.

first_usb_dev_port is a different case; it's explicitly data that needs 
to be saved from one invocation of the test (on USB port A for example) 
to another (on USB port B). The alt setting data should only be used 
within a single test invocation.

> If I remove "global" from alt_setting_{test|dummy}_file declaration
> then I will set them only to be valid at start_dfu() function scope.
> Hence at dfu_write_read_check() I will use those variables set to 0 and
> 1 respectively.
>
> One possible solution that I've found is to use "function attributes":
>
> def test_dfu:
> 	test_dfu.alt_setting_test_file
> 	test_dfu.alt_setting_dummy_file
>
> 	def start_dfu():
> 		test_dfu.alt_setting_test_file=env__dfu_config.get('alt_id_test_file','0')
> 		test_dfu.alt_setting_dummy_file=env__dfu_config.get('alt_id_dummy_file','1')
>
> 	def dfu_write_read_check():
> 		...
> 		dfu_write(test_dfu.alt_setting_test_file, test_f.abs_fn)
> 		...	
>
> Stephen, is the above solution acceptable for you?

I would expect the top-level test_dfu() function to read the 
configuration values. start_dfu() and dfu_write_read_check() can simply 
read them. That way, you wouldn't need the references to be 
"test_dfu.xxx" just "xxx". Se how dummy_f is assigned within the outer 
function, and the nested functions read that value.