[TOPIC-GPIO] convert 2-pin test to new API #19249

pabigot · 2019-09-18T14:11:23Z

Test that the new port API functions all behave as expected, including physical vs logical level for input and output as well as masked and set-based output operations. Also tests the new pin API functions.

This includes a devicetree binding that can be used to provide target-specific resources for target-specific tests.

zephyrbot · 2019-09-18T14:44:12Z

All checks are passing now.

checkpatch (informational only, not a failure)

-:210: WARNING:LONG_LINE: line over 80 characters
#210: FILE: tests/drivers/gpio/gpio_basic_api/src/test_callback_manage.c:49:
+		rc = gpio_pin_interrupt_configure(dev, PIN_OUT, GPIO_INT_DISABLE);

-:429: WARNING:LONG_LINE: line over 80 characters
#429: FILE: tests/drivers/gpio/gpio_basic_api/src/test_callback_trigger.c:93:
+	rc = gpio_pin_interrupt_configure(dev, PIN_IN, mode | GPIO_INT_DEBOUNCE);

-:594: WARNING:EMBEDDED_FUNCTION_NAME: Prefer using '"%s...", __func__' to using 'raw_in', this function's name, in a string
#594: FILE: tests/drivers/gpio/gpio_basic_api/src/test_gpio_port.c:28:
+		      "raw_in failed");

-:620: WARNING:EMBEDDED_FUNCTION_NAME: Prefer using '"%s...", __func__' to using 'raw_out', this function's name, in a string
#620: FILE: tests/drivers/gpio/gpio_basic_api/src/test_gpio_port.c:54:
+		      "raw_out failed");

-:746: WARNING:LONG_LINE: line over 80 characters
#746: FILE: tests/drivers/gpio/gpio_basic_api/src/test_gpio_port.c:180:
+		rc = gpio_port_set_clr_bits_raw(dev, BIT(PIN_OUT), BIT(PIN_OUT));

-:754: WARNING:LONG_LINE: line over 80 characters
#754: FILE: tests/drivers/gpio/gpio_basic_api/src/test_gpio_port.c:188:
+		rc = gpio_port_set_clr_bits_raw(dev, BIT(PIN_OUT), BIT(PIN_OUT));

-:954: WARNING:LONG_LINE: line over 80 characters
#954: FILE: tests/drivers/gpio/gpio_basic_api/src/test_gpio_port.c:388:
+	rc = gpio_pin_configure(dev, PIN_IN, GPIO_INPUT | GPIO_ACTIVE_LOW | GPIO_PULL_UP);

-:967: WARNING:LONG_LINE: line over 80 characters
#967: FILE: tests/drivers/gpio/gpio_basic_api/src/test_gpio_port.c:401:
+	rc = gpio_pin_configure(dev, PIN_IN, GPIO_INPUT | GPIO_ACTIVE_LOW | GPIO_PULL_DOWN);

- total: 0 errors, 8 warnings, 1054 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

galak

Not specific to your changes, but do we need a readme or something that describes how to setup this test from a hw point of view. I'm assuming one needs to wire one GPIO to another or something for this all to work.

But not seeing anything obvious about what that setup might be

pabigot · 2019-09-20T14:40:32Z

Documentation on the test requirements would be helpful; right now it's in test_gpio.h at the point where the pins are identified. When there's some evidence the test,resources devicetree binding has some support, and whether it should be the default, I'll see what can be done.

galak · 2019-09-20T14:46:14Z

Documentation on the test requirements would be helpful; right now it's in test_gpio.h at the point where the pins are identified. When there's some evidence the test,resources devicetree binding has some support, and whether it should be the default, I'll see what can be done.

With your test addition is it just one pin as output and one as input?

pabigot · 2019-09-20T14:52:34Z

With your test addition is it just one pin as output and one as input?

It's two pins on the same controller wired together, like it says in the header. No change from the original. One is nominally output, one is input, but there's no specific promise that the pins aren't configured in other ways.

