[U-Boot] [PATCH] py: test_gpio: Add support for GPIO pytest
From: Vipul Kumar vipul.kumar@xilinx.com
This patch tests the gpio commands using the gpio data from boardenv file.
Also one test will show the default status of all the gpio's supported by the processor.
Signed-off-by: Vipul Kumar vipulk@xilinx.com Signed-off-by: Michal Simek michal.simek@xilinx.com ---
test/py/tests/test_gpio.py | 111 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 test/py/tests/test_gpio.py
diff --git a/test/py/tests/test_gpio.py b/test/py/tests/test_gpio.py new file mode 100644 index 000000000000..5ab237c8abe5 --- /dev/null +++ b/test/py/tests/test_gpio.py @@ -0,0 +1,111 @@ +# Copyright (c) 2017 Xilinx, Inc. +# +# SPDX-License-Identifier: GPL-2.0 + +# Test various gpio-related functionality, such as the input, set, +# clear and toggle. + +import pytest +import random + +""" +Note: This test relies on boardenv_* containing configuration values to define +which the gpio available for testing. Without this, this test will be automat- +ically skipped. + +For example: + +# A list of gpio's that are going to be tested in order to validate the +# test. +env__gpio_val = { + "gpio": [0,1,2], +} + +# A list of gpio's that are shorted on the hardware board and first gpio of +# each group is configured as input and the other as output. If the pins are +# not shorted properly, then the test will be fail. +env__gpio_input_output = { + "list_of_gpios": [ [36, 37], [38, 39]], +} +""" + +def gpio_input(u_boot_console, gpio): + u_boot_console.run_command("gpio input %d" %gpio) + response = u_boot_console.run_command("gpio status -a %d" %gpio) + expected_response = "%d: input:" %gpio + assert(expected_response in response) + +def gpio_set(u_boot_console, gpio): + expected_response = "%d: output: 1" %gpio + u_boot_console.run_command("gpio set %d" %gpio) + response = u_boot_console.run_command("gpio status -a %d" %gpio) + assert(expected_response in response) + +def gpio_clear(u_boot_console, gpio): + expected_response = "%d: output: 0" %gpio + u_boot_console.run_command("gpio clear %d" %gpio) + response = u_boot_console.run_command("gpio status -a %d" %gpio) + assert(expected_response in response) + +def gpio_toggle(u_boot_console, gpio): + expected_response = "%d: output: 1" %gpio + u_boot_console.run_command("gpio toggle %d" %gpio) + response = u_boot_console.run_command("gpio status -a %d" %gpio) + assert(expected_response in response) + +@pytest.mark.buildconfigspec("cmd_gpio") +def test_gpio_status(u_boot_console): + response = u_boot_console.run_command("gpio status -a") + expected_response = "0: input:" + assert(expected_response in response) + +@pytest.mark.buildconfigspec("cmd_gpio") +def test_gpio(u_boot_console): + f = u_boot_console.config.env.get('env__gpio_val', None) + if not f: + pytest.skip('No GPIO readable file to read') + + gpin = f.get("gpio", None) + + for gpio in gpin: + gpio_input(u_boot_console, gpio) + gpio_set(u_boot_console, gpio) + gpio_clear(u_boot_console, gpio) + gpio_toggle(u_boot_console, gpio) + +@pytest.mark.buildconfigspec("cmd_gpio") +def test_gpio_input_output(u_boot_console): + f = u_boot_console.config.env.get('env__gpio_input_output', None) + if not f: + pytest.skip('No GPIO readable file to read') + + list_of_gpios = f.get("list_of_gpios", None) + + flag = 0 + for list in list_of_gpios: + for i in list: + if flag == 0: + gpio_in = i + expected_response = "%d: input:" %gpio_in + u_boot_console.run_command("gpio input %d" %gpio_in) + response = u_boot_console.run_command("gpio status -a %d" %gpio_in) + assert(expected_response in response) + flag = 1 + + else: + gpio_out = i + expected_response = "%d: output:" %gpio_out + u_boot_console.run_command("gpio set %d" %gpio_out) + response = u_boot_console.run_command("gpio status -a %d" %gpio_out) + assert(expected_response in response) + flag = 0 + + expected_response = "%d: input: 0" %gpio_in + u_boot_console.run_command("gpio clear %d" %gpio_out) + response = u_boot_console.run_command("gpio status -a %d" %gpio_in) + assert(expected_response in response) + + expected_response = "%d: input: 1" %gpio_in + u_boot_console.run_command("gpio set %d" %gpio_out) + response = u_boot_console.run_command("gpio status -a %d" %gpio_in) + assert(expected_response in response)
On 09/20/2017 01:55 AM, Michal Simek wrote:
From: Vipul Kumar vipul.kumar@xilinx.com
This patch tests the gpio commands using the gpio data from boardenv file.
Also one test will show the default status of all the gpio's supported by the processor.
Nit: Wrap the commit description to something like 74 characters; the text above is far too narrow.
Nit: GPIO not gpio in any free-form text (here and in comments in the source file)
Ah, I'd briefly though about GPIO tests before, but was wondering how to integrate e.g. a USB GPIO device to the host PC and interface that with the target. Loopback GPIOs make this a lot easier:-)
diff --git a/test/py/tests/test_gpio.py b/test/py/tests/test_gpio.py
+""" +Note: This test relies on boardenv_* containing configuration values to define +which the gpio available for testing. Without this, this test will be automat-
Nit: Double space before automatically.
+For example:
+# A list of gpio's that are going to be tested in order to validate the +# test. +env__gpio_val = {
"gpio": [0,1,2],+}
Why is this a dictionary with only one key? Better to just assign the array directly to env__gpio_val, or put both the "gpio" and "list_of_gpios" into a single dictionary.
+# A list of gpio's that are shorted on the hardware board and first gpio of +# each group is configured as input and the other as output. If the pins are +# not shorted properly, then the test will be fail. +env__gpio_input_output = {
"list_of_gpios": [ [36, 37], [38, 39]],+}
Let's make the examples for env__gpio_val and env__gpio_input_output use the same GPIO numbers so that they match, and it's obvious how the two relate to each-other. I assume they're meant to?
Actually, why do we even need env__gpio_val if env__gpio_input_output already lists all the pairs of GPIOs?
Again, let's collapse this dict/list pair into just a list without the dict wrapper.
"gpio_val", "gpio", "gpio_input_output", and "list_of_gpios" aren't exactly great names. At least for "gpio_input_output"/"list_of_gpios", I'd suggest a more semantic name is "env__looped_back_gpios", which is a plain list (of pairs of GPIO numbers).
+def gpio_input(u_boot_console, gpio):
- u_boot_console.run_command("gpio input %d" %gpio)
Nit: Space after % operator (the second % outside the string). Same comment in many other cases.
- response = u_boot_console.run_command("gpio status -a %d" %gpio)
- expected_response = "%d: input:" %gpio
- assert(expected_response in response)
+def gpio_set(u_boot_console, gpio):
- expected_response = "%d: output: 1" %gpio
- u_boot_console.run_command("gpio set %d" %gpio)
- response = u_boot_console.run_command("gpio status -a %d" %gpio)
- assert(expected_response in response)
Why not make all these functions do things in the same order; gpio_input() above sets expected_response immediately before the assert (which I think is the right place to do it since it relates to the assert and not the gpio set/clear/toggle command which is executed earlier), whereas the other functions set expected_response at the start of the function.
+@pytest.mark.buildconfigspec("cmd_gpio") +def test_gpio_status(u_boot_console):
- response = u_boot_console.run_command("gpio status -a")
- expected_response = "0: input:"
- assert(expected_response in response)
This test assumes that GPIO 0 is expected to be configured as an input by default (at boot) on all HW that supports GPIOs. I don't think this is a valid assumption.
+@pytest.mark.buildconfigspec("cmd_gpio") +def test_gpio(u_boot_console):
- f = u_boot_console.config.env.get('env__gpio_val', None)
- if not f:
pytest.skip('No GPIO readable file to read')
What "file" is this referring to? A better message might be 'env__gpio_val not defined for this board'.
- gpin = f.get("gpio", None)
- for gpio in gpin:
gpio_input(u_boot_console, gpio)gpio_set(u_boot_console, gpio)gpio_clear(u_boot_console, gpio)gpio_toggle(u_boot_console, gpio)
So this is simply to test executing those commands? Why not rely on testing them as a side-effect of the loopback test below? the only one untested is toggle, and that could be added to the loopback test.
+@pytest.mark.buildconfigspec("cmd_gpio") +def test_gpio_input_output(u_boot_console):
- f = u_boot_console.config.env.get('env__gpio_input_output', None)
- if not f:
pytest.skip('No GPIO readable file to read')- list_of_gpios = f.get("list_of_gpios", None)
- flag = 0
- for list in list_of_gpios:
for i in list:if flag == 0:gpio_in = iexpected_response = "%d: input:" %gpio_inu_boot_console.run_command("gpio input %d" %gpio_in)response = u_boot_console.run_command("gpio status -a %d" %gpio_in)assert(expected_response in response)flag = 1else:gpio_out = iexpected_response = "%d: output:" %gpio_outu_boot_console.run_command("gpio set %d" %gpio_out)response = u_boot_console.run_command("gpio status -a %d" %gpio_out)assert(expected_response in response)flag = 0
This (the inner "for i in list") doesn't need to be a loop, since it's using flag to do completely different things on each iteration anyway, and that logic is pretty confusing. Instead, just do:
for list in list_of_gpios: gpio_in = list[0] configure input GPIO gpio_out = list[1] set gpio_out to 0 validate gpio_in value is 0 set gpio_out to 1 validate gpio_in value is 1
(and "list" isn't a great variable name especially since it aliases with a Python type constructor function; gpio_pair would be better)
... and actually it might be better to do something like the following:
for every GPIO pair: set gpio_in to input for every GPIO pair: set gpio_out to 0 for every GPIO pair: set gpio_out to 1 for every GPIO pair: if inner gpio_in == output gpio_in: expected_val = 1 else: expected_val = 0 validate inner gpio_in is expected_val set gpio_out to 0
... since that will both test that each gpio_out value affects the associated gpio_in value, but also that no gpio_out value affects any unexpected/unassociated gpio_in value.
Hi Stephen,
On 20.9.2017 19:05, Stephen Warren wrote:
On 09/20/2017 01:55 AM, Michal Simek wrote:
From: Vipul Kumar vipul.kumar@xilinx.com
This patch tests the gpio commands using the gpio data from boardenv file.
Also one test will show the default status of all the gpio's supported by the processor.
Nit: Wrap the commit description to something like 74 characters; the text above is far too narrow.
Nit: GPIO not gpio in any free-form text (here and in comments in the source file)
Ah, I'd briefly though about GPIO tests before, but was wondering how to integrate e.g. a USB GPIO device to the host PC and interface that with the target. Loopback GPIOs make this a lot easier:-)
good.
diff --git a/test/py/tests/test_gpio.py b/test/py/tests/test_gpio.py
+""" +Note: This test relies on boardenv_* containing configuration values to define +which the gpio available for testing. Without this, this test will be automat-
Nit: Double space before automatically.
+For example:
+# A list of gpio's that are going to be tested in order to validate the +# test. +env__gpio_val = { + "gpio": [0,1,2], +}
Why is this a dictionary with only one key? Better to just assign the array directly to env__gpio_val, or put both the "gpio" and "list_of_gpios" into a single dictionary.
+# A list of gpio's that are shorted on the hardware board and first gpio of +# each group is configured as input and the other as output. If the pins are +# not shorted properly, then the test will be fail. +env__gpio_input_output = { + "list_of_gpios": [ [36, 37], [38, 39]], +}
Let's make the examples for env__gpio_val and env__gpio_input_output use the same GPIO numbers so that they match, and it's obvious how the two relate to each-other. I assume they're meant to?
The reason for two values is that you do that loopback between certain pins. It means you have to list pairs. I have tested this on the board where I can choose whatever configuration by external wires. Also this list is not the same with the above one because you are choosing which one is loopback but not all can be done as loopback.
Also I have tested that you can specify 36,37 as one pair and then 37,36 pair which simply saying which pin is used as output and which as input.
I don't think it is right to say that both of them can bidirectional because at least in our chip with have some pins which are just output only and input only.
Actually, why do we even need env__gpio_val if env__gpio_input_output already lists all the pairs of GPIOs?
Again, let's collapse this dict/list pair into just a list without the dict wrapper.
"gpio_val", "gpio", "gpio_input_output", and "list_of_gpios" aren't exactly great names. At least for "gpio_input_output"/"list_of_gpios", I'd suggest a more semantic name is "env__looped_back_gpios", which is a plain list (of pairs of GPIO numbers).
Vipul: Please look at these comments.
+def gpio_input(u_boot_console, gpio): + u_boot_console.run_command("gpio input %d" %gpio)
Nit: Space after % operator (the second % outside the string). Same comment in many other cases.
+ response = u_boot_console.run_command("gpio status -a %d" %gpio) + expected_response = "%d: input:" %gpio + assert(expected_response in response)
+def gpio_set(u_boot_console, gpio): + expected_response = "%d: output: 1" %gpio + u_boot_console.run_command("gpio set %d" %gpio) + response = u_boot_console.run_command("gpio status -a %d" %gpio) + assert(expected_response in response)
Why not make all these functions do things in the same order; gpio_input() above sets expected_response immediately before the assert (which I think is the right place to do it since it relates to the assert and not the gpio set/clear/toggle command which is executed earlier), whereas the other functions set expected_response at the start of the function.
+@pytest.mark.buildconfigspec("cmd_gpio") +def test_gpio_status(u_boot_console): + response = u_boot_console.run_command("gpio status -a") + expected_response = "0: input:" + assert(expected_response in response)
This test assumes that GPIO 0 is expected to be configured as an input by default (at boot) on all HW that supports GPIOs. I don't think this is a valid assumption.
ok. I think this can be removed - the key point of this test was to list all of them to see the status. It means return to shell should be fine.
Vipul: Please comment the rest of Stephen's comments and prepare v2.
Thanks, Michal
On 09/21/2017 06:24 AM, Michal Simek wrote:
Hi Stephen,
On 20.9.2017 19:05, Stephen Warren wrote:
On 09/20/2017 01:55 AM, Michal Simek wrote:
From: Vipul Kumar vipul.kumar@xilinx.com
This patch tests the gpio commands using the gpio data from boardenv file.
Also one test will show the default status of all the gpio's supported by the processor.
Nit: Wrap the commit description to something like 74 characters; the text above is far too narrow.
Nit: GPIO not gpio in any free-form text (here and in comments in the source file)
Ah, I'd briefly though about GPIO tests before, but was wondering how to integrate e.g. a USB GPIO device to the host PC and interface that with the target. Loopback GPIOs make this a lot easier:-)
good.
diff --git a/test/py/tests/test_gpio.py b/test/py/tests/test_gpio.py
+""" +Note: This test relies on boardenv_* containing configuration values to define +which the gpio available for testing. Without this, this test will be automat-
Nit: Double space before automatically.
+For example:
+# A list of gpio's that are going to be tested in order to validate the +# test. +env__gpio_val = {
"gpio": [0,1,2],+}
Why is this a dictionary with only one key? Better to just assign the array directly to env__gpio_val, or put both the "gpio" and "list_of_gpios" into a single dictionary.
+# A list of gpio's that are shorted on the hardware board and first gpio of +# each group is configured as input and the other as output. If the pins are +# not shorted properly, then the test will be fail. +env__gpio_input_output = {
"list_of_gpios": [ [36, 37], [38, 39]],+}
Let's make the examples for env__gpio_val and env__gpio_input_output use the same GPIO numbers so that they match, and it's obvious how the two relate to each-other. I assume they're meant to?
The reason for two values is that you do that loopback between certain pins. It means you have to list pairs. I have tested this on the board where I can choose whatever configuration by external wires. Also this list is not the same with the above one because you are choosing which one is loopback but not all can be done as loopback.
Also I have tested that you can specify 36,37 as one pair and then 37,36 pair which simply saying which pin is used as output and which as input.
I don't think it is right to say that both of them can bidirectional because at least in our chip with have some pins which are just output only and input only.
Sorry, I still don't understand why there are two lists. Certainly I see that there's different testing applied to each list, but I don't see any benefit from doing that. If you test just loopback pairs, then you've tested all the other APIs as part of that, so you don't need a separate test for a separate list of GPIOs.
Also, if the code was to include logic to test pairs of loopback GPIOs in both directions, the code would need to be careful to set both GPIOs to input after each test step. Without that, when setting up the output GPIO for the next round of testing, both GPIOs might be outputs for a while which could damage HW. I don't recall whether the current code is safe in this case or not.
On 21.9.2017 17:35, Stephen Warren wrote:
On 09/21/2017 06:24 AM, Michal Simek wrote:
Hi Stephen,
On 20.9.2017 19:05, Stephen Warren wrote:
On 09/20/2017 01:55 AM, Michal Simek wrote:
From: Vipul Kumar vipul.kumar@xilinx.com
This patch tests the gpio commands using the gpio data from boardenv file.
Also one test will show the default status of all the gpio's supported by the processor.
Nit: Wrap the commit description to something like 74 characters; the text above is far too narrow.
Nit: GPIO not gpio in any free-form text (here and in comments in the source file)
Ah, I'd briefly though about GPIO tests before, but was wondering how to integrate e.g. a USB GPIO device to the host PC and interface that with the target. Loopback GPIOs make this a lot easier:-)
good.
diff --git a/test/py/tests/test_gpio.py b/test/py/tests/test_gpio.py
+""" +Note: This test relies on boardenv_* containing configuration values to define +which the gpio available for testing. Without this, this test will be automat-
Nit: Double space before automatically.
+For example:
+# A list of gpio's that are going to be tested in order to validate the +# test. +env__gpio_val = { + "gpio": [0,1,2], +}
Why is this a dictionary with only one key? Better to just assign the array directly to env__gpio_val, or put both the "gpio" and "list_of_gpios" into a single dictionary.
+# A list of gpio's that are shorted on the hardware board and first gpio of +# each group is configured as input and the other as output. If the pins are +# not shorted properly, then the test will be fail. +env__gpio_input_output = { + "list_of_gpios": [ [36, 37], [38, 39]], +}
Let's make the examples for env__gpio_val and env__gpio_input_output use the same GPIO numbers so that they match, and it's obvious how the two relate to each-other. I assume they're meant to?
The reason for two values is that you do that loopback between certain pins. It means you have to list pairs. I have tested this on the board where I can choose whatever configuration by external wires. Also this list is not the same with the above one because you are choosing which one is loopback but not all can be done as loopback.
Also I have tested that you can specify 36,37 as one pair and then 37,36 pair which simply saying which pin is used as output and which as input.
I don't think it is right to say that both of them can bidirectional because at least in our chip with have some pins which are just output only and input only.
Sorry, I still don't understand why there are two lists. Certainly I see that there's different testing applied to each list, but I don't see any benefit from doing that. If you test just loopback pairs, then you've tested all the other APIs as part of that, so you don't need a separate test for a separate list of GPIOs.
Right but that's up to user what will be setup. It means for example having this configuration in boardenv is just increasing testing time and don't bringing up any value (in current style).
env__gpio_val = { "gpio": [36,37], }
env__gpio_input_output = { "list_of_gpios": [ [36, 37], [37, 36]] }
But if these two lists have different values then it makes sense to me.
#These two can be gpio leds env__gpio_val = { "gpio": [0,1], }
#And this is loop out 37 -> in 36 and out 36 -> in 37; env__gpio_input_output = { "list_of_gpios": [ [36, 37], [37, 36]] }
Also, if the code was to include logic to test pairs of loopback GPIOs in both directions, the code would need to be careful to set both GPIOs to input after each test step. Without that, when setting up the output GPIO for the next round of testing, both GPIOs might be outputs for a while which could damage HW. I don't recall whether the current code is safe in this case or not.
For loopback testing procedure should be: Setup a pin to input Setup another pin to output and do that testing.
If this is done at the beginning of the test it should be safe.
Thanks, Michal
participants (2)
-
Michal Simek -
Stephen Warren