[U-Boot] [PATCH 1/3] tests: py: dfu: Add global variables to store dfu alt numbers for test and dummy files

[U-Boot] [PATCH 1/3] tests: py: dfu: Add global variables to store dfu alt numbers for test and dummy files

[Lukasz Majewski]

This patch replaces hardcoded (i.e. 0 and 1) values passed to dfu_{read|write}
with global variables.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
 test/py/tests/test_dfu.py | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py
index 093e8d0..df3b561 100644
--- a/test/py/tests/test_dfu.py
+++ b/test/py/tests/test_dfu.py
@@ -80,6 +80,10 @@ test_sizes_default = (
 
 first_usb_dev_port = None
 
+# Default alt settings for test and dummy files
+alt_setting_test_file = 0
+alt_setting_dummy_file = 1
+
 @pytest.mark.buildconfigspec('cmd_dfu')
 def test_dfu(u_boot_console, env__usb_dev_port, env__dfu_config):
     """Test the "dfu" command; the host system must be able to enumerate a USB
@@ -229,15 +233,15 @@ def test_dfu(u_boot_console, env__usb_dev_port, env__dfu_config):
 
         u_boot_console.log.action('Writing test data to DFU primary ' +
             'altsetting')
-        dfu_write(0, test_f.abs_fn)
+        dfu_write(alt_setting_test_file, test_f.abs_fn)
 
         u_boot_console.log.action('Writing dummy data to DFU secondary ' +
             'altsetting to clear DFU buffers')
-        dfu_write(1, dummy_f.abs_fn)
+        dfu_write(alt_setting_dummy_file, dummy_f.abs_fn)
 
         u_boot_console.log.action('Reading DFU primary altsetting for ' +
             'comparison')
-        dfu_read(0, readback_fn)
+        dfu_read(alt_setting_test_file, readback_fn)
 
         u_boot_console.log.action('Comparing written and read data')
         written_hash = test_f.content_hash
@@ -266,7 +270,7 @@ def test_dfu(u_boot_console, env__usb_dev_port, env__dfu_config):
 
         u_boot_console.log.action(
             'Overwriting DFU primary altsetting with dummy data')
-        dfu_write(0, dummy_f.abs_fn)
+        dfu_write(alt_setting_test_file, dummy_f.abs_fn)
 
         for size in sizes:
             with u_boot_console.log.section('Data size %d' % size):
-- 
2.0.0.rc2

[U-Boot] [PATCH 2/3] tests: py: dfu: Add functionality to set different u-boot's dfu env variable

[Lukasz Majewski]

By default (on almost all systems) the dfu env variable, which defines
available alt settings, is named as "dfu_alt_info".

However on some platforms (i.e. Odroid XU3), the 'dfu_alt_info' is concatenated
from other variables - namely 'dfu_alt_boot' and 'dfu_alt_system' at run time
(when one types 'dfu 0 mmc 0' for first time).

'dfu_alt_boot' describes alt settings which depend on boot medium - for example
boot loader's LBA sectors which are different on eMMC and SD card because of e.g.
MBR/GPT.

'dfu_alt_system' describes board agnostic alt settings - like rootfs, kernel.
On such system we can only append/modify this env variable.

Because of the above, we must have way to modify other than "dfu_ale_info"
variable to perform tests.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
 test/py/tests/test_dfu.py | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py
index df3b561..1ed6020 100644
--- a/test/py/tests/test_dfu.py
+++ b/test/py/tests/test_dfu.py
@@ -44,6 +44,11 @@ env__dfu_configs = (
         # configurations, but don't want to test every single transfer size
         # on each, to avoid bloating the overall time taken by testing.
         "test_sizes": (63, 64, 65),
+        # Optional values.
+        # Those values are necessary on boards like Odroid XU3, where
+        # - dfu alt info env variable is concatenated from boot medium dependent
+        #   (dfu_alt_boot) and user defined (dfu_alt_system) variables
+        "alt_info_env_name": "dfu_alt_system",
     },
 )
 
@@ -124,7 +129,12 @@ 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')
 
-        cmd = 'setenv dfu_alt_info "%s"' % env__dfu_config['alt_info']
+        dfu_alt_info_env = "dfu_alt_info"
+        if "alt_info_env_name" in env__dfu_config:
+	        dfu_alt_info_env = env__dfu_config['alt_info_env_name']
+
+        cmd = 'setenv "%s" "%s"' % (dfu_alt_info_env,
+                                    env__dfu_config['alt_info'])
         u_boot_console.run_command(cmd)
 
         cmd = 'dfu 0 ' + env__dfu_config['cmd_params']
-- 
2.0.0.rc2

[U-Boot] [PATCH 2/3] tests: py: dfu: Add functionality to set different u-boot's dfu env variable

[Stephen Warren]

On 04/08/2016 09:44 AM, Lukasz Majewski wrote:
> By default (on almost all systems) the dfu env variable, which defines
> available alt settings, is named as "dfu_alt_info".
>
> However on some platforms (i.e. Odroid XU3), the 'dfu_alt_info' is concatenated
> from other variables - namely 'dfu_alt_boot' and 'dfu_alt_system' at run time
> (when one types 'dfu 0 mmc 0' for first time).
>
> 'dfu_alt_boot' describes alt settings which depend on boot medium - for example
> boot loader's LBA sectors which are different on eMMC and SD card because of e.g.
> MBR/GPT.
>
> 'dfu_alt_system' describes board agnostic alt settings - like rootfs, kernel.
> On such system we can only append/modify this env variable.
>
> Because of the above, we must have way to modify other than "dfu_ale_info"
> variable to perform tests.

Uggh. That's a pretty nasty user-experience since it prevents the user 
from controlling what happens. U-Boot should never over-ride what the 
user requests for alt_info, but rather simply provide help to the user 
allowing them to make use of the system-supplied information if they 
want. I.e. dfu_alt_boot should be set at boot time based on code. The 
user should then be free to define dfu_alt_info as they see fit; either 
hard-coded, or including the value of $dfu_alt_boot /if/ they want.

Anyway, I suppose since this feature exists, we have no choice but to 
support it.

> diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py
> index df3b561..1ed6020 100644
> --- a/test/py/tests/test_dfu.py
> +++ b/test/py/tests/test_dfu.py
> @@ -44,6 +44,11 @@ env__dfu_configs = (
>           # configurations, but don't want to test every single transfer size
>           # on each, to avoid bloating the overall time taken by testing.
>           "test_sizes": (63, 64, 65),
> +        # Optional values.
> +        # Those values are necessary on boards like Odroid XU3, where
> +        # - dfu alt info env variable is concatenated from boot medium dependent
> +        #   (dfu_alt_boot) and user defined (dfu_alt_system) variables
> +        "alt_info_env_name": "dfu_alt_system",

I think the "- " and leading space should be dropped; this isn't a list 
of items. I'd suggest re-phrasing this as:

# The name of the environment variable that the the dfu command reads
# alt info from. If unspecified, this defaults to dfu_alt_info, which is
# valid for most systems. Some systems use a different variable name.
# One example is the Odroid XU3,  which automatically generates
# $dfu_alt_info, each time the dfu command is run, by concatenating
# $dfu_alt_boot and $dfu_alt_system.

>       },
>   )
>
> @@ -124,7 +129,12 @@ 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')
>
> -        cmd = 'setenv dfu_alt_info "%s"' % env__dfu_config['alt_info']
> +        dfu_alt_info_env = "dfu_alt_info"
> +        if "alt_info_env_name" in env__dfu_config:
> +	        dfu_alt_info_env = env__dfu_config['alt_info_env_name']

That can be just:
dfu_alt_info_env = env__dfu_config.get('alt_info_env_name', \ 
'dfu_alt_info')

> +
> +        cmd = 'setenv "%s" "%s"' % (dfu_alt_info_env,
> +                                    env__dfu_config['alt_info'])
>           u_boot_console.run_command(cmd)
>
>           cmd = 'dfu 0 ' + env__dfu_config['cmd_params']

[U-Boot] [PATCH 2/3] tests: py: dfu: Add functionality to set different u-boot's dfu env variable

[Lukasz Majewski]

On Fri, 8 Apr 2016 10:28:06 -0600
Stephen Warren <swarren@wwwdotorg.org> wrote:

> On 04/08/2016 09:44 AM, Lukasz Majewski wrote:
> > By default (on almost all systems) the dfu env variable, which
> > defines available alt settings, is named as "dfu_alt_info".
> >
> > However on some platforms (i.e. Odroid XU3), the 'dfu_alt_info' is
> > concatenated from other variables - namely 'dfu_alt_boot' and
> > 'dfu_alt_system' at run time (when one types 'dfu 0 mmc 0' for
> > first time).
> >
> > 'dfu_alt_boot' describes alt settings which depend on boot medium -
> > for example boot loader's LBA sectors which are different on eMMC
> > and SD card because of e.g. MBR/GPT.
> >
> > 'dfu_alt_system' describes board agnostic alt settings - like
> > rootfs, kernel. On such system we can only append/modify this env
> > variable.
> >
> > Because of the above, we must have way to modify other than
> > "dfu_ale_info" variable to perform tests.
> 
> Uggh. That's a pretty nasty user-experience since it prevents the
> user from controlling what happens. U-Boot should never over-ride
> what the user requests for alt_info, but rather simply provide help
> to the user allowing them to make use of the system-supplied
> information if they want. I.e. dfu_alt_boot should be set at boot
> time based on code. The user should then be free to define
> dfu_alt_info as they see fit; either hard-coded, or including the
> value of $dfu_alt_boot /if/ they want.
> 
> Anyway, I suppose since this feature exists, we have no choice but to 
> support it.

This indeed could be done a bit differently. 

> 
> > diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py
> > index df3b561..1ed6020 100644
> > --- a/test/py/tests/test_dfu.py
> > +++ b/test/py/tests/test_dfu.py
> > @@ -44,6 +44,11 @@ env__dfu_configs = (
> >           # configurations, but don't want to test every single
> > transfer size # on each, to avoid bloating the overall time taken
> > by testing. "test_sizes": (63, 64, 65),
> > +        # Optional values.
> > +        # Those values are necessary on boards like Odroid XU3,
> > where
> > +        # - dfu alt info env variable is concatenated from boot
> > medium dependent
> > +        #   (dfu_alt_boot) and user defined (dfu_alt_system)
> > variables
> > +        "alt_info_env_name": "dfu_alt_system",
> 
> I think the "- " and leading space should be dropped; this isn't a
> list of items. I'd suggest re-phrasing this as:
> 
> # The name of the environment variable that the the dfu command reads
> # alt info from. If unspecified, this defaults to dfu_alt_info, which
> is # valid for most systems. Some systems use a different variable
> name. # One example is the Odroid XU3,  which automatically generates
> # $dfu_alt_info, each time the dfu command is run, by concatenating
> # $dfu_alt_boot and $dfu_alt_system.

+1

> 
> >       },
> >   )
> >
> > @@ -124,7 +129,12 @@ 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')
> >
> > -        cmd = 'setenv dfu_alt_info "%s"' %
> > env__dfu_config['alt_info']
> > +        dfu_alt_info_env = "dfu_alt_info"
> > +        if "alt_info_env_name" in env__dfu_config:
> > +	        dfu_alt_info_env =
> > env__dfu_config['alt_info_env_name']
> 
> That can be just:
> dfu_alt_info_env = env__dfu_config.get('alt_info_env_name', \ 
> 'dfu_alt_info')

This is the idiom which I was looking for :-). Thanks for hint.
I will rewrite it in such way.

> 
> > +
> > +        cmd = 'setenv "%s" "%s"' % (dfu_alt_info_env,
> > +                                    env__dfu_config['alt_info'])
> >           u_boot_console.run_command(cmd)
> >
> >           cmd = 'dfu 0 ' + env__dfu_config['cmd_params']
> 
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160408/6a88f32c/attachment.sig>

[U-Boot] [PATCH 2/3] tests: py: dfu: Add functionality to set different u-boot's dfu env variable

[Stephen Warren]

On 04/08/2016 09:44 AM, Lukasz Majewski wrote:
> By default (on almost all systems) the dfu env variable, which defines
> available alt settings, is named as "dfu_alt_info".
>
> However on some platforms (i.e. Odroid XU3), the 'dfu_alt_info' is concatenated
> from other variables - namely 'dfu_alt_boot' and 'dfu_alt_system' at run time
> (when one types 'dfu 0 mmc 0' for first time).
>
> 'dfu_alt_boot' describes alt settings which depend on boot medium - for example
> boot loader's LBA sectors which are different on eMMC and SD card because of e.g.
> MBR/GPT.
>
> 'dfu_alt_system' describes board agnostic alt settings - like rootfs, kernel.
> On such system we can only append/modify this env variable.
>
> Because of the above, we must have way to modify other than "dfu_ale_info"
> variable to perform tests.

> diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py

> +        # Optional values.
> +        # Those values are necessary on boards like Odroid XU3, where
> +        # - dfu alt info env variable is concatenated from boot medium dependent
> +        #   (dfu_alt_boot) and user defined (dfu_alt_system) variables
> +        "alt_info_env_name": "dfu_alt_system",

The previous value is also optional. I'd suggest simply prefixing each 
optional value with the same text as already used:

         # This value is optional.

[U-Boot] [PATCH 2/3] tests: py: dfu: Add functionality to set different u-boot's dfu env variable

[Lukasz Majewski]

On Fri, 8 Apr 2016 10:29:31 -0600
Stephen Warren <swarren@wwwdotorg.org> wrote:

> On 04/08/2016 09:44 AM, Lukasz Majewski wrote:
> > By default (on almost all systems) the dfu env variable, which
> > defines available alt settings, is named as "dfu_alt_info".
> >
> > However on some platforms (i.e. Odroid XU3), the 'dfu_alt_info' is
> > concatenated from other variables - namely 'dfu_alt_boot' and
> > 'dfu_alt_system' at run time (when one types 'dfu 0 mmc 0' for
> > first time).
> >
> > 'dfu_alt_boot' describes alt settings which depend on boot medium -
> > for example boot loader's LBA sectors which are different on eMMC
> > and SD card because of e.g. MBR/GPT.
> >
> > 'dfu_alt_system' describes board agnostic alt settings - like
> > rootfs, kernel. On such system we can only append/modify this env
> > variable.
> >
> > Because of the above, we must have way to modify other than
> > "dfu_ale_info" variable to perform tests.
> 
> > diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py
> 
> > +        # Optional values.
> > +        # Those values are necessary on boards like Odroid XU3,
> > where
> > +        # - dfu alt info env variable is concatenated from boot
> > medium dependent
> > +        #   (dfu_alt_boot) and user defined (dfu_alt_system)
> > variables
> > +        "alt_info_env_name": "dfu_alt_system",
> 
> The previous value is also optional. I'd suggest simply prefixing
> each optional value with the same text as already used:
> 
>          # This value is optional.
> 

Ok, no problem.

Best regards,
?ukasz Majewski
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20160408/c4d6f177/attachment.sig>

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

[Lukasz Majewski]

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.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
 test/py/tests/test_dfu.py | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py
index 1ed6020..af9073b 100644
--- a/test/py/tests/test_dfu.py
+++ b/test/py/tests/test_dfu.py
@@ -49,6 +49,10 @@ env__dfu_configs = (
         # - dfu alt info env variable is concatenated from boot medium dependent
         #   (dfu_alt_boot) and user defined (dfu_alt_system) variables
         "alt_info_env_name": "dfu_alt_system",
+        # - after concatenation dfu alt settings for test and dummy files are
+        #   moved from 0 and 1 to other values
+        "alt_num_test_file": "5",
+        "alt_num_dummy_file": "6",
     },
 )
 
@@ -119,6 +123,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
 
         fh = u_boot_utils.attempt_to_open_file(
             env__usb_dev_port['host_usb_dev_node'])
@@ -129,6 +135,12 @@ 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')
 
+        if "alt_num_test_file" in env__dfu_config:
+	        alt_setting_test_file = env__dfu_config['alt_num_test_file']
+
+        if "alt_num_dummy_file" in env__dfu_config:
+	        alt_setting_dummy_file = env__dfu_config['alt_num_dummy_file']
+
         dfu_alt_info_env = "dfu_alt_info"
         if "alt_info_env_name" in env__dfu_config:
 	        dfu_alt_info_env = env__dfu_config['alt_info_env_name']
-- 
2.0.0.rc2

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

[Stephen Warren]

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.

> +        "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.

or:

(b) Use names rather than numbers for the alt setting? 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.

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

[Lukasz Majewski]

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.

> 
> 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)

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?

> 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.

> 

-- 
Best regards,

Lukasz Majewski

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

[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;

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

[Lukasz Majewski]

Hi Stephen,

> 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.

I've double checked generation of dfu_alt_boot.

You can change its value at u-boot's prompt. 

ODROID-XU3 # setenv dfu_alt_boot "AABBCCDD"
ODROID-XU3 # printenv dfu_alt_boot
dfu_alt_boot=AABBCCDD

ODROID-XU3 # dfu 0 mmc 0 list
DFU alt info setting: done
DFU alt settings list:
dev: eMMC alt: 0 name: u-boot layout: RAW_ADDR
dev: eMMC alt: 1 name: bl1 layout: RAW_ADDR
dev: eMMC alt: 2 name: bl2 layout: RAW_ADDR
dev: eMMC alt: 3 name: tzsw layout: RAW_ADDR
dev: eMMC alt: 4 name: params.bin layout: RAW_ADDR


Unfortunately it is re-generated from CONFIG_DFU_ALT_BOOT_* each time I
run "dfu 0 mmc 0" command.

ODROID-XU3 printenv dfu_alt_boot
dfu_alt_boot=u-boot raw 0x3f 0x800;bl1 raw 0x1 0x1e;bl2 raw 0x1f
0x1d;tzsw raw 0x83f 0x200;params.bin raw 0x1880 0x20

> 
> >> 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)
> 
>      },
> )
> 

Yes, this is the solution I was trying to pursue :-)

> > 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.

Ok.

> 
> >> 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;
> 

Thanks for tip. However, I think that option described by you at point
1 is the way to go (I wouldn't need to modify u-boot and only extend
python code).

> 

Anyway thanks for support.

-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [PATCH v2 1/3] tests: py: dfu: Add global variables to store dfu alt numbers for test and dummy files

[Lukasz Majewski]

This patch replaces hardcoded (i.e. 0 and 1) values passed to dfu_{read|write}
with global variables.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
Changes for v2:
- None
---
 test/py/tests/test_dfu.py | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py
index 093e8d0..df3b561 100644
--- a/test/py/tests/test_dfu.py
+++ b/test/py/tests/test_dfu.py
@@ -80,6 +80,10 @@ test_sizes_default = (
 
 first_usb_dev_port = None
 
+# Default alt settings for test and dummy files
+alt_setting_test_file = 0
+alt_setting_dummy_file = 1
+
 @pytest.mark.buildconfigspec('cmd_dfu')
 def test_dfu(u_boot_console, env__usb_dev_port, env__dfu_config):
     """Test the "dfu" command; the host system must be able to enumerate a USB
@@ -229,15 +233,15 @@ def test_dfu(u_boot_console, env__usb_dev_port, env__dfu_config):
 
         u_boot_console.log.action('Writing test data to DFU primary ' +
             'altsetting')
-        dfu_write(0, test_f.abs_fn)
+        dfu_write(alt_setting_test_file, test_f.abs_fn)
 
         u_boot_console.log.action('Writing dummy data to DFU secondary ' +
             'altsetting to clear DFU buffers')
-        dfu_write(1, dummy_f.abs_fn)
+        dfu_write(alt_setting_dummy_file, dummy_f.abs_fn)
 
         u_boot_console.log.action('Reading DFU primary altsetting for ' +
             'comparison')
-        dfu_read(0, readback_fn)
+        dfu_read(alt_setting_test_file, readback_fn)
 
         u_boot_console.log.action('Comparing written and read data')
         written_hash = test_f.content_hash
@@ -266,7 +270,7 @@ def test_dfu(u_boot_console, env__usb_dev_port, env__dfu_config):
 
         u_boot_console.log.action(
             'Overwriting DFU primary altsetting with dummy data')
-        dfu_write(0, dummy_f.abs_fn)
+        dfu_write(alt_setting_test_file, dummy_f.abs_fn)
 
         for size in sizes:
             with u_boot_console.log.section('Data size %d' % size):
-- 
2.0.0.rc2

[U-Boot] [PATCH v2 2/3] tests: py: dfu: Add functionality to set different u-boot's dfu env variable

[Lukasz Majewski]

By default (on almost all systems) the dfu env variable, which defines
available alt settings, is named as "dfu_alt_info".

However on some platforms (i.e. Odroid XU3), the 'dfu_alt_info' is concatenated
from other variables - namely 'dfu_alt_boot' and 'dfu_alt_system' at run time
(when one types 'dfu 0 mmc 0' for first time).

'dfu_alt_boot' describes alt settings which depend on boot medium - for example
boot loader's LBA sectors which are different on eMMC and SD card because of e.g.
MBR/GPT.

'dfu_alt_system' describes board agnostic alt settings - like rootfs, kernel.
On such system we can only append/modify this env variable.

Because of the above, we must have way to modify other than "dfu_ale_info"
variable to perform tests.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>

---
Changes for v2:
- Rewrite of "alt_info_env_name" variable description
- Use of get() method on python's dictionary to easily obtain default
  value
---
 test/py/tests/test_dfu.py | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py
index df3b561..2ed38ad0 100644
--- a/test/py/tests/test_dfu.py
+++ b/test/py/tests/test_dfu.py
@@ -44,6 +44,14 @@ env__dfu_configs = (
         # configurations, but don't want to test every single transfer size
         # on each, to avoid bloating the overall time taken by testing.
         "test_sizes": (63, 64, 65),
+        # This value is optional.
+        # The name of the environment variable that the the dfu command reads
+        # alt info from. If unspecified, this defaults to dfu_alt_info, which is
+        # valid for most systems. Some systems use a different variable name.
+        # One example is the Odroid XU3,  which automatically generates
+        # $dfu_alt_info, each time the dfu command is run, by concatenating
+        # $dfu_alt_boot and $dfu_alt_system.
+        "alt_info_env_name": "dfu_alt_system",
     },
 )
 
