Zephyr: ADC: adc shell failure when mismatch with dts device label

[avigreen1978]

Describe the bug
using adc command forces adc ADC_0 when ADC_1 is the first enabled ADC, causing any command to fail with device is not in device list error

To Reproduce
Try on any STM32 EVB

Expected behavior
The root cause for this bug is the ambiguous use of the sub command name, here "ADC_0", and the device label configured in the dts.
if "ADC_0" means "the first enabled ADC instance" in line 45: .device_name = DT_INST_LABEL(inst), sets the device_name field in the hdl struct as "ADC_1", the device label string defined in the dts.
But, in line 111 in function get_adc_from_list(char *name): if (!strcmp(name, adc_list[adc_idx].device_name)) { it is compared to the argv parameter given, which is "ADC_0". thus always failing the search.

Note: using "ADC_1" as shell parameter fails the shell core as this is not a valid subcommand.

This can happen in the following cases, which causes the enabled device under instance index x to no be labelled "ADC_x":

1. The arch dts device label defines are not 0-based (like in STM32 which start at ADC_1)
2. The first enabled device is not ADC_0, but ADC_1, or 2
3. The arch dts doesn't even use the ADC_x naming for its labels. it can be "A2D_0" (for digital-to-analog) or something like that.

Solution:
I suggest avoiding the double use of ADC_x for both the subcommand syntax and the device label. using index only for the subcommand. using adc 0 read 15 for example.

If this "ADC_x" naming convention is necessary, using "ADC_0" for "first enabled instance" is wrong, and it should be "ADC_1" for the device label as defined in the dts. can be achieved by dynamically creating the subcommand syntax.

For now, this code is malfunctioning in every ST core we used.

[erwango]

@avigreen1978

I'm not familiar with adc. Can you provide commands to reproduce the issue ?
Then it may be naive, but I wonder if the following would solve the mismatch:

In drivers/adc/adc_shell.c

 #define ADC_SHELL_COMMAND(inst) \
-   SHELL_CMD(ADC_##inst, &sub_adc_cmds, "ADC_" #inst, NULL)
+   SHELL_CMD(ADC_##inst, &sub_adc_cmds, DT_INST_LABEL(inst), NULL)

[avigreen1978]

@erwango
The fix you proposed only affects the help string of the shell cmd, not the actual syntax.

Let's assume we only have ADC_1 enabled.
then, the syntax of any command would be:
adc
The 2nd argument is determined by the ADC_SHELL_COMMAND macro's first argument (syntax - which is then stringinfied). This would translate to ADC_0 (inst=0).
This argument is then compared to the DT_INST_LABEL(inst) (determined in the ADC_HDL_LIST_ENTRY macro), which would translate to ADC_1 (DT_INST_LABEL(0)="ADC_1").
This is the strcmp that fails.

To clarify the issue, if we run the following command:
SHELL_CMD(DT_INST_LABEL(inst), &sub_adc_cmds, DT_INST_LABEL(inst), NULL)
The macro would try to STRINGIFY the first argument, which is a string already.
The result of that fix would be that the command syntax would be:
adc "ADC_1"
instead of:
adc ADC_1

[erwango]

@avigreen1978 Txs for feedback, I'll have a new look

Also, can you tell me what sample you're using to reproduce the issue?
It'll be easier to solve if I can exercice the code myself. Thanks!

[avigreen1978]

Any sample (for example, hello_world) in which you set CONFIG_SHELL=y, CONFIG_ADC=y and CONFIG_ADC_SHELL=y would be ok. Of course, you should select a board/create an overlay so that the adc dts node's status is okay.

[erwango]

To clarify the issue, if we run the following command:
SHELL_CMD(DT_INST_LABEL(inst), &sub_adc_cmds, DT_INST_LABEL(inst), NULL)
The macro would try to STRINGIFY the first argument, which is a string already.

I'll have a look, but from what I understand we need a new SHELL DT dedicated command that should not stringify the "_syntax" argument. This might not be sufficient, but this is the first step.

[erwango]

@jakub-uC, @nordic-krch, @mbolivar

I've managed to make this working creating a new shell macro that doesn't stringify _syntax arg:

#if DT_NODE_HAS_STATUS(DT_DRV_INST(0), okay)
    SHELL_EXPR_CMD_ARG_S(1, DT_INST_LABEL(0), &sub_adc_cmds, DT_INST_LABEL(0), NULL, 0, 0),
#endif

With:
#define SHELL_EXPR_CMD_ARG_S(_expr, _syntax, _subcmd, _help, _handler, \
               _mand, _opt) \
    { \
        .syntax = (_expr) ? (const char *)_syntax : "", \
        .help  = (_expr) ? (const char *)_help : NULL, \
        .subcmd = (const struct shell_cmd_entry *)((_expr) ? \
                _subcmd : NULL), \
        .handler = (shell_cmd_handler)((_expr) ? _handler : NULL), \
        .args = { .mandatory = _mand, .optional = _opt} \
    }

So I think this kind of macro, that basically uses _syntax as _help (or vice versa) could be useful for similar situations outside of adc, as it allows the use of DT_INST_LABEL(0) to detect automatically which dt nodes are present and use them directly.

In a second step, this could be even combined with DT API macro to something like:

#define ADC_SHELL_INST(inst)  SHELL_INST(DT_INST_LABEL(int), &sub_adc_cmds),

SHELL_STATIC_SUBCMD_SET_CREATE(
    sub_adc,
        DT_INST_FOREACH_STATUS_OKAY(ADC_SHELL)
    SHELL_SUBCMD_SET_END /* Array terminated. */
);

So my first step would be the brute force approach of defining this new SHELL_EXPR_CMD_ARG_S macro (name tdb) that expect _syntax as a string going through the provision of new set of alternate macros to SHELL_CMD, SHELL_CMD_ARG, ....
Maybe you'd have a smarter way to realize this ?

EDIT: Pinged the wrong nodric- guy, apologies.

[jakub-uC]

@nordic-krch ^

[jakub-uC]

@erwango To be honest we can achieve what is needed with existing macros. It would require to rework existing commands a bit.
ADC channel could be passed as the last argument to each command instead of first. In addition, the ADC channel would need to be a dynamic command so you do not need to know the syntax during compile time.

[erwango]

@erwango To be honest we can achieve what is needed with existing macros. It would require to rework existing commands a bit.
ADC channel could be passed as the last argument to each command instead of first. In addition, the ADC channel would need to be a dynamic command so you do not need to know the syntax during compile time.

I knew there were smarter ways :-), would you have time to do that ?

[jakub-uC]

I can do it but it will not be done in 1-2 days :)

