drivers: gpio: shell: improve usability #63019

nixward · 2023-09-24T06:20:58Z

This commit implements this enhancement:
#63018

Also adds a 'info' command for ngpios, reversed pin and line
name information.

The forms of the gpio commands are now:

    gpio conf device pin ol0
    gpio set device pin 1
    gpio get device pin
    gpio blink device pin
    gpio info device

Device name, pin, configuration and set level sub commands now
are suggested/completed when tab is used.

Pin names are suggested with numbers and if available from the Devicetree
gpio line names.

GPIO pin control is now limited to pins that are not assigned as reserved.

Fixes #63018

nixward · 2023-09-25T09:32:58Z

@fabiobaltieri would you mind having a look at this?

nixward · 2023-09-26T10:44:43Z

@nandojve would you mind having a look as well?

nixward · 2023-09-28T12:28:30Z

@aasinclair I added a commit to add more gpio configuration options, I'm not sure it's exhaustive but it's expanded (and maybe the most useful). See what you think.

aasinclair · 2023-09-28T14:00:03Z

Hi @nixward.
Would it be possible to support a custom value, maybe something like:
uart:~$ gpio gpio@50000000 17 rawconf 0x10000

Implemented like this?


static int cmd_rawconf(const struct shell *sh, size_t argc, char **argv, void *data)
{
	struct sh_gpio gpio;
	uint32_t flags;
	int ret = 0;

	get_sh_gpio(sh, argv, -2, &gpio);

	flags = shell_strtoul(argv[1], 0, &ret);
	if (ret != 0) {
		shell_error(sh, "error: %d", ret);
		return ret;
	}

	ret = gpio_pin_configure(gpio.dev, gpio.pin, flags);
	if (ret != 0) {
		shell_error(sh, "error: %d", ret);
		return ret;
	}

	return 0;
}

nixward · 2023-09-29T12:13:44Z

Hi @aasinclair I've been reading the gpio API headers include/zephyr/drivers/gpio.h and include/zephyr/dt-bindings/gpio.h should only flags from the set (GPIO_INPUT | GPIO_OUTPUT | GPIO_DT_FLAGS_MASK) be allowed? To me it doesn't make sense to allow flags from GPIO_INT_MASK.

aasinclair · 2023-10-02T08:28:32Z

Hi @aasinclair I've been reading the gpio API headers include/zephyr/drivers/gpio.h and include/zephyr/dt-bindings/gpio.h should only flags from the set (GPIO_INPUT | GPIO_OUTPUT | GPIO_DT_FLAGS_MASK) be allowed? To me it doesn't make sense to allow flags from GPIO_INT_MASK.

Hi @nixward, allowing a custom value is most useful for device specific configuration (driver strength, debounce etc).
E.g.:
https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/dt-bindings/gpio/nuvoton-npcx-gpio.h
or
https://github.com/zephyrproject-rtos/zephyr/blob/main/include/zephyr/dt-bindings/gpio/atmel-sam-gpio.h

keith-zephyr · 2023-10-03T23:19:26Z

drivers/gpio/gpio_shell.c

-		shell_error(sh, "Wrong parameters for get");
-		return -EINVAL;
-	}
+	get_sh_gpio(sh, argv, -2, &gpio);


Please add some constants so you can calculate the argv index.
For instance:

#define ARGV_GPIO_PORT 1 #define ARGV_GPIO_PIN 2 #define ARGV_SUBCMD 3 #define ARGV_SET_VALUE 4

Then this call here becomes

get_sh_gpio(sh, argv, ARGV_GPIO_PORT - ARGV_GPIO_SUBCMD, &gpio);

Nice one, I'll add 👍

By reshuffling the args, it would be possible to remove the table pin_syntax and make it as a standard manual entry:

#define ARGV_SUBCMD 1 #define ARGV_SET_VALUE 2 #define ARGV_GPIO_PORT 3 #define ARGV_GPIO_PIN 4

keith-zephyr · 2023-10-03T23:28:13Z

drivers/gpio/gpio_shell.c

