Channel label support #1229

dNechita · 2025-01-16T09:27:17Z

PR Description

Add support for labels in IIO channels, similar to IIO devices.
These changes add iio_channel_get_label(const struct iio_channel *chn) to the existing API.

PR Type

Bug fix (a change that fixes an issue)
New feature (a change that adds new functionality)
Breaking change (a change that affects other repos or cause CIs to fail)

PR Checklist

I have conducted a self-review of my own code changes
I have commented new code, particulary complex or unclear areas
I have checked that I did not intoduced new warnings or errors (CI output)
I have checked that components that use libiio did not get broken
I have updated the documentation accordingly (GitHub Pages, READMEs, etc)

device.c

dNechita · 2025-01-17T13:43:41Z

V2:

fixed 'name' being used instead of 'label' (based on review fidings)
added new commits with updates for iio_info and iio_attr
rebased on latest main branch

nunojsa · 2025-01-17T14:05:36Z

Hi @dNechita,

I plan to give a better review on this one next week. For now, some points from me:

The git title in the first two patches don't follow the typical style. If you're touching lots of files (really because you have too), you can do treewide: ...;
The order of the patches does not look good to me. For example, exposing an API (the first patch) which will do nothing makes no sense to me. What about reversing the order of the first 3 patches?

rgetz · 2025-01-17T19:46:15Z

