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

Adding Sharp GP2Y0A21F_5V_DS #17

Merged
merged 5 commits into from
Sep 26, 2020
Merged

Adding Sharp GP2Y0A21F_5V_DS #17

merged 5 commits into from
Sep 26, 2020

Conversation

cgo
Copy link

@cgo cgo commented Sep 13, 2020

I have added values for the sensor that I found in my junk box. The values are from a power function fit that I did
by reading the values roughly from the plot provided in the datasheet. There is unfortunately no actual data.
The readings are not super accurate, but good enough for my purposes.

@DrGFreeman
Copy link
Owner

Hey @cgo, thanks for taking the time to make this contribution so others can benefit from it!

The code changes look good to me and the CI checks passed on all boards.

Do you have the "advertised" range (in cm) of the sensor and would you mind also updating the information on the README (list of models, coefficients table and analog range table)?

Thanks!

@cgo
Copy link
Author

cgo commented Sep 16, 2020

Hey @DrGFreeman, wow you're running a CI for this; nice!
I believe the range is said to be 10-80cm, but I would not bet my life on the output of that sensor. Using my hand, it seems more or less accurate up to a few cm (up to the calibration that I did by reading values off the graph in the datasheet). The further away, the larger the error seems to be, which is not surprising. The analog range in the code I set to [0,1023]; that limits the read voltage, right? I didn't want to limit it because of the apparent inaccuracies. The lower voltage limit in the datasheet says 0.4V, I think, which would be 409 or 410 in the codomain of the readAnalog function.

I also noticed that the values for the other sensors in the README are given in mm; the output for the one I added is in cm. Given the apparent accuracy, I think that is more than enough; do you want all of them to be in mm?

This is using cm for the added sensor model.
@cgo
Copy link
Author

cgo commented Sep 16, 2020

Looks like my emacs has cleaned up some whitespace -- if that's bothersome, I can undo that too.

@DrGFreeman
Copy link
Owner

Hi @cgo, thanks for the update of the README. No problem with the whitespaces, it's cleaner like this actually.

wow you're running a CI for this; nice!

Not much merit on my part, I just copied what Pololu are doing for their Arduino library. Basically checks that the example sketches compile OK, no more but useful nevertheless.

I believe the range is said to be 10-80cm, but I would not bet my life on the output of that sensor.

Agree, this is mostly for the list of models in the README to help people identify which sensor they have as it's easy to get mixed up in the Sharp model numbers. The range in cm can help confirm the model is the right one. I see you added it in the README, that's perfect. Thanks.

The analog range in the code I set to [0,1023]; that limits the read voltage, right? I didn't want to limit it because of the apparent inaccuracies.

That's fine, especially with a power function, extrapolation is not as bad as with a polynomial which can give unexpected results outside of the calibrated range.

I also noticed that the values for the other sensors in the README are given in mm; the output for the one I added is in cm.

I would prefer the output to be in mm for consistency with the other models (the Library Reference says the output of the getDist() method is in mm) as well as to provide better resolution (resolution != accuracy) since the output is of integer type. This, along with the median filter included in the library, can provide smoother output which in turn can help control systems like PID controllers provide a smoother response when using the distance output as input.

@DrGFreeman
Copy link
Owner

Also, I forgot to ask if you could add the model in the keywords.txt with the LITERAL1 value (tab delimited). This allows the model name to highlight in the Arduino IDE.

Thanks again!

@DrGFreeman DrGFreeman mentioned this pull request Sep 17, 2020
6 tasks
@DrGFreeman DrGFreeman added this to the v1.6.0 milestone Sep 17, 2020
@cgo
Copy link
Author

cgo commented Sep 20, 2020

I think I addressed all issues, let me know if there's something else or feel free to make modifications :)

Restore double trailing whitespaces (markdown line breaks) to maintain original formatting.
Tab separator is required by Arduino IDE.
@DrGFreeman
Copy link
Owner

I think I addressed all issues, let me know if there's something else or feel free to make modifications :)

Thanks @cgo. I made minor modifications to maintain the line breaks in the README file and tab spacing in keywords.txt.

This is ready to merge. Thanks again for your contribution!

I will proceed to make a new release (#18) shortly so the new version is available in the Arduino Library Manager.

@DrGFreeman DrGFreeman merged commit 45ebc04 into DrGFreeman:master Sep 26, 2020
@cgo
Copy link
Author

cgo commented Sep 28, 2020

This is ready to merge. Thanks again for your contribution!

You're welcome. Thanks for making this library available!

I will proceed to make a new release (#18) shortly so the new version is available in the Arduino Library Manager.

Sweet!

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.

2 participants