Create DrawingInTheAir_MotionActivated #39
Conversation
Replacing hardware button with the build in motion detection activation and audible feedback.
There was a problem hiding this comment.
Thanks for your contribution @xhuvom! I haven't had a chance to try the sketch yet, but it looks really cool so I'll try and do it sometime this week. In the meantime, there a few things that need to be fixed in the sketch code.
Please update this pull request by amending the same commit and force-pushing to the same branch, i.e. xhuvom:patch-1. Don't make any new commits or new branches to make the updates.
| * PME. Once training is finished, you can keep drawing letters and the PME | ||
| * will try to guess which letter you are drawing. | ||
| * | ||
| * This example requires a button to be connected to digital pin 4 |
There was a problem hiding this comment.
No button required here, right? Comment is wrong
| * This example demonstrates using the pattern matching engine (CuriePME) | ||
| * to classify streams of accelerometer data from CurieIMU. | ||
| * | ||
| * First, the sketch will prompt you to draw some letters in the air (just |
There was a problem hiding this comment.
This whole description is just a straight copy paste, which is mostly fine. However it would be nice if you had some specific information too, like for example, what's the difference between this sketch and the plain DrawingInTheAir sketch? put that extra information here, in the description
|
|
||
| /* Start the IMU (Intertial Measurement Unit) */ | ||
| CurieIMU.begin(); | ||
| CurieIMU.attachInterrupt(eventCallback); |
There was a problem hiding this comment.
Please be aware of the existing indentation (4 spaces). Sometimes you use 3, or 2, or 1, or none at all.
| CurieIMU.begin(); | ||
| CurieIMU.attachInterrupt(eventCallback); | ||
|
|
||
| /* Enable Zero Motion Detection */ |
There was a problem hiding this comment.
Indentation, next 10 lines
|
|
||
| CurieIMU.setAccelerometerRate(sampleRateHZ); | ||
| CurieIMU.setAccelerometerRange(2); | ||
| pinMode(11, OUTPUT); |
| void trainLetters() | ||
| { | ||
| for (char i = trainingStart; i <= trainingEnd; ++i) { | ||
| Serial.print("Initiate Motion and draw the letter '"); |
There was a problem hiding this comment.
I find this message a bit confusing. How about using the original message, with all of the button-related things removed?
Very simple and clear, like "Draw the letter 'A' in the air". That's really all we need here.
|
|
||
| // MOTION DETECTION interrupt subroutine | ||
|
|
||
| // global variable motion flag |
There was a problem hiding this comment.
global variable motion flag? what does this mean?
|
|
||
|
|
||
| static void eventCallback(void){ | ||
| if (CurieIMU.getInterruptStatus(CURIE_IMU_ZERO_MOTION)) { |
There was a problem hiding this comment.
Please use the same indentation level throughout the file, this function seems to switch between 2 and 4 spaces
|
|
||
| CurieIMU.setAccelerometerRate(sampleRateHZ); | ||
| CurieIMU.setAccelerometerRange(2); | ||
| pinMode(11, OUTPUT); |
There was a problem hiding this comment.
Is there supposed to be an LED attached to this pin? or something else? You need to mention that in the description. Tell users how to set things up.
But to be honest, the only reason the other example had a button was because it was required for input (no built-in buttons on the 101 board that can be used). Now that it's not needed to record samples, it would be really nice if this example required no extra hardware. Just plug in the board, upload the sketch, and start playing!
Would you consider removing the requirement for the button/speaker/whatever this is? or do you think it's really needed?
| CurieIMU.setAccelerometerRate(sampleRateHZ); | ||
| CurieIMU.setAccelerometerRange(2); | ||
| pinMode(11, OUTPUT); | ||
| digitalWrite(11, LOW); |
There was a problem hiding this comment.
Give the pin number a name, e.g. "LED_PIN" or whatever, instead of writing "11" everywhere
| /* While Motion detection inturrupt flag being held... */ | ||
| while (ledState == true) { | ||
|
|
||
| if (CurieIMU.accelDataReady()) { |
There was a problem hiding this comment.
accelDataReady() does not exist anymore, instead enable only the accelerometer with "CurieIME.begin(ACCEL)" and then you can check for "CurieIME.dataReady()". See the latest copy of the DrawingInTheAir sketch (in this repo) for details
| pinMode(buttonPin, INPUT); | ||
|
|
||
| /* Start the IMU (Intertial Measurement Unit) */ | ||
| CurieIMU.begin(); |
There was a problem hiding this comment.
use ACCEL to enable only the accelerometer, see latest copy of the DrawingInTheAir sketch (in this repo) for details
| * PME. Once training is finished, you can keep drawing letters and the PME | ||
| * will try to guess which letter you are drawing. | ||
| * | ||
| * The built in motion detection interrupt is being used to detect whether u are writing or not. |
There was a problem hiding this comment.
This line no longer needed with line 4, please remove it
| /* | ||
| * This example demonstrates using the pattern matching engine (CuriePME) | ||
| * to classify streams of accelerometer data from CurieIMU. | ||
| *** ENHANCEMENT: Automatic data capture with build in MOTION DETECTION functionality, no need of button or any extra input hardware. |
There was a problem hiding this comment.
This line has a lot of unnecessary words and capitalization. Please replace it with something simple and clear
"Motion is automatically detected using the CurieIMU motion detection interrupt"
| * will try to guess which letter you are drawing. | ||
| * | ||
| * The built in motion detection interrupt is being used to detect whether u are writing or not. | ||
| * While drawing in the air, the buzzer sounds, so wait until the buzzer turns off after drawing a letter and repeat the cycle. |
There was a problem hiding this comment.
Again, is the buzzer needed? I don't think it should be necessary. And a lot more people will be interested in this sketch if it requires no extra hardware.
| /* While Motion detection inturrupt flag being held... */ | ||
| while (motionFlag == true) { | ||
|
|
||
| if (CurieIMU.accelDataReady()) { |
There was a problem hiding this comment.
I'll repeat myself, again, accelDataReady does not exist anymore. Please look at the latest DrawingInTheAir example.
And update your version of corelibs-arduino101 from github, because this should not be compiling for you.
| } | ||
| if (CurieIMU.getInterruptStatus(CURIE_IMU_MOTION)) { | ||
| motionFlag = true; | ||
|
|
|
|
||
|
|
||
| // MOTION DETECTION interrupt subroutine | ||
|
|
There was a problem hiding this comment.
Why are there 4 blank lines here? remove them please.
| delay(2000); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
3 blank lines. use 1 only please, it looks messy & lazy otherwise.
| CurieIMU.begin(); | ||
| CurieIMU.attachInterrupt(eventCallback); | ||
|
|
||
| /* Enable Zero Motion Detection */ |
| CurieIMU.interrupts(CURIE_IMU_ZERO_MOTION); | ||
|
|
||
| /* Enable Motion Detection */ | ||
| CurieIMU.setDetectionThreshold(CURIE_IMU_MOTION, 20); // 20mg |
| pinMode(buzzerPin, OUTPUT); | ||
| digitalWrite(buzzerPin, LOW); | ||
| /* Start the IMU (Intertial Measurement Unit) */ | ||
| CurieIMU.begin(); |
There was a problem hiding this comment.
Again. Use "CurieIMU.begin(ACCEL)" to enable only accelerometer.
There was a problem hiding this comment.
Still several things not addressed here....
I still see lazy/wrong indenting all over. Please go through the whole file this time, and make sure everything is indented consistently
There are other things too. Please see all of my comments for specific details.
|
@xhuvom ping, any progress on this? Also I've been thinking, if the sketch works well, we might just add this feature to the original DrawingInTheAir sketch, instead of making a new sketch. What do you think about doing that instead? |
Replacing hardware button with the build in motion detection activation and audible feedback.