Discussion: Streamline "attr_read" and "attr_write" methods?

[Funky185540]

Hello,

I'm currently seeking to document the 'channel' module, and I wonder whether it might be possible to streamline all "attr_read_" and "attr_write_" calls into one of "attr_read/write" respectively?

I'm just throwing in thoughts here: Upon creating a Channel instance, the library could get all attributes and determine which datatype they have. As channels are always bound to exactly one device, the channel could carry around a HashMap of all attribute entries, along with their data types. Then we could take some T from the user, and cast it to the type needed to set the attributes (or the other way round for reading).

As an added bonus we could check whether an attribute the user attempts to set/get really exists (if it has a matching key in the HashMap), and return a descriptive error if it doesn't, or if the argument can't be converted to the requested attributes type.

I'm aware that it's very debatable (and probably naive!), because we would need to gather this information at channel creation, but I thought I'd put it up for discussion nonetheless.

Or, seeing that libiio exposes 15 functions merely to work with attributes, would it be worthwhile to make structs for the attributes, too? Then each channel could carry around a slice of attributes, where each instance holds it's name (as string), it's data type, and possibly it's current value...

Thoughts? Would this even be possible?

[fpagliughi]

Something to keep in mind is that the IIO has a nicely defined kernel API, but is exposed to user space through some fairly bizarre sysfs interfaces. That makes writing user-space apps fairly difficult, and the beauty of libiio is that it makes a lot of the nonsensical aspects a little more tame.

