Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cencarna
Copy link
Contributor

@cencarna cencarna commented Sep 3, 2024

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

  • 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

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

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.

Signed-off-by: Cedric Encarnacion <cedricjustine.encarnacion@analog.com>
@buha
Copy link
Contributor

buha commented Sep 9, 2024

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
Copy link
Contributor

danmois commented Sep 9, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

buha commented Sep 19, 2024

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
Copy link
Contributor Author

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
Copy link
Contributor

buha commented Oct 1, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants