drivers: adc_shell: rework commands #28599

jakub-uC · 2020-09-22T13:24:58Z

ADC shell commands can be only executed on the property label exiting in the device tree. It is realized by the dynamic subcommand returning the existing ADC Label.

Fixes #27350

Signed-off-by: Jakub Rzeszutko [email protected]

jakub-uC · 2020-09-22T13:25:57Z

@avigreen1978 may you please test it?

erwango · 2020-09-22T19:12:36Z

drivers/adc/adc_shell.c

 #define ADC_HDL_LIST_ENTRY(inst)					\
 	{								\
-		.device_name = DT_INST_LABEL(inst),			\
+		.device_name = inst,					\


If I'm reading it right (otherwise, please discard this comment):
While this is working and doing the expected thing, the use of inst is misleading here.
In DT macros, we're usually calling INST the ordinal number of the node (2nd, 3rd, ..).
In ADC_HDL_LIST_ENTRY, the argument is actually the label property value ("ADC_X" string).

So, to clarify this piece of code, I'd switch argument to something less misleading: node_label or similar.

EDIT: Reason for which I care is that I think this is the piece of code is likely to be useful and reused in other parts of the code. So let's make it as clear as possible.

Sure, I will update the naming. It is a property label like ADC_0 or ADC_1.

erwango · 2020-09-22T19:16:56Z

@jakub-uC basic test ok

erwango

Approved from my side, but need ADC friends reviews.

nordic-krch · 2020-09-24T11:46:48Z

drivers/adc/adc_shell.c

-	SHELL_CMD(EXT0, NULL, "EXT0", cmd_adc_ref),
-	SHELL_CMD(EXT1, NULL, "EXT1", cmd_adc_ref),
-	SHELL_SUBCMD_SET_END /* Array terminated. */
+	SHELL_CMD_ARG(VDD_1, NULL, "VDD", cmd_adc_ref, 1, 0),


we could think at some point about shell extension which would allow creating subcommand with a dictionary. In that case argv would contain direct value. Something like

SHELL_STATIC_SUBCMD_DICT_CREATE(sub, dictionary_array)

where array contains string + value. String for tab handle, value passed to command handler. That would make code much compact in cases like this and there are many cases like this one.

May you please create an issue for that?

anangl · 2020-09-28T12:30:36Z

drivers/adc/adc_shell.c

+	"Configure resolution\n"		\
+	"Usage: resolution <resolution>\n"
+
+#define CMD_HELP_REF	"Configure resolution\n"


resolution => reference

anangl · 2020-09-28T13:35:45Z

drivers/adc/adc_shell.c

-			&adc_list[chosen_adc].channel_config);
+
+	if (IS_ENABLED(CONFIG_ADC_CONFIGURABLE_INPUTS)) {
+		adc->channel_config.input_positive = 1;


This should not be hard-coded. Users should be able to select the input to use. And this value may be even invalid for some drivers.

@anangl Do you think it is a good idea to add one more command for setting this value?

carlescufi · 2020-10-02T09:45:34Z

@anangl please take another look

anangl · 2020-10-02T11:01:12Z

drivers/adc/adc_shell.c

+
+#define CMD_HELP_CH_ID				\
+	"Configure channel id\n"		\
+	"Usage: channel <channel_id>\n"


I guess this should be

Suggested change

"Usage: channel <channel_id>\n"

"Usage: id <channel_id>\n"

anangl · 2020-10-02T11:08:08Z

drivers/adc/adc_shell.c

+	{ .string = "ADC_GAIN_8", .gain = ADC_GAIN_8 },
+	{ .string = "ADC_GAIN_16", .gain = ADC_GAIN_16 },
+	{ .string = "ADC_GAIN_32", .gain = ADC_GAIN_32 },
+	{ .string = "ADC_GAIN_64", .gain = ADC_GAIN_64 }


Could you add ADC_GAIN_128 as well (here and in sub_gain_cmds), as it was added in adc.h?

pabigot · 2020-10-08T14:18:10Z

Last dev review we'd discussed #28566 and identified a generic way to associate devices with specific APIs. @jakub-uC are you pursuing that approach? Would doing that supersede this proposal?

It's a little hard to understand what this is doing since it packs a bunch of refactoring/reformatting along with the described device access change into a single commit. I think there's a device access change in there somewhere.

jakub-uC · 2020-10-09T10:01:26Z

@pabigot : I think we can merge this PR and next it could be updated according to your proposal.

The most important parts of the code of this change are here:

static void cmd_adc_dev_get(size_t idx, struct shell_static_entry *entry)
{
	/* -1 because the last element in the list is a "list terminator" */
	if (idx < ARRAY_SIZE(adc_list) - 1) {
		entry->syntax  = adc_list[idx].device_label;  // <=== HERE
		entry->handler = NULL;
		entry->subcmd  = &sub_adc_cmds;
		entry->help    = "Select subcommand for ADC property label.\n";
	} else {
		entry->syntax  = NULL;
	}
}

Here: entry->syntax = adc_list[idx].device_label; I need an array of strings with active ADCs. This string is used to: prompt a device name in the shell and bind an ADC device in the command handler.

A list of strings is created here:

#define INIT_MACRO() DT_INST_FOREACH_STATUS_OKAY(NODE_LABELS) "NA"

/* This table size is = ADC devices count + 1 (NA). */
static struct adc_hdl {
	char *device_label;
	struct adc_channel_cfg channel_config;
	uint8_t resolution;
} adc_list[] = {
	FOR_EACH(ADC_HDL_LIST_ENTRY, (,), INIT_MACRO())
};

I believe it is also part of the code that you would need to change according to your proposal.

pabigot

From #28599 (comment)

@pabigot : I think we can merge this PR and next it could be updated according to your proposal.

The most important parts of the code of this change are here:

This ideally would have been done in multiple, focused commits. Among all the re-ordering of help and structure definitions, conversion from array index to pointer, etc it's not at all clear what was done and why. Since this already has three approvals I'm not going to block this, but I'm not going to approve it because it's not worth the time to try to figure it all out.

nordic-krch · 2020-10-09T10:55:43Z

@jakub-uC what about using dictionary commands? Are you plan it in this PR?

jakub-uC · 2020-10-09T11:00:05Z

@nordic-krch : Yes I am reworking that and splitting to different commits.
When this PR was submitted there were no dictionary commands on the master branch.

Changed gain and reference commands to dictionary commands. This change removes an obsolete look-up table (string <-> value) for gain and reference commands. Now, each modification of gain or reference value will require only a dictionary command update. Signed-off-by: Jakub Rzeszutko <[email protected]>

Use dedicated shell macros so argument count can be validated before the command handler is executed. This change simplifies the command handlers implementation inside the adc_shell file. Signed-off-by: Jakub Rzeszutko <[email protected]>

The channel command has been extended with subcommands: id, positive, and negetive. Some boards require positive input configuration before measurement can be started. Signed-off-by: Jakub Rzeszutko <[email protected]>

ADC shell commands can be only executed on the property label existing in the device tree. It is realized by the dynamic subcommand returning existing ADC Label. Signed-off-by: Jakub Rzeszutko <[email protected]>

jakub-uC · 2020-10-09T15:49:30Z

@pabigot I've tried to make reasonable commits. The most important is the last commit: adc_shell: rework commands

In order to prompt valid ADC labels I've implemented dynamic command that is realized by the function static void cmd_adc_dev_get(size_t idx, struct shell_static_entry *entry). What this function needs for proper work is an array of valid ADC labels that can be used to bind the ADC device.

If we know that the ADC label, for example, ADC_0, is prompted by the shell as a subcommand it is not needed in the commands handler to check if the device is on the list. This is why commands were simplified.

jakub-uC requested a review from anangl as a code owner September 22, 2020 13:24

github-actions bot added the area: ADC Analog-to-Digital Converter (ADC) label Sep 22, 2020

jakub-uC requested review from nordic-krch and erwango September 22, 2020 13:25

jakub-uC added area: Drivers area: Shell Shell subsystem DNM This PR should not be merged (Do Not Merge) labels Sep 22, 2020

jakub-uC force-pushed the adc-shell-rework-commands branch 2 times, most recently from 33e1c9c to 01bf417 Compare September 22, 2020 15:00

erwango reviewed Sep 22, 2020

View reviewed changes

jakub-uC force-pushed the adc-shell-rework-commands branch from 01bf417 to fe4bfa2 Compare September 23, 2020 08:45

jakub-uC changed the title ~~[DNM] drivers: adc_shell: rework commands~~ drivers: adc_shell: rework commands Sep 23, 2020

jakub-uC removed the DNM This PR should not be merged (Do Not Merge) label Sep 23, 2020

jakub-uC force-pushed the adc-shell-rework-commands branch 2 times, most recently from 623827d to 7308b4b Compare September 23, 2020 09:08

jakub-uC added the DNM This PR should not be merged (Do Not Merge) label Sep 23, 2020

jakub-uC force-pushed the adc-shell-rework-commands branch from 7308b4b to 577dc12 Compare September 23, 2020 13:58

jakub-uC removed the DNM This PR should not be merged (Do Not Merge) label Sep 23, 2020

erwango approved these changes Sep 23, 2020

View reviewed changes

jakub-uC force-pushed the adc-shell-rework-commands branch from 577dc12 to 282444d Compare September 24, 2020 06:52

erwango requested review from mbolivar-nordic and galak September 24, 2020 07:13

jakub-uC force-pushed the adc-shell-rework-commands branch from 282444d to c47619a Compare September 24, 2020 10:02

nordic-krch reviewed Sep 24, 2020

View reviewed changes

nordic-krch approved these changes Sep 28, 2020

View reviewed changes

anangl reviewed Sep 28, 2020

View reviewed changes

jakub-uC force-pushed the adc-shell-rework-commands branch from c47619a to 463d5b0 Compare September 29, 2020 07:29

erwango mentioned this pull request Sep 29, 2020

ADC: adc shell failure when mismatch with dts device label #27350

Closed

anangl reviewed Oct 2, 2020

View reviewed changes

jakub-uC force-pushed the adc-shell-rework-commands branch from 463d5b0 to b59dd63 Compare October 2, 2020 11:54

anangl approved these changes Oct 2, 2020

View reviewed changes

pabigot self-requested a review October 9, 2020 10:25

pabigot reviewed Oct 9, 2020

View reviewed changes

Jakub Rzeszutko added 4 commits October 9, 2020 16:06

adc_shell: extend channel command

a2fa661

The channel command has been extended with subcommands: id, positive, and negetive. Some boards require positive input configuration before measurement can be started. Signed-off-by: Jakub Rzeszutko <[email protected]>

adc_shell: rework commands

b097382

ADC shell commands can be only executed on the property label existing in the device tree. It is realized by the dynamic subcommand returning existing ADC Label. Signed-off-by: Jakub Rzeszutko <[email protected]>

jakub-uC force-pushed the adc-shell-rework-commands branch from b59dd63 to b097382 Compare October 9, 2020 15:43

carlescufi requested a review from pabigot October 13, 2020 11:15

carlescufi merged commit 6ed1367 into zephyrproject-rtos:master Oct 14, 2020

erwango mentioned this pull request Feb 13, 2024

List property label for all active ADCs regardless of the board used #28566

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: adc_shell: rework commands #28599

drivers: adc_shell: rework commands #28599

jakub-uC commented Sep 22, 2020 •

edited

Loading

jakub-uC commented Sep 22, 2020

erwango Sep 22, 2020 •

edited

Loading

jakub-uC Sep 22, 2020

erwango commented Sep 22, 2020

erwango left a comment

nordic-krch Sep 24, 2020

jakub-uC Sep 24, 2020

anangl Sep 28, 2020

jakub-uC Sep 29, 2020

anangl Sep 28, 2020

jakub-uC Sep 28, 2020

carlescufi commented Oct 2, 2020

anangl Oct 2, 2020

jakub-uC Oct 2, 2020

anangl Oct 2, 2020

jakub-uC Oct 2, 2020

pabigot commented Oct 8, 2020

jakub-uC commented Oct 9, 2020

pabigot left a comment

nordic-krch commented Oct 9, 2020

jakub-uC commented Oct 9, 2020

jakub-uC commented Oct 9, 2020

drivers: adc_shell: rework commands #28599

drivers: adc_shell: rework commands #28599

Conversation

jakub-uC commented Sep 22, 2020 • edited Loading

jakub-uC commented Sep 22, 2020

erwango Sep 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwango commented Sep 22, 2020

erwango 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

carlescufi commented Oct 2, 2020

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

pabigot commented Oct 8, 2020

jakub-uC commented Oct 9, 2020

pabigot left a comment

Choose a reason for hiding this comment

nordic-krch commented Oct 9, 2020

jakub-uC commented Oct 9, 2020

jakub-uC commented Oct 9, 2020

jakub-uC commented Sep 22, 2020 •

edited

Loading

erwango Sep 22, 2020 •

edited

Loading