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

[Lukasz Majewski]

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.

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?

-- 
Best regards,

Lukasz Majewski

Samsung R&D Institute Poland (SRPOL) | Linux Platform Group