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...

:)

When the "label" channel attribute is found, initialize the channel's label to its value. Signed-off-by: Dan Nechita <[email protected]>

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 <[email protected]>

Make iio_device_find_channel() able to search not only by ID and name, but also by label. Signed-off-by: Dan Nechita <[email protected]>

Signed-off-by: Dan Nechita <[email protected]>

If a IIO channel has a label, display it after the name. Signed-off-by: Dan Nechita <[email protected]>

nunojsa · 2025-01-21T09:17:58Z

local.c

+			chn->label = iio_strdup(chn_label_ptr);
+		}
+		return 0;
+	}


To add on Robin's comments I wonder if we could not do this simpler:

name = get_short_attr_name(chn, name); if (!strcmp(name, "label") { const char *dev_id = iio_device_get_id(iio_channel_get_device(chn)); ret = local_do_read_dev_attr(dev_id, 0, name, label, sizeof(label), IIO_ATTR_TYPE_CHANNEL); /* But I actually wonder if we should ignore errors in this case?!! */ if (ret > 0) { ... return 0; } }

By simpler, do you mean doing the entire if() after name = get_short_attr_name(chn, name);? The thing is that name will never be equal to "label" since at this point, the channel name isn't known yet, therefore name will still resemble to something like: "in_voltage0_label".

For the other thing, I could add an error message for situations where label reading fails but I am not sure what else can be done. We will end up with a channel with no label and no attribute called 'label' and that is it. Are you considering doing more in this scenario?

If a IIO channel has a label, display it and support matching against the label. Signed-off-by: Dan Nechita <[email protected]>

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

dNechita added 7 commits January 21, 2025 11:17

treewide: Init channel label when found

1a31875

When the "label" channel attribute is found, initialize the channel's label to its value. Signed-off-by: Dan Nechita <[email protected]>

treewide: Add API function iio_channel_get_label()

c012cb9

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 <[email protected]>

Enhance iio_device_find_channel() to use labels

0fdefc3

Make iio_device_find_channel() able to search not only by ID and name, but also by label. Signed-off-by: Dan Nechita <[email protected]>

c++: Add channel method to get label

7896d88

Signed-off-by: Dan Nechita <[email protected]>

C#: Add channel method to get label

21ddf0c

Signed-off-by: Dan Nechita <[email protected]>

python: Add channel method to get label

708db12

Signed-off-by: Dan Nechita <[email protected]>

iio_info: Display channel label if present

78280f7

If a IIO channel has a label, display it after the name. Signed-off-by: Dan Nechita <[email protected]>

nunojsa reviewed Jan 21, 2025

View reviewed changes

iio_attr: Display channel label if present

8abf207

If a IIO channel has a label, display it and support matching against the label. Signed-off-by: Dan Nechita <[email protected]>

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

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

nunojsa Jan 21, 2025

dNechita Jan 21, 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

nunojsa Jan 21, 2025

Choose a reason for hiding this comment

dNechita Jan 21, 2025

Choose a reason for hiding this comment

dNechita commented Jan 16, 2025 •

edited

Loading