There's a performance test addition not in this PR that uses additional pins, but it doesn't require any specific wiring.

pabigot · 2019-09-22T22:27:46Z

@galak @mnkp I've put back the tests for the interrupts. I've only tested this on nrf52840_pca10056 at the moment, though it had previously been tested on all converted platforms.

This probably breaks shippable, since the test will work only on drivers that have been updated. I believe we've agreed to accept that on the topic branch. If not, I don't see how we can test anything.

galak · 2019-09-23T10:33:49Z

dts/bindings/misc/test,resources.yaml

+
+properties:
+    gpios:
+        type: compound


type: phandle-array

galak · 2019-09-23T10:40:46Z

tests/drivers/gpio/gpio_basic_api/frdm_k64f.overlay

+	resources {
+		compatible = "test,resources";
+		gpios =
+			<&gpioc 16 0>, <&gpioc 17 0>,


Just curious how this set or why its this set of GPIOs?

I needed two on the same port, hence C16 and C17 (also those are Arduino D0 and D1, which made some sense).

For the performance test that isn't in this PR I needed more on the same port, so added the others. They can be taken out.

galak

So I wondering if we can end up having a general test overlay per board.

Also wondering about having a bindings searched for in the project dir.

galak · 2019-09-23T14:14:54Z

@galak @mnkp I've put back the tests for the interrupts. I've only tested this on nrf52840_pca10056 at the moment, though it had previously been tested on all converted platforms.

This probably breaks shippable, since the test will work only on drivers that have been updated. I believe we've agreed to accept that on the topic branch. If not, I don't see how we can test anything.

Why would it break shippable? seems to fully pass from a build point of view.

pabigot · 2019-09-23T14:29:55Z

So I wondering if we can end up having a general test overlay per board.

I don't think so, and suggest some difference in test requirements in the commit message.

Also wondering about having a bindings searched for in the project dir.

I don't know what you mean by this. Ideally the overlays would be in the dts directory, but that's not where the cmake boilerplate looks. I've tried several approaches, and am happy with none of the ones that work.

Why would it break shippable? seems to fully pass from a build point of view.

Yes, it does build. I think at one point there was a change that did break builds; possibly pabigot@6c9de37 which is a little out of date, and might have caused problems in its original context but maybe not now. It'll need to go in sometime. I figure to do it separately.

pabigot · 2019-09-23T14:47:02Z

@galak I've corrected the binding, updated the rationale for pins k64f pins, and added an overlay for same70.

Tried to get same70 to work but it like k64f appears to require pinmux, but unlike k64f there aren't enough same70 examples to figure out how to configure the pins.

Looks like double-edge passes on Nordic, fails on EFR32 and MCUX.

The test works by toggling the output state within the callback, which should immediately flag a second state change. It may be that the way the interrupt is handled prevents the state change from being noticed. IMO this should be expected to work as intended so I'm going to assume this is a driver problem until somebody shows why it isn't.

mnkp · 2019-09-23T21:11:55Z

dts/bindings/misc/test,resources.yaml

+
+properties:
+    gpios:
+        type: phandle-array


DTS was created as a way to describe the hardware and its initial configuration during the boot process. So far we've kept the promise. gpios property is always attached to a hardware module. Even "gpio-leds" and "gpio-keys" bindings that we use to instantiate LEDs and buttons refer to real hardware. In this case we break that contract since "test,resources" is not a piece of hardware.

I'm personally not against introducing such a "virtual" component. It does solve neatly a class of problems. However, if we do so I would rather have a solution that's more general. IMHO the #18544 does a much better job at this.

Another way of handling the issue, which I believe would be more in line with the spirit of DTS, would be to add gpios property directly to the soc node. Since that's what we actually want to do, we need to name some pins that belong to SoC and attach them to the respective gpio-controller.

The third option, that I see, would be to stick with what we've been doing so far: no virtual components, no gpios property attached to the soc node, instead nodes that describe a real piece of hardware. In terms of hardware resources that we need to run the tests we should have a way to describe a short and probably also a pin that is not connected.

pabigot · 2019-09-24T12:26:46Z

Taking into account @mnkp's comments I've reworked this to use a test-specific binding that identifies the hardware required to run this application. I've also added a cmake patch so we can get the overlays out of the test root.

Hopefully this will be an acceptable path forward that won't involve major devicetree-related design discussions. I'll submit the cmake patch to master separately.

galak · 2019-09-24T12:57:41Z

tests/drivers/gpio/gpio_basic_api/dts/bindings/test,gpio_basic_api.yaml

+compatible: "test,gpio_basic_api"
+
+properties:
+    gpios:


would it make sense for this to be:

resources { compatible = "test,gpio_basic_api"; /* Arduino D0, D1 */ in-gpios = <&gpiof 6 0>; out-gpios = <&gpiof 5 0>; };

Yes. Updated PR, and added check for incompatible controllers.

mnkp

This PR proposes to keep overlay files in a per testcase directory. This makes tests completely unmaintainable and is a terrible idea. The long term goal is to have part of the test suite running on the real hardware. This means we should have some dedicated, custom test boards each of which can run maximum amount of the testcases. As such we should aim at reusing special resources and custom configurations employed on the board. GPIO pins which are physically shorted should be made available to all the testcases which may make use of it: e.g. remaining GPIO, as well as I2S or SPI testcases. Having a dozens of dedicated hardware boards crafted for a single testcase just doesn't go with the automated test suite.

galak · 2019-09-25T14:38:11Z

Would be nice if we could add the core changes to the test and resolve the DT stuff later.

pabigot · 2019-09-25T14:51:55Z

Would be nice if we could add the core changes to the test and resolve the DT stuff later.

The test needs something to provide the required resources. I've made my proposal; if it needs to be reworked later that's fine, but I don't think we should be expected to achieve the long-term goal now, and I don't see an actionable alternative being proposed.

galak · 2019-09-25T14:57:12Z

Would be nice if we could add the core changes to the test and resolve the DT stuff later.

The test needs something to provide the required resources. I've made my proposal; if it needs to be reworked later that's fine, but I don't think we should be expected to achieve the long-term goal now, and I don't see an actionable alternative being proposed.

I'm good with merging this as its a step forward and while I agree with @mnkp I don't have a great answer and this at least makes things work for this test - until we have some defined hardware shield or something that could be utilized across a wider set of tests.

erwango

Test assumes both pins are on same device.
I think we should not impose this constraint, specially if we want to enable tests with pre-defined generic test pins like ardiuno D0/D1.

pabigot · 2019-09-26T22:21:03Z

@erwango Is this a requirement you wish to impose before we merge the test to the topic branch?

As it stands we have no merged test that covers the functional requirements of the drivers. I understand and in general support the desire to do things right rather than do them over, but we also need to complete and provide the basic tools necessary so we can ask people to port the drivers.

There are no in-tree boards with arduino-header compatibles where D0 and D1 are not on the same peripheral, which is beside the point as this test does not currently use the arduino map. Could we perhaps move forward now, and address cross-device support in the context of a separate future discussion of how the pins should be specified?

erwango · 2019-09-26T22:25:28Z

@pabigot, I'm fine to do that later, but then maybe we should add an error check if PIN_IN and PIN_OUT are on a different device ?

pabigot · 2019-09-26T22:29:57Z

@erwango that's currently done in board_setup() in main.c when the pin information comes from test,gpio_basic_api, which is the only situation where it's possible to select pins on different devices. I tested the error/panic before I did the last update.

erwango · 2019-09-26T22:38:00Z

@pabigot, ok, right, just seen and tested. Works perfectly. Let's go with it then. Removing my nack (not acking though, since I'm still testing)

