Critique This VI!

[NMarsh94]

I'd appreciate anyone who could give me some pointers as to how to make this (attached) VI better.  I'm new to LabVIEW.  Any tips or comments would be great.  Please keep in mind, This does EVERYTHING it's supposed to do, and I'm quite happy with this.  All I'm looking to do is make it more efficient/cleaner/better if possible.  However, if there is some function I can add that will do that, I'm happy to make some changes!

The purpose of the VI is to recognize notes played by my guitar and display which note is being played via Boolean.  Each note has a certain range of frequencies for each octave, which I have programmed into the case structure which can be seen.  The values cover a total of 8 or 9 different octaves.  The only issue I've had since the beginning (and I don’t know how to fix) is that on my waveform chart, a frequency averaging around 100 Hz is being plotted, even if nothing is plugged in.  It isn’t that big of a deal, but if this was going to a customer, for them I’m sure it would be.

 

Clearly, this is going to be a tad difficult to test yourself.  In order to connect my guitar, I used this cord: 

 

https://www.google.com/shopping/product/2590944652720646368?hl=en&sugexp=les;&gs_rn=1&gs_ri=serp&tok=DuO5-FOT3EN7z5tMt31Gwg&pq=guitar+to+microphone+jack&cp=11&gs_id=1k&xhr=t&q=griffin+guitarconnect&pf=p&sclient=psy-ab&oq=griffin+gui&gs_l=&pbx=1&bav=on.2,or.r_gc.r_pw.r_qf.&biw=1600&bih=775&tch=1&ech=15ψ=lT4JUZCzJvKw0QGX0YBY.1359560370418.1&sa=X&ei=uz4JUYvSAePV0gGZjICADQ&sqi=2&ved=0CKIBEMwD

 

If there are questions, just post here.  Thanks!

Note Recognizer.zipUnavailable

[ShaunR]

Well. The case statement is looking a bit unwieldy.

You can take advantage of the fact that an octave is 2^n of the fundamental and a semitone is the 12th root of 2 (or about 1.059)since you are only interested in the fundamental frequency. Then you should be able to calculate directly and represent it as a bit pattern.

Edited January 30, 2013 by ShaunR

[ned]

Minor style point: straight wires look nicer than ones with lots of bends.

 

I suspect that you could eliminate the ~100hz signal on the chart by wrapping the chart terminal in a case structure so that it only writes new data to the chart when the frequency is above some minimal value.

[hooovahh]

The first issue I have with your code isn't your code at all.  You have in your profile settings saying you use 2011 but these VIs are saved in 2012.  No big deal just thought I'd mention to help LAVA try to have the version you use the most, the version in your profile.

 

You have wires going from right to left.  This makes code less readable and should always go from left to right.  If for some reason you just must go from right to left, I recommend right clicking the wire choose Visible Items >> Labels and then add the text "<< <<" (without quotes) to help identify places that have wires going the wrong way.

 

You're using a sub VI for the Elapsed Time, which was an Express VI.  I would recommend right clicking and choose "View as Icon".  This takes up less block diagram space and is a more commen way sub VIs are seen.

 

Wires go under things.  Whenever possible don't put wires under subVIs, or constants.  Try to just cross other wires.

 

Unnessicary bends.  Try to have as few bends as possible in the wires

 

Error handling.  If there is an error writing the file your loop will continue.  Try to have some error handling so it at least gives the user a chance to exit if there is a problem.

 

Your while loop never ends!  You wired a constant to the stop terminal.  This means your program never ends and you are forced to press the Abort button.  As someone else has said, this is like stopping your car by crashing into a tree.  It will stop you but you should use the brake.  At a minimum add a stop button so the file gets closed properly and the other resources closed.

 

Personal preference but I like to see labels to the left of their controls not on top, not a big deal.

 

I would have changed how the frequency determines the note with an array of constants, instead of using the case structure.  It is less readable, and harder to update if you ever wanted to change it.  The method Shaun mentioned is probably good too.

 

I don't like Silver controls for much, especially for simple things like Error that the user is never intended to see.  If you do use Silver controls try to be consistent.  You have the Frequency, Chart, and booleans silver but the threshold and sound format are not.

 

Add more comments.  I see very little explaining what's going on.

 

Add VI documentation explaining what this is and how to use it.

 

There is only one plot on the chart and it is named "Plot 0".  I would either name it, or remove the channel list from being visible.

 

Because there is a while loop I made the assumption that this is a standalone application.  If this is the case you should have the window appear like a normal Windows application, with close working, and other debug controls hidden.

 

When you run the VI it asked for a place to save a file.  It does not say what the file is for, or what the expected name/format is.  It should have a VI before it asking to get the path and handle these settings.

 

If the user cancels the file dialog an error is thrown, this should be handled.

 