So the attributes are passed to and from the kernel as strings. Period. All those libiio calls are merely convenience functions converting the different types to and from strings, and then calling the string read or write function. (I didn't realize this when I first wrote the library, otherwise I might have done it differently).

So, in the Rust layer, we could write generic functions to read and write attributes. Anything would work so long as the type could be converted to/from a string in a generic manner (such as having the Display trait for writes), and, of course, that the driver recognizes the string/type.

For reading, we could have an attr_read<T>() that uses the Rust type resolution (implicit or turbofish), like:

let freq: f64  = dev.attr_read("sampling_frequency");

or

let freq = dev.attr_read::<f64>("sampling_frequency");

Going beyond this to try and predict the specific type desired for each attribute is probably not worth it. I did something like this in my Paho MQTT library where each property has a very specific type and must be read or written as that type or the operation fails.
https://github.com/eclipse/paho.mqtt.rust/blob/master/src/properties.rs

That got really messy and I probably need to rewrite the whole thing at some point.
But for IIO, to go to and from strings, that seems easy enough.

[fpagliughi]

OK. I started pushing some code to the develop branch as described above, starting with devices and buffers. Should have it finished (including channels, etc) and cleaned up in a day or so.

[fpagliughi]

It's funny that even a simple generic that only needs to work with a few mostly primitive types (int, float, bool, and string) can still run into complications. In this case, going to and from strings for all the types mostly works - except for bool.

Sysfs uses "0" and "1" for boolean values, whereas Rust uses "false" and "true". Trying to make a special case for bools becomes difficult without partial specialization. I put in a hack for now, but need to come up with something better and more stable.

[Funky185540]

Sorry, I attempted to compile the iio_dummy kernel module for my device, failed and got a bit frustrated about it...

I see that you're using the regular Display trait to convert the types values to strings. What about if we add our own trait that has the "attr_to_string" and "string_to_attr" methods? We could then add a default implementation that handles all cases and do a specialized implementation for the bool type that handles the special case.

I've hacked some code for this, you can find it here. I'd really like to hear your opinion on this!
I had a pretty difficult time getting the trait constraints within the trait right and integrating it into the existing code, but I think it's okay now. I've tested the tsbuf example only so far. I still can't run the tests due to a lack of kernel modules, so I hope that maybe you can do that for me instead... ?

[fpagliughi]

OK. I think I like this idea... But maybe we can clean it up a bit and get rid of some redundancies.

For now I want to keep all the explicit calls like attr_read_bool(), but maybe I can mark them as obsolete to prepare to get rid of them in the following (v0.5?) release. I'm still trying to figure out if there are any weird corner cases that we would miss by doing this, but after reading through the libiio code, I think it's OK to convert to/from a string for these types, because that's all it seems to be doing.

As for implementing your trait:

1. You can just call the trait Attribute. You don't need the IIO. What other kind of trait could it be in this IIO library?
2. Maybe the functions can just be to_string() and from_string() ? Or would that clash with other attributes?
3. With a boolean variables, I prefer not to compare to true or false. (This is an ancient holdover from my embedded C bitmask days, but I cling to it). The bool is already true or false, so just:
   if *self { "1" } else { "0" }
4. In the bool from_string(), you don't need to convert the result .into() since it's already a bool. With to_string() you need the .into() to convert the &str into a String.
5. Don't forget that you need to be able to read or write string attributes.

So, something like:

pub trait Attribute: fmt::Display + FromStr + Sized {
    /// Converts the attribute name and value to CString's that can be sent to
    /// the C library.
    ///
    /// `attr` The name of the attribute
    /// `val` The value to write. This should typically be an int, float, bool,
    ///     or string type.
    fn to_string(&self) -> Result<String> {
        Ok(format!("{}", self))
    }

    fn from_string(s: &str) -> Result<Self> {
        let val = Self::from_str(s).map_err(
            |_| Error::StringConversionError)?;
        Ok(val)
    }
}

impl Attribute for bool {
    fn to_string(&self) -> Result<String> {
        Ok((if *self { "1" } else { "0" }).into())
    }

    fn from_string(s: &str) -> Result<bool> {
        Ok(if s.trim() == "0" { false } else { true })
    }
}

impl Attribute for i64 {}
impl Attribute for f64 {}
impl Attribute for String {}

But, one issue with this is that you can't implement the trait for &str since you can't return the reference in from_string(). That would require you to explicitly convert every outbound &str with a .to_string(), which is a total PIA.

Perhaps what we could do is split the trait into two different ones. One for converting to an attribute string and one for converting from an attribute string. Something like this might be good:

/// Trait to convert a value to a proper attribute string.
pub trait ToAttribute: fmt::Display + Sized {
    /// Converts the attribute name and value to an attribute string that
    /// can be sent to the C library.
    ///
    /// `attr` The name of the attribute
    /// `val` The value to write.
    fn to_attr(&self) -> Result<String> {
        Ok(format!("{}", self))
    }
}

/// Trait to convert an attribute string to a typed value.
pub trait FromAttribute: FromStr + Sized {
    /// Converts a string attribute to a value type.
    fn from_attr(s: &str) -> Result<Self> {
        let val = Self::from_str(s).map_err(
            |_| Error::StringConversionError)?;
        Ok(val)
    }
}

/// Attribute conversion for the bool type.
/// 
/// The bool type needs a special implementation of the attribute conversion
/// trait because it's default Rust string counterparts are "true" and "false"
/// for true and false values respectively. However, sysfs expects these to be
/// "1" or "0".
impl ToAttribute for bool {
    fn to_attr(&self) -> Result<String> {
        Ok((if *self { "1" } else { "0" }).into())
    }
}

impl FromAttribute for bool {
    fn from_attr(s: &str) -> Result<bool> {
        Ok(if s.trim() == "0" { false } else { true })
    }
}

// Default trait implementations for the types in the IIO lib
impl ToAttribute for i64 {}
impl ToAttribute for f64 {}
impl ToAttribute for &str {}
impl ToAttribute for String {}

impl FromAttribute for i64 {}
impl FromAttribute for f64 {}
impl FromAttribute for String {}

This also gets rid of the possible confusion of to_string() and from_string() with the implicit acknowledgement that the underlying attribute is a string.

The final thing I wonder is whether we need the Result<> in ToAttribute. None of our implementations can fail. Maybe just:

pub trait ToAttribute: fmt::Display + Sized {
    fn to_attr(&self) -> String {
        format!("{}", self)
    }
}

When using the traits from a generic T type, you can just use the T to specify the function, like:

pub fn attr_write<T: ToAttribute>(&self, attr: &str, val: T) -> Result<()> {
    let sval = T::to_attr(&val);
    ...

Would you have time to get this implemented in the next day or two? I'm supposed to have the next release out by the end of the week, but at a minimum, I'd like to finish any changes by Friday and then test and debug over the weekend.

[fpagliughi]

OK. I'm in a hurry to get this release out this week. So I went ahead and added it with the separate ToAttribute and FromAttribute.

@Funky185540 Let me know if this works for you.

[Funky185540]

Well things have certainly picked up pace in terms of time between releases now. :-)

First up: Thanks for the feedback! I really appreciate it.

I won't be near my PC for the rest of the day (UTC+1 here), but I'll be glad to have a detailed look tomorrow and give a more thorough reply to your last message! The code looks good on the first glance.

[Funky185540]

As for implementing your trait:

1. You can just call the trait Attribute. You don't need the IIO. What other kind of trait could it be in this IIO library?

Of course. I think I've read too much C-code in my life...

2. Maybe the functions can just be to_string() and from_string() ? Or would that clash with other attributes?

I just named them like this to be compatible with the functions you had written. I don't really insist on this naming. ;-)

3. With a boolean variables, I prefer not to compare to true or false. (This is an ancient holdover from my embedded C bitmask days, but I cling to it). The bool is already true or false, so just:
   if *self { "1" } else { "0" }
4. In the bool from_string(), you don't need to convert the result .into() since it's already a bool. With to_string() you need the .into() to convert the &str into a String.

Okay, good to know. Thanks!

5. Don't forget that you need to be able to read or write string attributes.

Yeah I see what you mean. You'd like to support &str because that's what string constants in the source code are stored as, right? Also as &str is a reference, it doesn't have the FromStr trait, which my implementation requires. So this wouldn't work... Dang...

The final thing I wonder is whether we need the Result<> in ToAttribute. None of our implementations can fail. Maybe just:

I don't know. Can it really not fail? What if the host runs out of memory to allocate another (possibly very long) string, or any other unlikely scenario? Of course I'm not an experienced Rust programmer, but having these functions return a Result feels "right" to me. In any case you're the more seasoned software dev, so it's up to you.

@Funky185540 Let me know if this works for you.

It does! I think it looks pretty and reads very nicely now.

Just another thing:

For now I want to keep all the explicit calls like attr_read_bool(), but maybe I can mark them as obsolete to prepare to get rid of them in the following (v0.5?) release.

Wouldn't this sort of thing (changing the public API) be a breaking change in terms of semver? Shouldn't we bump the major release number then (i.e. make it 1.0.0)? After all I mean users could update their crates simply by using cargo update for minor version upgrades, but they probably wouldn't expect all of the functions to read/write attributes to disappear in that case, would they?

[fpagliughi]

I have some cleanup code that I'm in the middle of finishing. Hopefully up today. (Fixing the unit tests, etc)

The only thing that stinks is that trait also needs to be implemented for every integer type that the app or drivers might want to use (i32, u32, u64, etc). So I added a few more that will get pushed today as well.

Wouldn't this sort of thing (changing the public API) be a breaking change in terms of semver? Shouldn't we bump the major release number then (i.e. make it 1.0.0)? After all I mean users could update their crates simply by using cargo update for minor version upgrades, but they probably wouldn't expect all of the functions to read/write attributes to disappear in that case, would they?

Yes. These are breaking changes. With semver, versions prior to 1.0 (meaning 0.x.y) are considered "pre-release". The meaning of the version numbers shift down a level. So going from 0.4.x to 0.5.0 allows for API breaking changes. By keeping the version 0.x, you indicate to users that the library and API are in active development and will be breaking often as you settle into an API.

Once you move to v 1.0.0, it's an indication that the library has reached a certain maturity and will not be introducing breaking changes very often, if at all. We're certainly not there yet with this library!

Thanks for all the help. It's looking good.

[Funky185540]

The only thing that stinks is that trait also needs to be implemented for every integer type that the app or drivers might want to use (i32, u32, u64, etc). So I added a few more that will get pushed today as well.

Although I think one could call this a luxury problem. The C lib only offers 4 types in total after all. But some luxury certainly won't hurt... :-)

Thanks for clarifying! I'll close this now as the issue pretty much seems done.

[fpagliughi]

The C lib only offers 4 types in total after all.

But the C lib implicitly converts. So it works with all the integer types... although it doesn't report out-of-range errors.

And now I'm debating whether or not to get rid of the individual attribute functions and consolidate. The only thing I'm not sure about is that the floating-point conversions in libiio are platform and locale specific. I haven't tested to see if the Rust library gives the same results in all circumstances, and I don't think I have the capacity to test everything.

[Funky185540]

Ah, yeah I see. Good point.

How would you propose to test this? I may do it and I have some very few sensors here, but I don't think that any of them have float values as IIO attributes... Do you have some in your head that work with float values?