| Message ID | 1461081084-16649-3-git-send-email-l.majewski@samsung.com |
|---|---|
| State | Superseded |
| Delegated to: | Łukasz Majewski |
| Headers | show |
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[].
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?
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.
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')
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(+)