From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Warren Date: Mon, 11 Apr 2016 11:05:25 -0600 Subject: [U-Boot] [PATCH 3/3] tests: py: dfu: Provide functionality to set test and dummy files alt settings In-Reply-To: <20160411104232.775b8d03@amdc2363> References: <1460130291-24223-1-git-send-email-l.majewski@samsung.com> <1460130291-24223-3-git-send-email-l.majewski@samsung.com> <5707DD96.80103@wwwdotorg.org> <20160411104232.775b8d03@amdc2363> Message-ID: <570BD955.2040601@wwwdotorg.org> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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;