Test that the new port API functions all behave as expected, including physical vs logical level for input and output as well as masked and set-based output operations. Also tests the new pin API functions. For running on real hardware this test now uses a local test-specific devicetree binding. For build-only tests any platform with a GPIO alias should be tested. The new code increases flash requirements so add a filter to exclude platforms that won't link. Signed-off-by: Peter Bigot <[email protected]>

Switch to gpio_pin_interrupt_configure() and the new interrupt flags. Use logical level pin set operations. Signed-off-by: Peter Bigot <[email protected]>

Switch to gpio_pin_interrupt_configure() and the new interrupt flags. Use logical level pin set operations. Test all standard interrupt configurations including double edge. Signed-off-by: Peter Bigot <[email protected]>

pabigot · 2019-10-02T15:51:16Z

Update with two changes

Fail fast: if it appears that the output is not wired to the input stop immediately.
Verify pull behavior: test that input pull configurations work and are not affected by logic level. I believe this is the expected interpretation, however frdm_k64f fails.

galak · 2019-10-02T20:55:40Z

Verify pull behavior: test that input pull configurations work and are not affected by logic level. I believe this is the expected interpretation, however frdm_k64f fails.

Fix to make frdm_k64f pass is in PR #19556

while DTS changes aren't great, its forward progress.