[erwango]

I can do it but it will not be done in 1-2 days :)

No rush (I guess). Is that ok if I assign the point to you ?

[jakub-uC]

sure

[jakub-uC]

@avigreen1978 , @erwango I've started working on my idea. It will be easy to fix the problem but I will need some support from people that are working on Device Tree. I need a function that will return a pointer to an array of strings with active ADC names.
Any idea who can help with that?

[erwango]

@avigreen1978 , @erwango I've started working on my idea. It will be easy to fix the problem but I will need some support from people that are working on Device Tree. I need a function that will return a pointer to an array of strings with active ADC names.
Any idea who can help with that?

From include/devicetree.h, I think you can use DT_INST_FOREACH_STATUS_OKAY and go with something like:

# Need an intermediate macro to allow building a coma separated list
#define NODE_LABELS(n) DT_INST_LABEL(n),

const struct flash_area labels_array[] = {
    DT_INST_FOREACH_STATUS_OKAY(NODE_LABELS)
};

[jakub-uC]

It is a solution but I think I need to assume that all ADCs have the same label ADC_x. Please correct me if I am wrong but in my opinion, if the user will change a label of ADC this approach will not work correctly.

[erwango]

It is a solution but I think I need to assume that all ADCs have the same label ADC_x. Please correct me if I am wrong but in my opinion, if the user will change a label of ADC this approach will not work correctly.

Not sure to get your point, how the user could change an ADC label if not in device tree ?
This method will query the adc node labels as put in dts files.

[erwango]

@avigreen1978 would you mind have a check on https://github.com/zephyrproject-rtos/zephyr/pull/28599 ?