+	(inu, (GPIO_INPUT | GPIO_PULL_UP), "input with pull up"),
+	(ind, (GPIO_INPUT | GPIO_PULL_DOWN), "input with pull down"),


Non-blocking suggestion:

Suggested change

(inu, (GPIO_INPUT | GPIO_PULL_UP), "input with pull up"),

(ind, (GPIO_INPUT | GPIO_PULL_DOWN), "input with pull down"),

(in_pu, (GPIO_INPUT | GPIO_PULL_UP), "input with pull up"),

(in_pd, (GPIO_INPUT | GPIO_PULL_DOWN), "input with pull down"),

keith-zephyr · 2023-10-03T23:36:48Z

drivers/gpio/gpio_shell.c

+	(al_in, (GPIO_ACTIVE_LOW_INPUT), "active low input"),
+	(al_inu, (GPIO_ACTIVE_LOW_INPUT | GPIO_PULL_UP), "active low input with pull up"),
+	(al_ind, (GPIO_ACTIVE_LOW_INPUT | GPIO_PULL_DOWN), "active low input with pull down"),
+	(al_out, (GPIO_ACTIVE_LOW_OUTPUT), "active low output")


Two alternatives:

Suggested change

(al_in, (GPIO_ACTIVE_LOW_INPUT), "active low input"),

(al_inu, (GPIO_ACTIVE_LOW_INPUT | GPIO_PULL_UP), "active low input with pull up"),

(al_ind, (GPIO_ACTIVE_LOW_INPUT | GPIO_PULL_DOWN), "active low input with pull down"),

(al_out, (GPIO_ACTIVE_LOW_OUTPUT), "active low output")

(al_in, (GPIO_ACTIVE_LOW_INPUT), "active low input"),

(al_in_pu, (GPIO_ACTIVE_LOW_INPUT | GPIO_PULL_UP), "active low input with pull up"),

(al_in_pd, (GPIO_ACTIVE_LOW_INPUT | GPIO_PULL_DOWN), "active low input with pull down"),

(al_out, (GPIO_ACTIVE_LOW_OUTPUT), "active low output")

Suggested change

(al_in, (GPIO_ACTIVE_LOW_INPUT), "active low input"),

(al_inu, (GPIO_ACTIVE_LOW_INPUT | GPIO_PULL_UP), "active low input with pull up"),

(al_ind, (GPIO_ACTIVE_LOW_INPUT | GPIO_PULL_DOWN), "active low input with pull down"),

(al_out, (GPIO_ACTIVE_LOW_OUTPUT), "active low output")