pabigot requested a review from nashif as a code owner September 18, 2019 14:11

pabigot requested review from MaureenHelm, mnkp and galak September 18, 2019 14:11

pabigot added area: GPIO area: Tests Issues related to a particular existing or missing test labels Sep 18, 2019

pabigot force-pushed the gpio/test2pin branch from 1853182 to 691328e Compare September 18, 2019 21:24

pabigot mentioned this pull request Sep 19, 2019

[TOPIC-GPIO] gpio: Update mcux igpio driver to use new gpio api #19247

Merged

galak force-pushed the topic-gpio branch from 5fc6ccb to 4bc09ff Compare September 19, 2019 20:29

galak force-pushed the gpio/test2pin branch from 691328e to 548b339 Compare September 19, 2019 20:30

pabigot mentioned this pull request Sep 19, 2019

[TOPIC-GPIO] miscellaneous API documentation updates #19252

Merged

pabigot force-pushed the gpio/test2pin branch from 548b339 to 08030f4 Compare September 19, 2019 22:58

zephyrbot added the area: Devicetree label Sep 19, 2019

pabigot force-pushed the gpio/test2pin branch 2 times, most recently from a434324 to 7599e57 Compare September 20, 2019 12:18

galak reviewed Sep 20, 2019

View reviewed changes

galak reviewed Sep 23, 2019

View reviewed changes

dts/bindings/misc/test,resources.yaml Outdated

properties:

gpios:

type: compound

Copy link

Collaborator

galak Sep 23, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type: phandle-array

galak reviewed Sep 23, 2019

View reviewed changes

pabigot force-pushed the gpio/test2pin branch 2 times, most recently from 785fee3 to ac0c44d Compare September 23, 2019 14:46

mnkp reviewed Sep 23, 2019

View reviewed changes

zephyrbot added the area: Build System label Sep 24, 2019

galak reviewed Sep 24, 2019

View reviewed changes

SebastianBoe removed their request for review September 24, 2019 13:21

pabigot force-pushed the gpio/test2pin branch from 6a6e0b7 to f8f2d21 Compare September 24, 2019 14:16

mnkp previously requested changes Sep 24, 2019

View reviewed changes

pabigot force-pushed the gpio/test2pin branch from f8f2d21 to 51f45fc Compare September 25, 2019 14:24

galak approved these changes Sep 25, 2019

View reviewed changes

pabigot force-pushed the gpio/test2pin branch from 51f45fc to 5f82e5f Compare September 25, 2019 15:46

pabigot mentioned this pull request Sep 25, 2019

[TOPIC-GPIO] drivers: gpio_sx1509b: update to use new GPIO API #19379

Merged

erwango previously requested changes Sep 26, 2019

View reviewed changes

erwango self-requested a review September 26, 2019 22:38

galak force-pushed the topic-gpio branch from 470d655 to 683ab8f Compare October 2, 2019 03:39

