channel type: Improve code readability for channel type

[RubenNatanael]

PR Description

Currently, the channel type is stored as enum iio_chan_type, even when it represents a hwmon_chan_type. By changing this field to int, we improve code readability and flexibility. This change also enables the implementation of validation checks, such as in hwmon_channel_get_type(), which now issues a warning if a hwmon function is incorrectly called for a non-hwmon channel and vice versa.

• Issue: Incorrect mapping from hwmon sensor type into iio sensor type #1401

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, particularly complex or unclear areas
• I have checked that I did not introduce 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)

[nunojsa]

I'm not sure I agree with this change, It seems like unneeded churn to me. It is fair to assume that users will call hwmon_channel_get_type() on an hwmon device. If not, they need to fix their app anyways! So I would either:

1. Leave as-is;
2. Or be strict about it. Meaning, return an error in case we try to get an IIO channel from an hwmon device or vice-versa.

Honestly I would probably go with 1. For 2, we might need to change the API a bit...

Having said the above, we should have introduced different types for hwmon devices. Yeah, it would have been more complicated but we would not have these discussions.

[nunojsa]

well, we give a warning but prevent nothing... IOW, we continue to do as before.

[RubenNatanael]

I agree. Like I said in the issue, this doesn't stop the error from happening, it's just a 'hint' for developers who assume that the member enum iio_chan_type type in struct iio_channel can't be an hwmon_chan_type. That said, I'm not entirely sure about this change either, so I'd like to hear other opinions.

[dNechita]

well, we give a warning but prevent nothing... IOW, we continue to do as before.

Yes, we continue as before which aligns with your suggestion: "1. Leave as-is;"

Myself and @RubenNatanael looked at this from multiple angles and also came to the same conclusion as you and @rgetz that this isn't an issue, that there isn't an incorrect mapping taking place anywhere. Well, that not entirely true, because the only actor doing this mapping the debugger. So, we said, since we got to this point let's just fix that. Additionally, we thought to warn when hwmon_channel_get_type() and iio_channel_get_type() aren't used properly. But I see that at the same time we've updated the documentation asking users to make sure they are using the proper iio device by calling iio_device_is_hwmon(). But we're also calling it inside hwmon_channel_get_type() and iio_channel_get_type(). Maybe we just stick with updating the documentation and not issue the warnings.
But I don't see any harm in fixing the storage type for channel information.

[rgetz]

I'm not sure I agree with this change, It seems like unneeded churn to me.

Ack and agree - it's not only unneeded - but also will break things. We want to have people treat hwmon devices as IIO devices. a Voltage is a voltage. It does not matter if the backing device is implemented in the hwmon subsystem or the iio one.

[damijans]

Hi all and thanks for all responses. I didn't expect the #1401 will lead to so quick reactions. It doesn't happen so often in Open Source community.

I believe we all agree that actually there is nothing wrong in the current implementation, if you use it correctly it works as expected. The only problem is about confusion faced to developers who are based on reading the code and not documentation. And the fact is that the code is the best documentation. Unfortunately, if you see iio_chan_type type in iio_channel struct (yes, I know it is private structure) then you simply mean that it represent iio channel type and not hwmon type. And we know these two types are not compatible. It true the voltage is a voltage (as mentioned above) and temperature is temperature, but there exist types which can not be simply converted

The fact is that in existing implementation developer is confused when seeing iio_chan_type type attribute which semantically in certain cases means something else. Proposed change in this thread (change attribute into opaque integer) leads developer to think about "hey, this type is opaque, I should do something about that". Proposed change about having a warning if incorrect function is used is also good direction of using the library properly. The alternative, to make the code more self explicable, could also be usage of union instead of integer.

Anyway, whatever decision will be made, I am glad of having such quick responds, thank you.

[dNechita]

it's not only unneeded - but also will break things.

What breaks with this change?
This PR modifies the underlying storage type for channel information. Previously, we used an enum iio_chan_type variable to store values that could actually be from either enum iio_chan_type or enum hwmon_chan_type. Now, we use an int to properly accommodate both enum types.

For end users (libiio clients), nothing changes. No change in functionality or API.
For developers, the debugging experience changes. When inspecting struct iio_channel member type in a debugger, a plain integer value will be seen instead of enum symbolic names. This is a tradeoff:
- downside: less readable - the convenience of seeing symbolic enum names gets lost
- upside: more accurate - avoiding seeing misleading enum names when the value is actually from enum hwmon_chan_type

[rgetz]

The kernel uses a enum. Libiio should remain sync'ed with the kernel.

The patch weakens the type system by changing struct iio_channel::type from enum iio_chan_type to int. That removes compile-time guarantees and forces additional casts elsewhere (as already visible in events.c).

As you point out - the "problem" seems to be a debugger issue. From the discussion:

the only actor doing this mapping the debugger

That means the motivation is essentially:

• debugger pretty-printing confusion

Changing internal types to accommodate debugger output is generally a bad tradeoff. As also pointed out - Documentation or debugger scripts would be the correct fix.

If you really want to fix it - use a tagged union. (but unsure if the complexity is worth it)...

you add two new enums:

enum channel_domain {
    CHANNEL_DOMAIN_IIO,
    CHANNEL_DOMAIN_HWMON,
};

struct channel_type {
    enum channel_domain domain;

    union {
        enum iio_chan_type iio;
        enum hwmon_chan_type hwmon;
    };
};

and then in

struct iio_channel {
    ...
    struct channel_type type;
};

and then you get to do:

chn->type.iio
chn->type.hwmon

The compiler will catch misuse.

enum hwmon_chan_type hwmon_channel_get_type(const struct iio_channel *chn)
{
    if (chn->type.domain != CHANNEL_DOMAIN_HWMON)
        return -EIO;

    return chn->type.hwmon;
}

but to go back to:

What breaks with this change?

Type checking. Now anything can be stored:

type = 4200
type = -7
type = hwmon enum
type = garbage

the patch removes compile-time checking, and forces people to debug it run time.

[dNechita]

What breaks with this change?

Type checking. Now anything can be stored:

That's a very good point!
The complexity outweighs the benefits.

This PR could be simplified to just improving the documentation for hwmon_channel_get_type() and iio_channel_get_device() by clarifying that callers must use them only on channels belonging to the appropriate devices.