Evaluation of Vi

[ceilingwalker]

Hello all. I have nearly completed my first Vi. It does exactly what I want it to (apply 1-5 Volt signal out, read an input voltage in, read a series capacitance in, read the dissipation factor, and place all the results on a spreadsheet which I then open with Excel). Off to the left of the Vi is a sub Vi that is still in the works, it is simply a confirmation that the device is in position for test but haven't finished it yet. Very simple but since it is my first I am pretty most here will say it's crap and could be done much better and that is why I am posting it here, even though embarrassing, so that I may learn what I could do better with the same results. I have been through Core1, 2, and 3 online but this is the only practicle use of my training I have. This might be an unreasonable request however, I really want to learn so all criticism is welcomed. Thank you

[Bob_Schor]

I'm amazed -- you've managed to post a program that totally locks up my LabVIEW 2014 system!  Oh, there it goes -- it just takes a minute or two for the Windows 7 "busy" wheel to stop spinning when I try to scroll to the left to see the beginning of your VI.

So I'm a bit confused as to the nature of this post.  What is the Question?  Are you looking for general "design feedback" on this VI as a whole?  Are you looking for help in the section that is Diagram-Disabled (on the left)?

You've gotten a lot of help over the past several weeks from this Forum, but it is not evident that you have taken much of it to task.  I will refrain from making any comments until I have a better idea of what it is you are requesting.

Bob Schor

[PeterFoerster]

Here are a few suggestions:

• Make the enum for the state machine a typedef. That way you don't have to change every single one every time you add a state.
• Not sure what the for loop with N=1 does, but you should initialize and deinitialize the VISA connection outside of it.
• Same with the DAQmx VIs.
• There are a lot of Controls/Indicators without name in the BD. How is anyone supposed to understand this ever?
• You should use more clusters/arrays.
• The stop case should be handled within the state machine and not as a separate case.
• You should avoid the local variables.
• Why are there 2 stop buttons, one is properly handled and one isn't?

[altenbach]

1. Your state enum should be a  type definition, else you would need to correct it in many places if you ever change the items (e.g. add a new state).
2. Whtat's the purpose of the disabled code on the left?
3. Instead of the outer case structure, make a state for stop. It is much easier to maintain a single case structure than a stack of them. Shouldn't th TRUE be wired to the tunnel in the true case?
4. The stop button should be "latch when released" and not some exotic mechanical action. It should be a stop button, not a switch.
5. Elininate the stop2 local variable.
6. RESET LCR state:
1. Eliminate overlapping code.
2. What is the purpose of the for loop? doens't do anything useful, delete it!
3. You have two serial communications running in parallel or random order. I am sure you should define an order. Why do you configure the serial port twice in parallel?
4. ...
7. Your stacks of indicators should be kep in arrays instead. Keep them in a shift register and eliminate the local variables.
8. Your "X volt" states differ only by a few values. Combine the code!
9. Wouldn't it be sufficient to configure the serial,  AI and AO once at the start of the program, i.e. before the loop? Once is enough!

(EDIT: It seems Peter was faster today :D)

[ceilingwalker]

Yes Sir, I am asking for criticism of my project. I used as much of the advice given me, that I understood. Some of it I learned trial and error. I wish I understood all of the advice given me but the truth is, I don't. Having something like this, that required my effort, makes it easier to understand when advice is given. I hope this makes sense to you. Thank you

[ceilingwalker]

---

altenbach wrote:

1. Whtat's the purpose of the disabled code on the left? In my original post I must not have been very clear. It is something else that I am working on but am not using yet. It is supposed to be a permissive for the loop to begin.
2. Instead of the outer case structure, make a state for stop. It is much easier to maintain a single case structure than a stack of them. Shouldn't th TRUE be wired to the tunnel in the true case? I am not exactly sure I understand what you are saying. Are you suggesting that inside of the while loop it have only 1 case structure?
3. The stop button should be "latch when released" and not some exotic mechanical action. It should be a stop button, not a switch. I had to do this to satisfy the local variables I use however, I am getting feedback from several here that the local variables might not be a good idea.
4. Elininate the stop2 local variable. Yes, this is why I have the mechanism set the way it is.
5. RESET LCR state: I forgot about this case. I wasn't thru with it yet, it is a work in progress. I wasn't sure I really needed it yet, that's why it isn't used yet. Sorry about the confusion.
1. Eliminate overlapping code.
2. What is the purpose of the for loop? doens't do anything useful, delete it!
3. You have two serial communications running in parallel or random order. I am sure you should define an order. Why do you configure the serial port twice in parallel?
4. ...
6. Your stacks of indicators should be kep in arrays instead. Keep them in a shift register and eliminate the local variables. I am not exactly sure how to do this but I will try.
7. Your "X volt" states differ only by a few values. Combine the code! I am not sure what you mean to combine. Do you recomend keeping all the different states or combining them?
8. Wouldn't it be sufficient to configure the serial,  AI and AO once at the start of the program, i.e. before the loop? Once is enough! I tried this but when I did, it wrote all of my data onto one row on the write to spreadsheet. This was the only way I could find to get each state to be represented on the spreadsheet.

(EDIT: It seems Peter was faster today :D)

---

[ceilingwalker]

---

@PeterFoerster wrote:

Here are a few suggestions:

