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