iio: fix string-to-value parsing in iio_parse_value.

cencarna commented 2 months ago

Pull Request Description

Fix wrong parsing for cases when fractional part has leading zeros, when fractional part exceeds subunit limit, and when values to parse are between 0 and -1.

PR Type

[x] Bug fix (change that fixes an issue)
[ ] New feature (change that adds new functionality)
[ ] Breaking change (has dependencies in other repos or will cause CI to fail)

PR Checklist

[x] I have followed the Coding style guidelines
[x] I have performed a self-review of the changes
[x] I have commented my code, at least hard-to-understand parts
[ ] I have build all projects affected by the changes in this PR
[x] I have tested in hardware affected projects, at the relevant boards
[x] I have signed off all commits from this PR
[x] I have updated the documentation (wiki pages, ReadMe etc), if applies

buha commented 2 months ago

looks good but i'm wondering how come we didn't detect it until now ?

i think we should test this more before merging, can you lay out an easy way to reproduce this ? perhaps using the iio_demo project and using the iio_attr command

danmois commented 2 months ago

looks good but i'm wondering how come we didn't detect it until now ?

i think we should test this more before merging, can you lay out an easy way to reproduce this ? perhaps using the iio_demo project and using the iio_attr command

I have tested with the AXI DAC and IIO-Oscilloscope. In this case, when trying to set the frequency to 0,005 or the phase to 0,5 we have correct operation (value saved and read correctly). Can you please let us know how the situation you are correcting occurs?

cencarna commented 2 months ago

This is a bug in iio_parse_value API from no-OS IIO. To reproduce, you may test a sample project on a driver that uses this API with the format IIO_VAL_INT_PLUS_MICRO or IIO_VAL_INT_PLUS_NANO. Example test case is when an attribute is set to 5.005 with format IIO_VAL_INT_PLUS_MICRO, integer and fractional values parsed are set to 5 and 500000, respectively (expected is 5 and 5000). I will later send a forked branch to reproduce this using iio_demo project,

buha commented 2 months ago

I had a look at this and it seems this is also broken and needs fixing: https://github.com/analogdevicesinc/no-OS/blob/main/iio/iio.c#L682-L687

If someone wants to read a negative value like -0.01 in INT_PLUS_MICRO mode, it's represented as 2 int32s, 0 and -10000. The problem is that the sign does not make it to the string and needs special handling, something like:

    case IIO_VAL_INT_PLUS_MICRO:
        return snprintf(buf, len, "%s%"PRIi32".%06"PRIu32"%s", vals[1] < 0 ? "-" : "", vals[0],
                (uint32_t)vals[1], dB ? " dB" : "");

cencarna commented 2 months ago

I had a look at this and it seems this is also broken and needs fixing: https://github.com/analogdevicesinc/no-OS/blob/main/iio/iio.c#L682-L687

If someone wants to read a negative value like -0.01 in INT_PLUS_MICRO mode, it's represented as 2 int32s, 0 and -10000. The problem is that the sign does not make it to the string and needs special handling, something like:
  case IIO_VAL_INT_PLUS_MICRO:
      return snprintf(buf, len, "%s%"PRIi32".%06"PRIu32"%s", vals[1] < 0 ? "-" : "", vals[0],
              (uint32_t)vals[1], dB ? " dB" : "");
Should we add this fix in the PR? I have also reproduced that error using this branch where I modified iio_demo project and related demo drivers.

Some test cases that are seemingly broken are the following:

$iio_attr -u 'serial:COM35,57600,8n1' -d dac_demo dac_global_attr 5.05
5.500000 #expected 5.050000

$iio_attr -u 'serial:COM35,57600,8n1' -d dac_demo dac_global_attr -0.5
0.500000 #expected -0.500000

$iio_attr -u 'serial:COM35,57600,8n1' -d dac_demo dac_global_attr -5.2323232 #value exceeds micro-scale limit
-5.000000 #expected -5.232323 (rounded off or truncated)

buha commented 2 months ago

sorry for delay, i was on holiday, yes, i would say this needs to be fixed in this PR

analogdevicesinc / no-OS