(in#, (GPIO_ACTIVE_LOW_INPUT), "active low input"),

(in_pu#, (GPIO_ACTIVE_LOW_INPUT | GPIO_PULL_UP), "active low input with pull up"),

(in_pd#, (GPIO_ACTIVE_LOW_INPUT | GPIO_PULL_DOWN), "active low input with pull down"),

(out#, (GPIO_ACTIVE_LOW_OUTPUT), "active low output")

marco85mars20

@nixward what do you think about these suggestions?

marco85mars20 · 2023-10-12T13:25:30Z

drivers/gpio/gpio_shell.c

-		shell_error(sh, "Wrong parameters for get");
-		return -EINVAL;
-	}
+	get_sh_gpio(sh, argv, -2, &gpio);


By reshuffling the args, it would be possible to remove the table pin_syntax and make it as a standard manual entry:

#define ARGV_SUBCMD 1 #define ARGV_SET_VALUE 2 #define ARGV_GPIO_PORT 3 #define ARGV_GPIO_PIN 4

marco85mars20 · 2023-10-12T13:47:36Z

drivers/gpio/gpio_shell.c

+static const char * const pin_syntax[] = {
+	 "0",  "1",  "2",  "3",  "4",  "5",  "6",  "7",  "8",  "9",
+	"10", "11", "12", "13", "14", "15", "16", "17", "18", "19",
+	"20", "21", "22", "23", "24", "25", "26", "27", "28", "29",
+	"30", "31"
+};
+
+static void cmd_gpio_pin_get(size_t idx, struct shell_static_entry *entry)
+{
+	if (idx < ARRAY_SIZE(pin_syntax)) {
+		entry->syntax  = pin_syntax[idx];
+		entry->handler = NULL;
+		entry->subcmd = &sub_gpio;
+		entry->help = "Pin";
+	} else {
+		entry->syntax = NULL;
+		entry->subcmd = &sub_gpio;
+	}
+}


Would it be possible to remove pin_syntax and generate the list of available pins on the fly based on DT_PROP(DT_NODELABEL(dev->name), ngpios) ?

Suggested change

static const char * const pin_syntax[] = {

"0", "1", "2", "3", "4", "5", "6", "7", "8", "9",

"10", "11", "12", "13", "14", "15", "16", "17", "18", "19",

"20", "21", "22", "23", "24", "25", "26", "27", "28", "29",

"30", "31"

};

static void cmd_gpio_pin_get(size_t idx, struct shell_static_entry *entry)

{

if (idx < ARRAY_SIZE(pin_syntax)) {

entry->syntax = pin_syntax[idx];

entry->handler = NULL;

entry->subcmd = &sub_gpio;

entry->help = "Pin";

} else {

entry->syntax = NULL;

entry->subcmd = &sub_gpio;

}

}

static void cmd_gpio_pin_get(size_t idx, struct shell_static_entry *entry)

{

struct sh_gpio gpio;

uint8_t ngpios;

/* NOTE: Not sure how to retrieve 'sh' */

get_sh_gpio(sh, argv, ARGV_GPIO_PORT - ARGV_GPIO_PIN, &gpio);

ngpios = DT_PROP_OR(DT_NODELABEL(gpio.dev->name), ngpios, 0);

if (idx < ngpios) {

entry->syntax = pin_syntax[idx];

entry->handler = NULL;

entry->subcmd = &sub_gpio;

entry->help = "Pin";

} else {

entry->syntax = NULL;

entry->subcmd = &sub_gpio;

}

}

@marco85mars20 yes, similar to this I've been using the Devicetree to generate the pin argument code, I'll hopefully push soon.

keith-zephyr · 2023-11-06T16:00:22Z

boards/arm/nrf52840dk_nrf52840/nrf52840dk_nrf52840.dts

+	gpio-reserved-ranges = <0 1>, <12 4>;
+	gpio-line-names = "", "D0", "D1", "D2", "D3", "D4", "D5", "D6",
+		"D7", "", "D8", "D9", "", "", "", "";


Do you need a reserved range for <9 1>?

keith-zephyr · 2023-11-06T16:07:02Z

drivers/gpio/gpio_shell.c

+	if (((flags & GPIO_INPUT) != 0) == ((flags & GPIO_OUTPUT) != 0)) {
+		shell_error(sh, "must be either input or output");
+		shell_help(sh);
+		return SHELL_CMD_HELP_PRINTED;
+	}


Non-blocking comment - This duplicates the logic from gpio_pin_configure(), which uses asserts.
As a follow up CL, you could create a helper function for this validation that is used both by the shell command and gpio_pin_configure().

keith-zephyr · 2023-11-06T16:11:12Z

drivers/adc/adc_ads114s0x.c

@@ -1080,7 +1080,7 @@ static int ads114s0x_gpio_write_config(const struct device *dev)
 {
 	struct ads114s0x_data *data = dev->data;
 	const struct ads114s0x_config *config = dev->config;
-	uint8_t register_addresses[2];
+	enum ads114s0x_register register_addresses[2];


This commit and the following commits are not related to the GPIO shell changes. Please move to a separate PR.

This issue was was one of many with the ads114s0x_gpio driver/test that were causing CI to fail for this PR. I've opened #64876

#64876 has merged. Please rebase this PR.

keith-zephyr

Looks good to me once you drop the top two commits that enable the GPIO shell by default.

Add gpio-reserved-ranges and gpio-line-names properties. Signed-off-by: Nick Ward <[email protected]>

This commit implements this enhancement: zephyrproject-rtos#63018 The forms of the gpio commands are now: gpio conf device pin ol0 gpio set device pin 1 gpio get device pin gpio blink device pin Device name and pin subcommands now are suggested/completed when tab is used. Pin names are suggested with numbers and line names if available from the gpio controller’s Devicetree node. GPIO pin command is now limited to pins that are not assigned as reserved. Signed-off-by: Nick Ward <[email protected]>

Usage: gpio info [device] The new command prints gpio controller information for a specific device if specified or if no device is specified it prints out all controller information ordered by line name. Also added Kconfig option so this command can be removed if resources need to be conserved. Signed-off-by: Nick Ward <[email protected]>

Adds CONFIG_GPIO_SHELL_BLINK_CMD symbol. Saves around 300 bytes when command is disabled. Signed-off-by: Nick Ward <[email protected]>

Use gpio toggle api instead of manually toggling. Remove redundant text. Print error and break from blinking if it occurs. Only print 'how to exit' text if first toggle is successful. Saves roughly 40 bytes. Signed-off-by: Nick Ward <[email protected]>

Allow the optional setting of vendor specific flags in the conf command. Signed-off-by: Nick Ward <[email protected]>

nixward · 2023-11-16T20:15:04Z

Looks good to me once you drop the top two commits that enable the GPIO shell by default.

Done

nixward requested a review from mnkp as a code owner September 24, 2023 06:20

zephyrbot added the area: GPIO label Sep 24, 2023

zephyrbot requested a review from henrikbrixandersen September 24, 2023 06:21

nixward force-pushed the gpio_shell_improve_tab_complete_support branch from eff0740 to ac76d79 Compare September 24, 2023 10:01

nixward changed the title ~~drivers: gpio: shell: improve tab complete support~~ drivers: gpio: shell: improve usability Sep 24, 2023

nandojve requested a review from fabiobaltieri September 26, 2023 15:31

nixward mentioned this pull request Sep 28, 2023

drivers: gpio: shell: Improved error reporting #61796

Closed

nixward force-pushed the gpio_shell_improve_tab_complete_support branch from ac76d79 to bf02499 Compare September 28, 2023 11:27

nixward requested a review from aasinclair September 28, 2023 11:33

nixward force-pushed the gpio_shell_improve_tab_complete_support branch from bf02499 to c1e6d11 Compare September 28, 2023 12:21

nixward force-pushed the gpio_shell_improve_tab_complete_support branch from c1e6d11 to 09783be Compare September 28, 2023 13:40

nixward force-pushed the gpio_shell_improve_tab_complete_support branch from 09783be to 5a2e054 Compare September 28, 2023 23:12

nixward added the In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on label Sep 29, 2023

fabiobaltieri requested review from keith-zephyr and cfriedt October 3, 2023 13:48

keith-zephyr reviewed Oct 3, 2023

View reviewed changes

marco85mars20 reviewed Oct 12, 2023

View reviewed changes

nixward force-pushed the gpio_shell_improve_tab_complete_support branch from 5a2e054 to f41c62e Compare October 22, 2023 00:55

nixward requested review from nashif, jakub-uC, nordic-krch and carlescufi as code owners October 22, 2023 00:55

zephyrbot added area: Shell Shell subsystem platform: nRF Nordic nRFx labels Oct 22, 2023

zephyrbot added the area: ADC Analog-to-Digital Converter (ADC) label Nov 3, 2023

nixward force-pushed the gpio_shell_improve_tab_complete_support branch 3 times, most recently from 7f5bd2b to 8a475dd Compare November 4, 2023 04:19

zephyrbot added the area: EEPROM label Nov 4, 2023

nixward force-pushed the gpio_shell_improve_tab_complete_support branch 2 times, most recently from 059c6d3 to d3b0f0d Compare November 4, 2023 11:30

keith-zephyr reviewed Nov 6, 2023

View reviewed changes

nixward force-pushed the gpio_shell_improve_tab_complete_support branch from d3b0f0d to 3c5c7fe Compare November 6, 2023 19:35

nixward mentioned this pull request Nov 6, 2023

Fix ci build issue for ads1145s0x #64876

Merged

nixward force-pushed the gpio_shell_improve_tab_complete_support branch 2 times, most recently from 1d20828 to 4cc8d1c Compare November 6, 2023 20:12

nixward force-pushed the gpio_shell_improve_tab_complete_support branch 2 times, most recently from c42260c to 02dfbfa Compare November 15, 2023 20:35

zephyrbot added the Release Notes To be mentioned in the release notes label Nov 15, 2023

nixward removed the In progress For PRs: is work in progress and should not be merged yet. For issues: Is being worked on label Nov 15, 2023

keith-zephyr reviewed Nov 16, 2023

View reviewed changes

nixward added 6 commits November 17, 2023 07:09

boards: nrf52840dk_nrf52840: add gpio reserved ranges and line names

116ae3e

Add gpio-reserved-ranges and gpio-line-names properties. Signed-off-by: Nick Ward <[email protected]>

drivers: gpio: shell: make blink command optional

e36c42f

Adds CONFIG_GPIO_SHELL_BLINK_CMD symbol. Saves around 300 bytes when command is disabled. Signed-off-by: Nick Ward <[email protected]>

drivers: gpio: shell: add vendor specific flags argument

f2c7ef3

Allow the optional setting of vendor specific flags in the conf command. Signed-off-by: Nick Ward <[email protected]>

nixward force-pushed the gpio_shell_improve_tab_complete_support branch from 02dfbfa to f2c7ef3 Compare November 16, 2023 20:11

keith-zephyr approved these changes Nov 16, 2023

View reviewed changes

aasinclair approved these changes Nov 17, 2023

View reviewed changes

carlescufi unassigned jakub-uC Nov 17, 2023

carlescufi merged commit cd9f307 into zephyrproject-rtos:main Nov 17, 2023
20 checks passed

nixward deleted the gpio_shell_improve_tab_complete_support branch November 17, 2023 14:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: gpio: shell: improve usability #63019

drivers: gpio: shell: improve usability #63019

nixward commented Sep 24, 2023 •

edited

Loading

nixward commented Sep 25, 2023

nixward commented Sep 26, 2023

nixward commented Sep 28, 2023

aasinclair commented Sep 28, 2023 •

edited

Loading

nixward commented Sep 29, 2023

aasinclair commented Oct 2, 2023

keith-zephyr Oct 3, 2023

nixward Oct 5, 2023

marco85mars20 Oct 12, 2023

keith-zephyr Oct 3, 2023

keith-zephyr Oct 3, 2023

marco85mars20 left a comment

marco85mars20 Oct 12, 2023

marco85mars20 Oct 12, 2023

nixward Oct 14, 2023

keith-zephyr Nov 6, 2023

nixward Nov 6, 2023

keith-zephyr Nov 6, 2023

keith-zephyr Nov 6, 2023

nixward Nov 6, 2023

keith-zephyr Nov 13, 2023

nixward Nov 16, 2023

keith-zephyr left a comment

nixward commented Nov 16, 2023

		(inu, (GPIO_INPUT \| GPIO_PULL_UP), "input with pull up"),
		(ind, (GPIO_INPUT \| GPIO_PULL_DOWN), "input with pull down"),

drivers: gpio: shell: improve usability #63019

drivers: gpio: shell: improve usability #63019

Conversation

nixward commented Sep 24, 2023 • edited Loading

nixward commented Sep 25, 2023

nixward commented Sep 26, 2023

nixward commented Sep 28, 2023

aasinclair commented Sep 28, 2023 • edited Loading

nixward commented Sep 29, 2023

aasinclair commented Oct 2, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marco85mars20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keith-zephyr left a comment

Choose a reason for hiding this comment

nixward commented Nov 16, 2023

nixward commented Sep 24, 2023 •

edited

Loading

aasinclair commented Sep 28, 2023 •

edited

Loading