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

AnalogIn test correction #7

Closed
wants to merge 1 commit into from
Closed

AnalogIn test correction #7

wants to merge 1 commit into from

Conversation

bcostm
Copy link
Contributor

@bcostm bcostm commented Oct 17, 2016

Change initial value of prev_value + do only one adc read.

Correction of Issue ARMmbed/mbed-os#2956

@BlackstoneEngineering
Copy link
Contributor

BlackstoneEngineering commented Oct 17, 2016

@bcostm Wouldnt the simpler fix be just to initialize outputs and y to 1?

@pan-
Copy link
Member

pan- commented Oct 19, 2016

@BlackstoneEngineering I think the algorithm is done in the wrong order, it should be:

  • Read the current voltage
  • Enable one more pin
  • Assert that the voltage is now more important than the previous value read.

it doesn't makes sense to assert that something is different if nothing has changed yet.

    for(x = 0; x<5; x++) {
        prev_value = ain.read();
        y = (y<<1) + 1;
        outputs = y;
        TEST_ASSERT_MESSAGE(ain.read() > prev_value,"Analog Input did not incriment. Check that you have assigned valid pins in mbed_app.json file")
    }

@BlackstoneEngineering
Copy link
Contributor

@pan- Agreed.
@bcostm does vincents solution work for you on your platform? Will merge Vincents solution on Friday if I do not hear back from you before then

@bcostm
Copy link
Contributor Author

bcostm commented Oct 20, 2016

Hi,
Yes I agree. Let me check and I come back to you.
So, yes this method works. I tested it on Nucleo_F429ZI only but I am confident...
How do you want to proceed ? I close this PR or I update it ?

@BlackstoneEngineering
Copy link
Contributor

I will close the issue and update directly to master, i've already got a couple patches pending, I'll toss in this one line change.

Thanks all for the input and helping us build a better ecosystem!

BlackstoneEngineering added a commit that referenced this pull request Oct 20, 2016
@0xc0170
Copy link
Collaborator

0xc0170 commented Nov 16, 2016

@BlackstoneEngineering Whats the status of this PR? Can it be closed?

@BlackstoneEngineering
Copy link
Contributor

@0xc0170 yeah, the changes have been mainlined already

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.

None yet

4 participants