galak force-pushed the gpio/test2pin branch from 5f82e5f to 4ca4cf7 Compare October 2, 2019 03:40

pabigot added 3 commits October 2, 2019 10:48

tests/drivers/gpio: replace legacy API in callback manage tests

e88026e

Switch to gpio_pin_interrupt_configure() and the new interrupt flags. Use logical level pin set operations. Signed-off-by: Peter Bigot <[email protected]>

tests/drivers/gpio: replace legacy API in callback tests

623f250

Switch to gpio_pin_interrupt_configure() and the new interrupt flags. Use logical level pin set operations. Test all standard interrupt configurations including double edge. Signed-off-by: Peter Bigot <[email protected]>

pabigot force-pushed the gpio/test2pin branch from 4ca4cf7 to 623f250 Compare October 2, 2019 15:50

galak merged commit 8b92251 into zephyrproject-rtos:topic-gpio Oct 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TOPIC-GPIO] convert 2-pin test to new API #19249

[TOPIC-GPIO] convert 2-pin test to new API #19249

pabigot commented Sep 18, 2019 •

edited

Loading

zephyrbot commented Sep 18, 2019 •

edited

Loading

galak left a comment

pabigot commented Sep 20, 2019

galak commented Sep 20, 2019

pabigot commented Sep 20, 2019

pabigot commented Sep 22, 2019

galak Sep 23, 2019

galak Sep 23, 2019

pabigot Sep 23, 2019

galak left a comment

galak commented Sep 23, 2019

pabigot commented Sep 23, 2019

pabigot commented Sep 23, 2019

mnkp Sep 23, 2019

pabigot commented Sep 24, 2019

galak Sep 24, 2019

pabigot Sep 24, 2019

mnkp left a comment

galak commented Sep 25, 2019

pabigot commented Sep 25, 2019

galak commented Sep 25, 2019

erwango left a comment

pabigot commented Sep 26, 2019

erwango commented Sep 26, 2019

pabigot commented Sep 26, 2019 •

edited

Loading

erwango commented Sep 26, 2019

pabigot commented Oct 2, 2019

galak commented Oct 2, 2019

[TOPIC-GPIO] convert 2-pin test to new API #19249

[TOPIC-GPIO] convert 2-pin test to new API #19249

Conversation

pabigot commented Sep 18, 2019 • edited Loading

zephyrbot commented Sep 18, 2019 • edited Loading

checkpatch (informational only, not a failure)

galak left a comment

Choose a reason for hiding this comment

pabigot commented Sep 20, 2019

galak commented Sep 20, 2019

pabigot commented Sep 20, 2019

pabigot commented Sep 22, 2019

galak Sep 23, 2019

Choose a reason for hiding this comment

galak Sep 23, 2019

Choose a reason for hiding this comment

pabigot Sep 23, 2019

Choose a reason for hiding this comment

galak left a comment

Choose a reason for hiding this comment

galak commented Sep 23, 2019

pabigot commented Sep 23, 2019

pabigot commented Sep 23, 2019

mnkp Sep 23, 2019

Choose a reason for hiding this comment

pabigot commented Sep 24, 2019

galak Sep 24, 2019

Choose a reason for hiding this comment

pabigot Sep 24, 2019

Choose a reason for hiding this comment

mnkp left a comment

Choose a reason for hiding this comment

galak commented Sep 25, 2019

pabigot commented Sep 25, 2019

galak commented Sep 25, 2019

erwango left a comment

Choose a reason for hiding this comment

pabigot commented Sep 26, 2019

erwango commented Sep 26, 2019

pabigot commented Sep 26, 2019 • edited Loading

erwango commented Sep 26, 2019

pabigot commented Oct 2, 2019

galak commented Oct 2, 2019

pabigot commented Sep 18, 2019 •

edited

Loading

zephyrbot commented Sep 18, 2019 •

edited

Loading

pabigot commented Sep 26, 2019 •

edited

Loading