reversing the order of the patches will cause build failures (won't it?) - since you will be using label, before it's added/initialized.

I can see swapping the order of:

so that local is filled out when you start using them - but it shouldn't really matter too much...

rgetz · 2025-01-17T20:29:01Z

I think the patch set is incomplete/wrong somewhere:

running it I get (on iio_info):

        hwmon18: max15301 (label: VCCPSINT)
                5 channels found:
                        curr1:  (label: iout1) (input)
                        4 channel-specific attributes found:
                                attr  0: crit (curr1_crit) value: 11015
                                attr  1: crit_alarm (curr1_crit_alarm) value: 0
                                attr  2: input (curr1_input) value: 1396
                                attr  3: label (curr1_label) value: iout1
                        temp2:  (input)
                        3 channel-specific attributes found:
                                attr  0: crit (temp2_crit) value: 115000
                                attr  1: crit_alarm (temp2_crit_alarm) value: 0
                                attr  2: input (temp2_input) value: 47125
                        in1:  (label: vin) (input)
                        3 channel-specific attributes found:
                                attr  0: alarm (in1_alarm) value: 0
                                attr  1: input (in1_input) value: 12000
                                attr  2: label (in1_label) value: vin
                        temp1:  (input)
                        3 channel-specific attributes found:
                                attr  0: crit (temp1_crit) value: 115000
                                attr  1: crit_alarm (temp1_crit_alarm) value: 0
                                attr  2: input (temp1_input) value: 43125
                        in2:  (label: vout1) (input)
                        3 channel-specific attributes found:
                                attr  0: alarm (in2_alarm) value: 0
                                attr  1: input (in2_input) value: 849
                                attr  2: label (in2_label) value: vout1

the issue I want to point out is that the lable still shows up as an attribute, when it should be eaten as a label...

We see:

                        curr1:  (label: iout1) (input)
                        4 channel-specific attributes found:
                                attr  0: crit (curr1_crit) value: 11015
                                attr  1: crit_alarm (curr1_crit_alarm) value: 0
                                attr  2: input (curr1_input) value: 1396
------->                        attr  3: label (curr1_label) value: iout1
                        temp2:  (input)

rather than:

                        curr1:  (label: iout1) (input)
                        4 channel-specific attributes found:
                                attr  0: crit (curr1_crit) value: 11015
                                attr  1: crit_alarm (curr1_crit_alarm) value: 0
                                attr  2: input (curr1_input) value: 1396
                        temp2:  (input)

It doesn't make sense to expose the curr_label - now that we have a proper way of doing it...

I think this just means it's missing an early return somewhere in local.c for example, this is what the early return does here:

static int add_attr_to_device(struct iio_device *dev, const char *attr)
{
	unsigned int i;

	for (i = 0; i < ARRAY_SIZE(device_attrs_denylist); i++)
		if (!strcmp(device_attrs_denylist[i], attr))
			return 0;

	if (!strcmp(attr, "name") || !strcmp(attr, "label"))
		return 0;

nunojsa · 2025-01-18T14:32:25Z

reversing the order of the patches will cause build failures (won't it?) - since you will be using label, before it's added/initialized.

I didn't meant literally. Likely, as you pointed out, it needs some rework in order to change the order. But it should be doable to code all the foundation and the last patch to just be exposing the API to the "outside world". As it stands, I do not think it makes to much sense to expose an API without being implemented.

dNechita · 2025-01-20T08:11:21Z

I think the patch set is incomplete/wrong somewhere:

running it I get (on iio_info):

        hwmon18: max15301 (label: VCCPSINT)
                5 channels found:
                        curr1:  (label: iout1) (input)
                        4 channel-specific attributes found:
                                attr  0: crit (curr1_crit) value: 11015
                                attr  1: crit_alarm (curr1_crit_alarm) value: 0
                                attr  2: input (curr1_input) value: 1396
                                attr  3: label (curr1_label) value: iout1
                        temp2:  (input)
                        3 channel-specific attributes found:
                                attr  0: crit (temp2_crit) value: 115000
                                attr  1: crit_alarm (temp2_crit_alarm) value: 0
                                attr  2: input (temp2_input) value: 47125
                        in1:  (label: vin) (input)
                        3 channel-specific attributes found:
                                attr  0: alarm (in1_alarm) value: 0
                                attr  1: input (in1_input) value: 12000
                                attr  2: label (in1_label) value: vin
                        temp1:  (input)
                        3 channel-specific attributes found:
                                attr  0: crit (temp1_crit) value: 115000
                                attr  1: crit_alarm (temp1_crit_alarm) value: 0
                                attr  2: input (temp1_input) value: 43125
                        in2:  (label: vout1) (input)
                        3 channel-specific attributes found:
                                attr  0: alarm (in2_alarm) value: 0
                                attr  1: input (in2_input) value: 849
                                attr  2: label (in2_label) value: vout1

the issue I want to point out is that the lable still shows up as an attribute, when it should be eaten as a label...

We see:

                        curr1:  (label: iout1) (input)
                        4 channel-specific attributes found:
                                attr  0: crit (curr1_crit) value: 11015
                                attr  1: crit_alarm (curr1_crit_alarm) value: 0
                                attr  2: input (curr1_input) value: 1396
------->                        attr  3: label (curr1_label) value: iout1
                        temp2:  (input)

rather than:

                        curr1:  (label: iout1) (input)
                        4 channel-specific attributes found:
                                attr  0: crit (curr1_crit) value: 11015
                                attr  1: crit_alarm (curr1_crit_alarm) value: 0
                                attr  2: input (curr1_input) value: 1396
                        temp2:  (input)

It doesn't make sense to expose the curr_label - now that we have a proper way of doing it...

I think this just means it's missing an early return somewhere in local.c for example, this is what the early return does here:

static int add_attr_to_device(struct iio_device *dev, const char *attr)
{
	unsigned int i;

	for (i = 0; i < ARRAY_SIZE(device_attrs_denylist); i++)
		if (!strcmp(device_attrs_denylist[i], attr))
			return 0;

	if (!strcmp(attr, "name") || !strcmp(attr, "label"))
		return 0;

Yes, I have missed adding the implementation that would consider the label not to be a simple attribute. I will include it. Thanks!

dNechita · 2025-01-20T09:06:21Z

reversing the order of the patches will cause build failures (won't it?) - since you will be using label, before it's added/initialized.

I didn't meant literally. Likely, as you pointed out, it needs some rework in order to change the order. But it should be doable to code all the foundation and the last patch to just be exposing the API to the "outside world". As it stands, I do not think it makes to much sense to expose an API without being implemented.

Regarding the order of the commits, this time I just used the order presented in issue that was requesting the feature. But if it wasn't for that order I would have still started with the end in mind, meaning adding API, and even updating the clients of the API and then going for the implementation. This is more of an order of how I would develop, and I can keep it while changing the commits order when creating a PR if that is easier to review.

nunojsa · 2025-01-20T10:55:53Z

This is more of an order of how I would develop, and I can keep it while changing the commits order when creating a PR if that is easier to review.

It's not it is easier to review. It just does not make sense to me to have a public API without being implemented. Naturally you're thinking on just having the complete PR merged and then this does not matter... I'm more looking at the series on a patch by patch basis and what makes sense to me. Sometimes, one also should have in mind things like git bisect for trying to figure what patch introduced some bug and so we should make sure that a specific patch won't willingly break something.

All the above said, this is a nitpick and a minor detail

dNechita · 2025-01-20T15:06:10Z

V3:

Reorder the commits such that the implementation goes in the 1st commit
Reworded messages of commits that touch multiple files by adding 'treewide:' prefix
added the commit for the C# bindings

include/iio/iio.h

local.c

utils/iio_attr.c

rgetz · 2025-01-20T22:43:39Z

I think the iio_info issue is fixed...

       hwmon2: dell_smm
                6 channels found:
                        fan1:  (label: Processor Fan) (input)        <- looks cool
                        1 channel-specific attributes found:
                                attr  0: input (fan1_input) value: 1009

and main branch:

rgetz@brain:~/github/libiio/build$ sudo ./utils/iio_attr -u local: -c hwmon2 fan1
dev 'dell_smm', channel 'fan1' (input), attr 'input', value '1008'
dev 'dell_smm', channel 'fan1' (input), attr 'label', value 'Processor Fan'

turns into:

rgetz@brain:~/github/libiio/build$ sudo ./utils/iio_attr -u local: -c hwmon2 fan1
dev 'dell_smm', channel 'fan1' (input), label 'Processor Fan', attr 'input', value '1010'

which I think is what we are looking for...

:)

local.c

dNechita · 2025-01-21T11:00:46Z

V4:

Use 'label' instead of 'name' in the description of iio_channel_get_label() in iio.h
Use "_label" instead of variable for better readability in local.c
Add new code without touching existing code in iio_attr.c

rgetz · 2025-01-21T12:37:47Z

Looks good to me - check on the issues Codacy is pointing out, and decide which ones you want to ignore/fix.

dNechita · 2025-01-21T15:05:55Z

V5:

add error message if it fails to read channel label in local.c

rgetz

looks good to me. should tag this with #1222 to keep things in sync.

rgetz · 2025-01-21T21:14:43Z

index 1500c39d..29c14e58 100644
--- a/examples/iio-monitor.c
+++ b/examples/iio-monitor.c
@@ -165,7 +165,7 @@ static void * read_thd(void *d)

        while (!stop) {
                struct iio_device *dev;
-               const char *name;
+               const char *name, *label;
                int row, col, len, align, line = 2;
                unsigned int i, nb_channels, nb = 0;
                char buf[1024];
@@ -208,9 +208,12 @@ static void * read_thd(void *d)

                        nb++;
                        name = iio_channel_get_name(chn);
+                       label = iio_channel_get_label(chn);
                        id = iio_channel_get_id(chn);
                        if (!name)
                                name = id;
+                       if (label)
+                               name = label;
                        unit = id_to_unit(id);

also helps

dNechita · 2025-01-22T09:58:55Z

if (label)

Will add. Thanks for the code snippet.

nunojsa · 2025-01-22T08:21:09Z

local.c

+	const char *dev_id;
+
+	if (strlen(name) >= lbl_len &&
+		!strcmp(name + strlen(name) - lbl_len, "_label")) {


I'm still not convinced the above is not just doing something get_short_attr_name() is already doing... You did not replied to my last comment but unless I'm missing something, if the channel name is in_voltage0_label, that function should return "label" and you can avoid all that pointer arithmetic. If you look at the implementation the whole point of the function is to give the channel short name, meaning, the name after the last _. Is there any other subtle thing I'm missing?

I forgot to address your concern.

With these changes:

diff --git a/local.c b/local.c index 2b28912c..28fe25b7 100644 --- a/local.c +++ b/local.c @@ -851,6 +851,8 @@ static int add_attr_to_channel(struct iio_channel *chn, } else { chn_perror(chn, ret, "Unable to read channel label"); } + fprintf(stderr, "initial_name: %s\n", name); + fprintf(stderr, "name_after_get_short: %s\n", get_short_attr_name(chn, name)); return 0; }

I am getting the following:

analog@analog:~/workspace-dan/libiio/build_v1 $ utils/iio_info
initial_name: out_altvoltage1_TX1_I_F2_label
name_after_get_short: TX1_I_F2_label
initial_name: out_altvoltage0_TX1_I_F1_label
name_after_get_short: TX1_I_F1_label
initial_name: out_altvoltage7_TX2_Q_F2_label
name_after_get_short: TX2_Q_F2_label
initial_name: out_altvoltage6_TX2_Q_F1_label
name_after_get_short: TX2_Q_F1_label
initial_name: out_altvoltage3_TX1_Q_F2_label
name_after_get_short: TX1_Q_F2_label
initial_name: out_altvoltage2_TX1_Q_F1_label
name_after_get_short: TX1_Q_F1_label
initial_name: out_altvoltage5_TX2_I_F2_label
name_after_get_short: TX2_I_F2_label
initial_name: out_altvoltage4_TX2_I_F1_label
name_after_get_short: TX2_I_F1_label
initial_name: out_altvoltage1_TX_LO_label
name_after_get_short: TX_LO_label
initial_name: out_altvoltage0_RX_LO_label
name_after_get_short: RX_LO_label
initial_name: in_voltage7_vrefn_label
name_after_get_short: vrefn_label
initial_name: in_voltage1_vccaux_label
name_after_get_short: vccaux_label
initial_name: in_voltage2_vccbram_label
name_after_get_short: vccbram_label
initial_name: in_voltage3_vccpint_label
name_after_get_short: vccpint_label
initial_name: in_voltage0_vccint_label
name_after_get_short: vccint_label
initial_name: in_voltage6_vrefp_label
name_after_get_short: vrefp_label
initial_name: in_voltage5_vccoddr_label
name_after_get_short: vccoddr_label
initial_name: in_voltage4_vccpaux_label
name_after_get_short: vccpaux_label

Which seems that it strips away the input/output information and the channel id.

Yeah, I was now just getting more familiar with libiio code for populating channels and attrs. Indeed for legacy devices which make use of extended_names (which are now exposed as labels) the short name function won't work.

out_altvoltage1_TX1_I_F2_label - TX1_I_F2 is the extended name and will be seen as the channel name by libiio. This stuff seems to be addressed later on in set_channel_name() where we set the proper attr name. So, even if we handle things later and not in add_attr_to_channel(), not sure it would be simpler. So yeah, looks good as-is

nunojsa

LGTM, just some minor nits...

nunojsa · 2025-01-23T07:50:41Z

local.c

+			label, sizeof(label), IIO_ATTR_TYPE_CHANNEL);
+		if (ret > 0) {
+			chn_label_ptr = label;
+			chn->label = iio_strdup(chn_label_ptr);


can this simply be chn->label = iio_strdup(label);?

it can be. Thanks!

nunojsa · 2025-01-23T07:51:42Z

device.c

@@ -139,6 +139,7 @@ struct iio_channel * iio_device_find_channel(const struct iio_device *dev,
 			continue;

 		if (!strcmp(chn->id, name) ||
+				(chn->label && !strcmp(chn->label, name)) ||


nothing wrong with the patch but commit subject does not match the style :)

do you mean that there is an extra tab on this line?

no.. the commit title itself. nothing wrong with the code. It's a nitpick but consistency across git log is a good thing. Also good for grepping the history

Oh, I got it now. I need to grab another coffee.

When the "label" channel attribute is found, initialize the channel's label to its value. Signed-off-by: Dan Nechita <dan.nechita@analog.com>

This function can be used to retrieve the label for this IIO Channel, if it was specified in linux driver. Signed-off-by: Dan Nechita <dan.nechita@analog.com>

Make iio_device_find_channel() able to search not only by ID and name, but also by label. Signed-off-by: Dan Nechita <dan.nechita@analog.com>

Signed-off-by: Dan Nechita <dan.nechita@analog.com>

If a IIO channel has a label, display it after the name. Signed-off-by: Dan Nechita <dan.nechita@analog.com>

If a IIO channel has a label, display it and support matching against the label. Signed-off-by: Dan Nechita <dan.nechita@analog.com>

Signed-off-by: Dan Nechita <dan.nechita@analog.com>

dNechita · 2025-01-23T08:42:36Z

V6:

Simplify code by using the label array and no longer use a pointer that was pointing to the same array
Update the 3rd commit title to preserve the style (add hint of the files being affected by change)

mhennerich reviewed Jan 17, 2025

View reviewed changes

device.c Outdated Show resolved Hide resolved

dNechita force-pushed the channel_label_support branch from 603820a to 3fb2071 Compare January 17, 2025 13:40

dNechita marked this pull request as ready for review January 17, 2025 13:44

dNechita requested a review from cristina-suteu as a code owner January 17, 2025 13:44

dNechita force-pushed the channel_label_support branch from 3fb2071 to 384d318 Compare January 20, 2025 15:00

rgetz reviewed Jan 20, 2025

View reviewed changes

include/iio/iio.h Outdated Show resolved Hide resolved

local.c Outdated Show resolved Hide resolved

utils/iio_attr.c Outdated Show resolved Hide resolved

nunojsa reviewed Jan 21, 2025

View reviewed changes

local.c Show resolved Hide resolved

dNechita force-pushed the channel_label_support branch from 384d318 to 8abf207 Compare January 21, 2025 09:22

dNechita force-pushed the channel_label_support branch from 8abf207 to 4acaafe Compare January 21, 2025 15:04

rgetz approved these changes Jan 21, 2025

View reviewed changes

dNechita force-pushed the channel_label_support branch from 4acaafe to 066d1ef Compare January 22, 2025 10:23

nunojsa reviewed Jan 22, 2025

View reviewed changes

nunojsa reviewed Jan 23, 2025

View reviewed changes

dNechita added 2 commits January 23, 2025 10:37

treewide: Init channel label when found

443d8a8

When the "label" channel attribute is found, initialize the channel's label to its value. Signed-off-by: Dan Nechita <dan.nechita@analog.com>

treewide: Add API function iio_channel_get_label()

978eb30

This function can be used to retrieve the label for this IIO Channel, if it was specified in linux driver. Signed-off-by: Dan Nechita <dan.nechita@analog.com>

dNechita added 7 commits January 23, 2025 10:37

treewide: Enhance iio_device_find_channel() to use labels

7a884a3

Make iio_device_find_channel() able to search not only by ID and name, but also by label. Signed-off-by: Dan Nechita <dan.nechita@analog.com>

c++: Add channel method to get label

b7e2678

Signed-off-by: Dan Nechita <dan.nechita@analog.com>

c#: Add channel method to get label

da5a2b1

Signed-off-by: Dan Nechita <dan.nechita@analog.com>

python: Add channel method to get label

d7bf42a

Signed-off-by: Dan Nechita <dan.nechita@analog.com>

iio_info: Display channel label if present

eb366f2

If a IIO channel has a label, display it after the name. Signed-off-by: Dan Nechita <dan.nechita@analog.com>

iio_attr: Display channel label if present

a236dc1

If a IIO channel has a label, display it and support matching against the label. Signed-off-by: Dan Nechita <dan.nechita@analog.com>

iio-monitor: Display the channel label if it exists

d27b264

Signed-off-by: Dan Nechita <dan.nechita@analog.com>

dNechita force-pushed the channel_label_support branch from 066d1ef to d27b264 Compare January 23, 2025 08:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Channel label support #1229

Channel label support #1229

dNechita commented Jan 16, 2025 •

edited

Loading

dNechita commented Jan 17, 2025

nunojsa commented Jan 17, 2025

rgetz commented Jan 17, 2025

rgetz commented Jan 17, 2025

nunojsa commented Jan 18, 2025

dNechita commented Jan 20, 2025

dNechita commented Jan 20, 2025

nunojsa commented Jan 20, 2025

dNechita commented Jan 20, 2025

rgetz commented Jan 20, 2025

dNechita commented Jan 21, 2025

rgetz commented Jan 21, 2025

dNechita commented Jan 21, 2025

rgetz left a comment •

edited

Loading

rgetz commented Jan 21, 2025

dNechita commented Jan 22, 2025

nunojsa Jan 22, 2025

dNechita Jan 22, 2025

nunojsa Jan 22, 2025

nunojsa left a comment

nunojsa Jan 23, 2025

dNechita Jan 23, 2025

nunojsa Jan 23, 2025

dNechita Jan 23, 2025

nunojsa Jan 23, 2025

dNechita Jan 23, 2025

dNechita commented Jan 23, 2025

Channel label support #1229

Are you sure you want to change the base?

Channel label support #1229

Conversation

dNechita commented Jan 16, 2025 • edited Loading

PR Description

PR Type

PR Checklist

dNechita commented Jan 17, 2025

nunojsa commented Jan 17, 2025

rgetz commented Jan 17, 2025

rgetz commented Jan 17, 2025

nunojsa commented Jan 18, 2025

dNechita commented Jan 20, 2025

dNechita commented Jan 20, 2025

nunojsa commented Jan 20, 2025

dNechita commented Jan 20, 2025

rgetz commented Jan 20, 2025

dNechita commented Jan 21, 2025

rgetz commented Jan 21, 2025

dNechita commented Jan 21, 2025

rgetz left a comment • edited Loading

Choose a reason for hiding this comment

rgetz commented Jan 21, 2025

dNechita commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nunojsa 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

dNechita commented Jan 23, 2025

dNechita commented Jan 16, 2025 •

edited

Loading

rgetz left a comment •

edited

Loading