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

attribute_multiple_values and tol parameter typing #608

Closed
threexc opened this issue Oct 18, 2024 · 3 comments
Closed

attribute_multiple_values and tol parameter typing #608

threexc opened this issue Oct 18, 2024 · 3 comments

Comments

@threexc
Copy link
Contributor

threexc commented Oct 18, 2024

Hi @tfcollins,

I'm currently working on AD7625 support in pyadi-iio (the driver isn't quite merged upstream yet, but it will be soon). One of the planned tests in test/test_ad7625.py is to test setting the sampling_frequency attribute to multiple different values. The problem I'm running into while testing with actual hardware is that, due to hardware limitations, there is some rounding that occurs, and the resulting values for sampling_frequency may exceed or fall under the expected ones by different percentages.

For example, with a test like:

#########################################                                                                                                                                                                                                                     
@pytest.mark.iio_hardware(hardware)                                                                                                                                                                                                                           
@pytest.mark.parametrize("classname", [(classname)])                                                                                                                                                                                                          
@pytest.mark.parametrize(                                                                                                                                                                                                                                     
    "attr, val",                                                                                                                                                                                                                                              
    [                                                                                                                                                                                                                                                         
        (                                                                                                                                                                                                                                                     
            "sampling_frequency",                                                                                                                                                                                                                             
            [10000, 50000, 100000, 200000, 500000, 1000000, 2000000, 6000000],                                                                                                                                                                                
        ),                                                                                                                                                                                                                                                    
    ],                                                                                                                                                                                                                                                        
)                                                                                                                                                                                                                                                             
def test_ad7625_attr(test_attribute_multiple_values, iio_uri, classname, attr, val):                                                                                                                                                                          
    test_attribute_multiple_values(iio_uri, classname, attr, val, 1)

I see:

Failed to set: sampling_frequency
Set: 50000
Got: 50005.0

If I boost the tol value I can make this go away, but later values in the list need higher tolerances, and since this parameter is a static integer there comes a point where the set tolerance value is actually larger than the target value, meaning it'll never actually fail. For comparison, here's the expected versus rounded value at the top end:

Failed to set: sampling_frequency
Set: 6000000
Got: 5952381.0

I can set a tol value of around 50000 to make this one work, but obviously that makes most of the other values guaranteed to pass the test even if the actual value deviates significantly from the expected.

Where I am going with this: should the API for attribute_multiple_values() (and the dev_interface() function underneath) be rewritten to test with a percentage, rather than a flat integer? Maybe we need to add a variant that takes a percentage instead of an integer (absolute value)?

For now, I am thinking that I may have to write some separate tests for max/min sampling rates with separate tolerance values.

@threexc
Copy link
Contributor Author

threexc commented Oct 18, 2024

Worked around this in #609 by following the example of https://github.com/analogdevicesinc/pyadi-iio/blob/main/test/test_ltc2387.py and adjusting the test values for sampling_frequency, but the question remains if maybe there should be a percentage tolerance, or if we should just rely on different test functions.

@tfcollins
Copy link
Collaborator

tfcollins commented Oct 18, 2024

If you need a unique value for each sample rate that can be done by using the parameterize marks differently. Where you generate use case for each sample rate you want like so:

sampling_frequencies = range(1000000, 15000000, 1000000)
percent_tol = 5
repeats = 1
tols = [int(x * percent_tol / 100) for x in sampling_frequencies]
cases = [
    ("sampling_frequency", rate, rate + 1, 1, tol, repeats)
    for rate, tol in zip(sampling_frequencies, tols)
]
@pytest.mark.parametrize(
    "attr, start, stop, step, tol, repeats",
    cases,
)
def test_my_attr(
    attr,
    start,
    stop,
    step,
    tol,
    repeats,
):
    print(attr, start, stop, step, tol, repeats)

Alternatively you will need to enhance the tests to support a percentage

@threexc
Copy link
Contributor Author

threexc commented Oct 24, 2024

Thank you. I'll leave the PR I've opened as-is for now, but if the review determines that the test there could be improved I'll try that approach instead.

@threexc threexc closed this as completed Oct 24, 2024
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

No branches or pull requests

2 participants