• Make the enum for the state machine a typedef. That way you don't have to change every single one every time you add a state. I remember a section of Core 1 or 2 taught about this, I will reference my matirial.
• Not sure what the for loop with N=1 does, but you should initialize and deinitialize the VISA connection outside of it. It is to reset an LCR meter however, I am not sure I need it at this point and neglected to point that out, in my original statement. Sorry about the confusion.
• Same with the DAQmx VIs.
• There are a lot of Controls/Indicators without name in the BD. How is anyone supposed to understand this ever? I understand
• You should use more clusters/arrays. Originally I started with some arrays but couldn't get the desired affect.
• The stop case should be handled within the state machine and not as a separate case. Are you suggesting I put a boolean in each state and wire it to the while loops stop?
• You should avoid the local variables. I couldn't figure out how to reset all the values to "0" when initialized. So I made them local variables and then write a "0" to each register at beginning of test cycle.
• Why are there 2 stop buttons, one is properly handled and one isn't? I was testing different ways of turning it off.
• Thank you for your input.

---

[altenbach]

---

@ceilingwalker wrote:

---

altenbach wrote:

1. Whtat's the purpose of the disabled code on the left? In my original post I must not have been very clear. It is something else that I am working on but am not using yet. It is supposed to be a permissive for the loop to begin.
2. Instead of the outer case structure, make a state for stop. It is much easier to maintain a single case structure than a stack of them. Shouldn't th TRUE be wired to the tunnel in the true case? I am not exactly sure I understand what you are saying. Are you suggesting that inside of the while loop it have only 1 case structure?
3. The stop button should be "latch when released" and not some exotic mechanical action. It should be a stop button, not a switch. I had to do this to satisfy the local variables I use however, I am getting feedback from several here that the local variables might not be a good idea.
4. Elininate the stop2 local variable. Yes, this is why I have the mechanism set the way it is.
5. RESET LCR state: I forgot about this case. I wasn't thru with it yet, it is a work in progress. I wasn't sure I really needed it yet, that's why it isn't used yet. Sorry about the confusion.
1. Eliminate overlapping code.
2. What is the purpose of the for loop? doens't do anything useful, delete it!
3. You have two serial communications running in parallel or random order. I am sure you should define an order. Why do you configure the serial port twice in parallel?
4. ...
6. Your stacks of indicators should be kep in arrays instead. Keep them in a shift register and eliminate the local variables. I am not exactly sure how to do this but I will try.
7. Your "X volt" states differ only by a few values. Combine the code! I am not sure what you mean to combine. Do you recomend keeping all the different states or combining them?
8. Wouldn't it be sufficient to configure the serial,  AI and AO once at the start of the program, i.e. before the loop? Once is enough! I tried this but when I did, it wrote all of my data onto one row on the write to spreadsheet. This was the only way I could find to get each state to be represented on the spreadsheet.

---

---

(2) Yes.

(3) You had it "latch until released" which is also not compatible with local variables. So what was your point? (I was talking about "stop", not "stop 2")

(7) if I say "combine", I probably mean combine, right? You can assign multiple states to the same case. Inside the case, create a very small case structure, one for each "X volt" state that contains the values that differ, then wire into the common, shared code.

(8) Your statement makes absolutely no sense. What does DAQ have to do with file writing???

[Bob_Schor]

Here are some Styles suggestions, designed to make your code more "intuitive" (which makes it easier for you and others to understand it, debug it, modify it, etc.).  I urge you to incorporate many of these into your code -- it takes a little discipline to do this after the fact.

• Strive to make every Block Diagram fit on a moderate-sized Monitor (say 1280 x 1024 pixels).  For all but the most trivial VI, the only practical way to achieve this is through using (many) sub-VIs.
• When creating a sub-VI, give it an Icon to identify it on the Block Diagram, and fill out its Description Property (at least list the Inputs and Outputs).
• (Almost) all sub-VIs should use the 4-2-2-4 Connector pattern, with Error In and Error Out in the lower outside connectors.
• The Error Line should run through every VI/sub-VI and should be used.
• Use Data Flow to sequence your VIs.  You almost never need a Frame (and certainly all of the Frames in your example could, and should, be removed).
• Try to keep wires straight, parallel, and short.
• All controls and indicators in the Block Diagram should have visible labels.  If you don't want the label to show on the Front Panel, turn it off on the Front Panel.  it should be possible to deduce the function of your Block Diagram just by looking at it (without having to find its corresponding Control/Indicator on the Front Panel).
• Strive to use absolutely no Local Variables.  The logic of stopping State Machines and multiple loops does not require (and is often harmed by) Local Variables.
• One way to stop a State Machine is to have the Stop button force the next State to be the Stop State, which has the True constant wired to the While Loop's Stop Indicator.  The Stop State does the opposite of the Initialize State, namely it closes down what needs to be closed down.
• As a corollary, don't change Boolean controls that LabVIEW defines as "Latch on Release" -- if you don't understand why they do it that way, ask -- there is probably a very good reason for it.
• The five States "1 Volt" .. "5 Volts" seem to be the same, differing only in the constant wired to the AO function.  [Note that there may be other differences, but it is extremely difficult for a code reviewer to see the differences between the loops).  Note that encapsulating the contents of these cases in a single sub-VI, with "what varies" brought out through Connectors.  Now what's going on is obvious.
• It is not clear why you designed this with individual cases for each voltage.  If you replace these States with a sub-VI with a "Voltage" input, you could replace the 5 States with a single State having a For loop counting to 5.
• Try to make your Block Diagram, particularly at the highest levels, indicate clearing what you are trying to do, and hide (in sub-VIs) the messy details of how you are doing it.

[ceilingwalker]

Thank you Sir for your input. Under what circumstances would one use a local variable? I remember in the training lesson it said something about preventing a race condition, and for feeding parallel loops, but I could be wrong about that.

"The five States "1 Volt" .. "5 Volts" seem to be the same, differing only in the constant wired to the AO function." That is correct, only the constant for each DAQ write changes. Originally I started this project using a sequencer. It was a lot more organized than the monster I created but for some reason it didn't seem correct. It also expanded my Vi, I was attempting to keep everything to where I could view the entire Vi without scrolling.