I'm guessing a way to fix the 100Hz is to check the "Detected Amplitude" from the cluster and if it is below a threshold then ignore the value.  

 

I've done some of these changes but not the ones I felt unsure of how you would want it done.

Note Recognizer Hooovahh edit.zipFetching info...

[NMarsh94]

First off, thank you everyone for your help!  I really appreciate it!!  On a more personal scale...

ShaunR - How do you know that math there? Also, I'm more or less using a range (halfway between each semitone +/- 1 Hz) to show the note.  Sounds a little weird maybe, but I usually keep my guitar in tune but don't always know what note I play.  So there's a chance that these notes played won't be EXACTLY what the frequency of a note actually is.  So how can I use this math to keep my ranges?

ned - Great idea!!

 

hooovahh - Your edited VI and information/advice is impeccable.  I can't thank you enough.  My only question is how do I get to checking the "Detected Amplitude" from a cluster?  Like, which cluster are you talking about and how do I do it? Also, my profile does say I use 2012, although it says I started in 2011.

 

Thanks again everyone.

hooovahh - scratch that question about the detected amplitude. Feeling stupid now haha.

[hooovahh]

  On 1/30/2013 at 8:01 PM, NMarsh94 said:

Also, my profile does say I use 2012, although it says I started in 2011.

Oops you are right sorry.  

[ShaunR]

  NMarsh94 said:

ShaunR - How do you know that math there? Also, I'm more or less using a range (halfway between each semitone +/- 1 Hz) to show the note.  Sounds a little weird maybe, but I usually keep my guitar in tune but don't always know what note I play.  So there's a chance that these notes played won't be EXACTLY what the frequency of a note actually is.  So how can I use this math to keep my ranges?

Google is your friend But looking at the javascript source here should help you out

 

As for ranges. Well. They key is to convert your measured frequency to numbers of semitones from a base frequency. You are not counting in frequency, rather, semitones so round up or down fractions of semitones/crotchets/quavers/lemons/whatever (just be consistent).

 

@Hoover.

Glad I don't work for you.lol.

Edited January 30, 2013 by ShaunR

[todd]

Threshold, in range and coerce and number-to-boolean array-to-cluster for display.

A little longer way around than how Shaun mentioned.

tones.zipFetching info...

Edited January 30, 2013 by todd

[NMarsh94]

Thank you todd!  

 

Hooovahh, another question for you.  Why did you include a "stop" event case?  Couldn't you have just wired the stop button to the "or" function outside the event structure, but inside the while loop?  Just to be clear, I'm not criticizing, just curious.  Maybe I'm missing something is all haha.

[hooovahh]

  On 1/31/2013 at 1:25 PM, NMarsh94 said:

Thank you todd!  

 

Hooovahh, another question for you.  Why did you include a "stop" event case?  Couldn't you have just wired the stop button to the "or" function outside the event structure, but inside the while loop?  Just to be clear, I'm not criticizing, just curious.  Maybe I'm missing something is all haha.

No problem, feel free to question things like this for clarification.

 

I thought about leaving the Stop in the timeout case, and this is a valid way of handling it for a stop if timeout occurs often (and in this case it does).  I wanted to properly handle the closing of the VI which is why I added the event structure and the "Panel Close?" event.  For consistency I added the "Stop" Value change because it is parallel to the "Panel Close?" in functionality.  Another thing to be aware of is not all applications have a timeout.  Many VIs may only take action if the user performs some action, and in these cases wiring an Or to the Stop terminal is not a good idea, and instead you should have an event that handles the pressing of the stop button.

 

As a quick example take a look at the VI attached.  It shows three cases, enabled using the Disable Diagram Structure.  It shows polling the stop button, then reading the stop button but without a timeout causes issues, and then no polling but instead using events.  I added the Stop Value change because it is the more correct solution in many cases, but for your VI it didn't really matter.

Stopping Test.viFetching info...

[ShaunR]

  On 1/31/2013 at 1:25 PM, NMarsh94 said:

Thank you todd!  

 

Hooovahh, another question for you.  Why did you include a "stop" event case?  Couldn't you have just wired the stop button to the "or" function outside the event structure, but inside the while loop?  Just to be clear, I'm not criticizing, just curious.  Maybe I'm missing something is all haha.

To paraphrase Hoover............

With a stop button only. If you click the X button on the form (as users have been trained to do). Your panel will disappear (so can't get to the stop button) but your VI will still be running.

Edited January 31, 2013 by ShaunR

[NMarsh94]

I get it now!!  Thanks everyone so much!  

[Daklu]

Eliminating unnecessary wire bends is important, but I'd like to call attention to something on Hoovahh's block diagram that really helps readability.  Notice how all the file I/O vis are (mostly) aligned horizontally and the sound vis are (mostly) aligned horizontally at a different height.  IMO spatially separating the vi "families" like this goes a long ways towards helping me quickly understand what is happening in the vi.