@@ -124,7 +132,11 @@ 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')
 
-        cmd = 'setenv dfu_alt_info "%s"' % env__dfu_config['alt_info']
+        dfu_alt_info_env = env__dfu_config.get('alt_info_env_name', \
+	                                               'dfu_alt_info')
+
+        cmd = 'setenv "%s" "%s"' % (dfu_alt_info_env,
+                                    env__dfu_config['alt_info'])
         u_boot_console.run_command(cmd)
 
         cmd = 'dfu 0 ' + env__dfu_config['cmd_params']
-- 
2.0.0.rc2

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

[Lukasz Majewski]

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.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
Changes for v2:
- generate "alt_info" automatically
- use file names as alt settings instead of numerical values
- Extend in-code documentation
---
 test/py/tests/test_dfu.py | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py
index 2ed38ad0..f2830db 100644
--- a/test/py/tests/test_dfu.py
+++ b/test/py/tests/test_dfu.py
@@ -30,6 +30,13 @@ env__usb_dev_ports = (
     },
 )
 
+# Optional entries (required only when "alt_id_test_file" and
+# "alt_id_dummy_file" are specified).
+test_file_name = "/dfu_test.bin"
+dummy_file_name = "/dfu_dummy.bin"
+# Above files are used to generate proper "alt_info" entry
+"alt_info": "/%s ext4 0 2;/%s ext4 0 2" % (test_file_name, dummy_file_name),
+
 env__dfu_configs = (
     # eMMC, partition 1
     {
@@ -52,6 +59,16 @@ env__dfu_configs = (
         # $dfu_alt_info, each time the dfu command is run, by concatenating
         # $dfu_alt_boot and $dfu_alt_system.
         "alt_info_env_name": "dfu_alt_system",
+        # This value is optional.
+        # For boards which require the "test file" alt setting number other than
+        # default (0) it is possible to specify exact file name to be used as
+        # this parameter.
+        "alt_id_test_file": test_file_name,
+        # This value is optional.
+        # For boards which require the "dummy file" alt setting number other
+        # than default (1) it is possible to specify exact file name to be used
+        # as this parameter.
+        "alt_id_dummy_file": dummy_file_name,
     },
 )
 
@@ -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
 
         fh = u_boot_utils.attempt_to_open_file(
             env__usb_dev_port['host_usb_dev_node'])
@@ -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')
+
         dfu_alt_info_env = env__dfu_config.get('alt_info_env_name', \
 	                                               'dfu_alt_info')
 
-- 
2.0.0.rc2

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

[Stephen Warren]

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[].

[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

[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.

[U-Boot] [PATCH v2 1/3] tests: py: dfu: Add global variables to store dfu alt numbers for test and dummy files

[Stephen Warren]

On 04/19/2016 09:51 AM, Lukasz Majewski wrote:
> This patch replaces hardcoded (i.e. 0 and 1) values passed to dfu_{read|write}
> with global variables.

Why do these variables need to be global? They're only used inside 
test_dfu(). They should be local to that function.

[U-Boot] [PATCH v2 1/3] tests: py: dfu: Add global variables to store dfu alt numbers for test and dummy files

[Lukasz Majewski]

Hi Stephen,

> On 04/19/2016 09:51 AM, Lukasz Majewski wrote:
> > This patch replaces hardcoded (i.e. 0 and 1) values passed to
> > dfu_{read|write} with global variables.
> 
> Why do these variables need to be global? They're only used inside 
> test_dfu(). They should be local to that function.
> 

Ok, I will fix that.

-- 
Best regards,

Lukasz Majewski

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

[U-Boot] [PATCH v3 1/3] tests: py: dfu: Add variables to store dfu alt numbers for test and dummy files

[Lukasz Majewski]

This patch replaces hardcoded (i.e. 0 and 1) values passed to dfu_{read|write}
with variables.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
Changes for v3:
- Replace per module global variables with ones defined inside a function
Changes for v2:
- None
---
 test/py/tests/test_dfu.py | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py
index 093e8d0..2e6cd7b 100644
--- a/test/py/tests/test_dfu.py
+++ b/test/py/tests/test_dfu.py
@@ -99,6 +99,10 @@ def test_dfu(u_boot_console, env__usb_dev_port, env__dfu_config):
         Nothing.
     """
 
+    # Default alt settings for test and dummy files
+    alt_setting_test_file = 0
+    alt_setting_dummy_file = 1
+
     def start_dfu():
         """Start U-Boot's dfu shell command.
 
@@ -229,15 +233,15 @@ def test_dfu(u_boot_console, env__usb_dev_port, env__dfu_config):
 
         u_boot_console.log.action('Writing test data to DFU primary ' +
             'altsetting')
-        dfu_write(0, test_f.abs_fn)
+        dfu_write(alt_setting_test_file, test_f.abs_fn)
 
         u_boot_console.log.action('Writing dummy data to DFU secondary ' +
             'altsetting to clear DFU buffers')
-        dfu_write(1, dummy_f.abs_fn)
+        dfu_write(alt_setting_dummy_file, dummy_f.abs_fn)
 
         u_boot_console.log.action('Reading DFU primary altsetting for ' +
             'comparison')
-        dfu_read(0, readback_fn)
+        dfu_read(alt_setting_test_file, readback_fn)
 
         u_boot_console.log.action('Comparing written and read data')
         written_hash = test_f.content_hash
@@ -266,7 +270,7 @@ def test_dfu(u_boot_console, env__usb_dev_port, env__dfu_config):
 
         u_boot_console.log.action(
             'Overwriting DFU primary altsetting with dummy data')
-        dfu_write(0, dummy_f.abs_fn)
+        dfu_write(alt_setting_test_file, dummy_f.abs_fn)
 
         for size in sizes:
             with u_boot_console.log.section('Data size %d' % size):
-- 
2.0.0.rc2

[U-Boot] [PATCH v3 2/3] tests: py: dfu: Add functionality to set different u-boot's dfu env variable

[Lukasz Majewski]

By default (on almost all systems) the dfu env variable, which defines
available alt settings, is named as "dfu_alt_info".

However on some platforms (i.e. Odroid XU3), the 'dfu_alt_info' is concatenated
from other variables - namely 'dfu_alt_boot' and 'dfu_alt_system' at run time
(when one types 'dfu 0 mmc 0' for first time).

'dfu_alt_boot' describes alt settings which depend on boot medium - for example
boot loader's LBA sectors which are different on eMMC and SD card because of e.g.
MBR/GPT.

'dfu_alt_system' describes board agnostic alt settings - like rootfs, kernel.
On such system we can only append/modify this env variable.

Because of the above, we must have way to modify other than "dfu_ale_info"
variable to perform tests.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>

---
Changes for v3:
- None

Changes for v2:
- Rewrite of "alt_info_env_name" variable description
- Use of get() method on python's dictionary to easily obtain default
  value
---
 test/py/tests/test_dfu.py | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py
index 2e6cd7b..0442af8 100644
--- a/test/py/tests/test_dfu.py
+++ b/test/py/tests/test_dfu.py
@@ -44,6 +44,14 @@ env__dfu_configs = (
         # configurations, but don't want to test every single transfer size
         # on each, to avoid bloating the overall time taken by testing.
         "test_sizes": (63, 64, 65),
+        # This value is optional.
+        # The name of the environment variable that the the dfu command reads
+        # alt info from. If unspecified, this defaults to dfu_alt_info, which is
+        # valid for most systems. Some systems use a different variable name.
+        # One example is the Odroid XU3,  which automatically generates
+        # $dfu_alt_info, each time the dfu command is run, by concatenating
+        # $dfu_alt_boot and $dfu_alt_system.
+        "alt_info_env_name": "dfu_alt_system",
     },
 )
 
@@ -124,7 +132,11 @@ 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')
 
-        cmd = 'setenv dfu_alt_info "%s"' % env__dfu_config['alt_info']
+        dfu_alt_info_env = env__dfu_config.get('alt_info_env_name', \
+	                                               'dfu_alt_info')
+
+        cmd = 'setenv "%s" "%s"' % (dfu_alt_info_env,
+                                    env__dfu_config['alt_info'])
         u_boot_console.run_command(cmd)
 
         cmd = 'dfu 0 ' + env__dfu_config['cmd_params']
-- 
2.0.0.rc2

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

[Lukasz Majewski]

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.

Signed-off-by: Lukasz Majewski <l.majewski@samsung.com>
---
Changes for v3:
- replace variables declarations with ones read from configuration file
- remove not necessary str() conversion at DFU host command generation

Changes for v2:
- generate "alt_info" automatically
- use file names as alt settings instead of numerical values
- extend in-code documentation
---
 test/py/tests/test_dfu.py | 26 +++++++++++++++++++++-----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/test/py/tests/test_dfu.py b/test/py/tests/test_dfu.py
index 0442af8..8649d87 100644
--- a/test/py/tests/test_dfu.py
+++ b/test/py/tests/test_dfu.py
@@ -30,6 +30,13 @@ env__usb_dev_ports = (
     },
 )
 
+# Optional entries (required only when "alt_id_test_file" and
+# "alt_id_dummy_file" are specified).
+test_file_name = "/dfu_test.bin"
+dummy_file_name = "/dfu_dummy.bin"
+# Above files are used to generate proper "alt_info" entry
+"alt_info": "/%s ext4 0 2;/%s ext4 0 2" % (test_file_name, dummy_file_name),
+
 env__dfu_configs = (
     # eMMC, partition 1
     {
@@ -52,6 +59,16 @@ env__dfu_configs = (
         # $dfu_alt_info, each time the dfu command is run, by concatenating
         # $dfu_alt_boot and $dfu_alt_system.
         "alt_info_env_name": "dfu_alt_system",
+        # This value is optional.
+        # For boards which require the "test file" alt setting number other than
+        # default (0) it is possible to specify exact file name to be used as
+        # this parameter.
+        "alt_id_test_file": test_file_name,
+        # This value is optional.
+        # For boards which require the "dummy file" alt setting number other
+        # than default (1) it is possible to specify exact file name to be used
+        # as this parameter.
+        "alt_id_dummy_file": dummy_file_name,
     },
 )
 
@@ -107,10 +124,6 @@ def test_dfu(u_boot_console, env__usb_dev_port, env__dfu_config):
         Nothing.
     """
 
-    # Default alt settings for test and dummy files
-    alt_setting_test_file = 0
-    alt_setting_dummy_file = 1
-
     def start_dfu():
         """Start U-Boot's dfu shell command.
 
@@ -188,7 +201,7 @@ def test_dfu(u_boot_console, env__usb_dev_port, env__dfu_config):
             Nothing.
         """
 
-        cmd = ['dfu-util', '-a', str(alt_setting), up_dn_load_arg, fn]
+        cmd = ['dfu-util', '-a', alt_setting, up_dn_load_arg, fn]
         if 'host_usb_port_path' in env__usb_dev_port:
             cmd += ['-p', env__usb_dev_port['host_usb_port_path']]
         u_boot_utils.run_and_log(u_boot_console, cmd)
@@ -276,6 +289,9 @@ def test_dfu(u_boot_console, env__usb_dev_port, env__dfu_config):
     dummy_f = u_boot_utils.PersistentRandomFile(u_boot_console,
         'dfu_dummy.bin', 1024)
 
+    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')
+
     ignore_cleanup_errors = True
     try:
         start_dfu()
-- 
2.0.0.rc2

[U-Boot] [PATCH v3 1/3] tests: py: dfu: Add variables to store dfu alt numbers for test and dummy files

[Stephen Warren]

On 04/21/2016 09:40 AM, Lukasz Majewski wrote:
> This patch replaces hardcoded (i.e. 0 and 1) values passed to dfu_{read|write}
> with variables.

The series,
Acked-by: Stephen Warren <swarren@nvidia.com>