Added pingRead implementation. #48
Conversation
|
Some servo changes were just added. I suspect that the merge conflict is caused by the source mapping in the built file. Can you rebase against master and rebuild? |
| pingRead() { | ||
| throw new Error('pingRead is not yet implemented'); | ||
| pingRead(config, handler) { | ||
| const pulseOut = config.pulseOut; |
There was a problem hiding this comment.
This and the next line can be shortened to:
const { pulseOut = 5, value = HIGH, pin } = config;Yay ES6 :)
EDIT: I just checked the info on this method at https://github.com/rwaldron/io-plugins#pingreadsettings-handler, and there's a few more things we need to do with settings. Specifically, we need to assign default values to some of the properties (I put them in the example above).
There's also the value property that we need to handle as well. I'm actually not sure what it's supposed to do though. @rwaldron, can you shed some light on this property?
There was a problem hiding this comment.
I wasn't too sure myself - especially about pulseOut. I'm assuming it is used to indicate how often a reading should be taken from the ultrasonic sensor. But not sure what the unit is either. 5μs / 5ms seems too short and 5s seems too long.
There was a problem hiding this comment.
It's the second arg passed to pulseIn
pulseIn(pin, HIGH);
Reads a pulse (either HIGH or LOW) on a pin. For example, if value is HIGH, pulseIn() waits for the pin to go HIGH, starts timing, then waits for the pin to go LOW and stops timing. Returns the length of the pulse in microseconds or 0 if no complete pulse was received within the timeout.
(https://www.arduino.cc/en/Reference/PulseIn)
Looks like it's irrelevant to Sonar, so you can safely ignore it. I'll make a proper table for those properties
There was a problem hiding this comment.
Look at Particle-IO, it just ignores everything except for pin property: https://github.com/rwaldron/particle-io/blob/master/lib/particle.js#L544-L565
There was a problem hiding this comment.
|
I left a few comments, please take a look when you get a chance, and thanks for the pull request! |
|
Thanks for all the great feedback. Will definitely need time to absorb it all. In Tokyo for the weekend so will resume looking into starting Next week. |
|
|
||
| const interval = setInterval(() => { | ||
| sonar.read(handler); | ||
| }, pulseOut); |
There was a problem hiding this comment.
This is incorrect—pingRead should only read once and emit the reading, take a look at how it's used: https://github.com/rwaldron/johnny-five/blob/master/lib/proximity.js#L192-L199
Note that particle-io only does a one time event: https://github.com/rwaldron/particle-io/blob/master/lib/particle.js#L558 and voodoospark doesn't register reporting for that pin when it receives a PING_READ command.
Same with Firmata.js: https://github.com/jgautier/firmata/blob/master/lib/firmata.js#L1437
pingRead is really just garbage and everyone should use HCSR04 sensors with an I2C backpack. Generally, once there are more than 1 of these pingRead operations, the process is likely being blocked too much.
There was a problem hiding this comment.
@rwaldron the algorithm was translated from a python example. My thoughts also were that that the constant polling would tie up the CPU; the plan is to switch using interrupts in raspi-sonar once integration is completed. Would you see this as a more viable approach? Especially for using more than a single ultrasonic sensor?
There was a problem hiding this comment.
All I meant what that this call should be changed: setInterval => setImmediate
|
@nebrius, @rwaldron - I've rebased and modified the code based on the discussion and changes requested. I've tested the code with Johnny-five and ensured using a Proximity class is working as expected and the results (distance measurement) being produced are accurate. Can you please let me know if there are any other outstanding issues that I might have missed? Thanks again for all the help 😄 |
Thank you :) that's the most important thing ❤️ @nebrius is the captain of this ship, but you get my vote! |
|
Sorry, been busy. I have time set aside early this evening (PST) to go through this PR, so hang tight! |
|
I'm testing things out now, although my sonar sensor seems to have died :( Not a big deal though, I should be able to work around it. In the meantime, I opened a pair of issues on raspi-sonar for some documentation stuff, can you take a look? |
|
@rwaldron Thanks 👍 @nebrius Yikes, sorry to hear that - I broke a sensor too while I was playing around with getting it working initially. Thanks for raising the issues. I've addressed both: License? & Documentation you have raised in the raspi-sonar repo. Fighting off a sore throat at the moment so was able to look at this on my day off... 😷 accidentally closed the PR oops 🚶 |
| @@ -1,3 +1,9 @@ | |||
| ## 5.3.0 (2016-2-19) | |||
There was a problem hiding this comment.
Can you try and figure out why these are showing up as a diff? It looks like GitHub thinks two of my commits are new...which worries me a little.
There was a problem hiding this comment.
Probably something went wrong during my rebase attempt. There aren't supposed to be any changes at all to this file from my side. I'll try to roll the changes to this file back (not sure how yet).
Not sure how to proceed. Bit puzzling for me too...
There was a problem hiding this comment.
Sorry, I somehow missed your last comment, that was my bad.
OK, I think I worked out a solution for you:
# Save your current work on pingread
git checkout -b temp
# Go back to master
git checkout master
# DANGER: this will delete all of your work off of master. If anything at all goes
# wrong, stop here and report back. Your work will be saved on branch temp.
git reset --hard f750377e
# If you've already added my repo as a remote, feel free to skip
git remote add nebrius git@github.com:nebrius/raspi-io.git
# If you already added my repo as a remote, use the name you gave it here instead of nebrius
git pull nebrius master
# This will remove your work from your master branch on GitHub and add in my latest changes
git push --force origin master
# This will be the branch that is used for the PR
git checkout -b pingread
# This will pull your branches from temp, but without my commits in the middle giving a clean history
git cherry-pick --strategy=recursive -X theirs f713bb0e 903286ed 5a273eb7 c01f2685
# Push the branch
git push origin pingread
After you've done those steps, open a new PR from that branch and close this one, and everything should be good!
There was a problem hiding this comment.
Closing this PR following the above steps and opening a new PR from pingread branch.
|
Sorry been a bit hectic so couldn't get around to this again for a while. Created new PR 55 based on the above instructions. |
|
Did this make it into rasp-io? Keen to start using J5 proximity with the HC-SR04 connected to Pi2 |
|
@alexcroox not yet. Check out #55 to follow along with the progress. Also, ping @sesh-kebab, can you take a look at the feedback in #55? |
pingRead implementation as a result of discussion started on the johnny-five issue842.
Interacting with an ultrasonic sensor requires timing pulses to 10 microseconds and measuring the response to microsecond accuracy. To enable this, created a new native node plugin raspi-sonar. This pull request is to add a dependency to raspi-sonar to allow implementation of the pingRead method of the io-plugin interface.
Thanks for all your help with everything and patiently answering all my questions @nebrius & @rwaldron. Have learnt so much as result 😃
I'm expecting more work is needed before this PR can be accepted. But I believe the implementation has reached a stage where we can start having a conversation to move towards completing the pingRead implementation so we can start using ultrasonic sensors using jhonny-five on a RaspberryPi.