Style Tips for Spaghetti Block Diagram

[TUzzell]

I've been programming in LabVIEW for a few years and have a dozen or so programs that I've written use for calibration of test items. However, as the number of inputs and outputs grows for some of my programs, my block diagrams suffer from a clear case of spaghetti code. I have a top level VI that calls about 20 sub-vis during different steps in the calibration process, and about 50 user inputs from the front panel. I've attached a few screenshots of the spaghetti code below. It's hard to see what is going on, but you get the general idea.

Whenever I need to modify the block diagram to add additional measurement steps or add more user inputs based on different test parameters, I end up spending a huge amount of time trying to create space by moving wires and SubVIs around to try to fit everything so it isn't 5 screens wide, and the wires paths are at least somewhat visible. Even as it is, I have 5 layer of stacked sequences to process everything.

I sure there is a better way. Do you all have some tips as to how to build more manageable block diagrams? For the data, instead of wiring everything separately should I just make a 50 input cluster typedef and then wire the cluster to all my VI and unbundle the inputs I need inside each SubVI? Should I instead make 5 clusters of 10 inputs each to make the number of inputs more manageable? Any other style suggestions? I know from reading the forums that many LabVIEW programmers discourage using stacked sequences and would instead prefer to use a Producer/Consumer design pattern with queues, but I don't really understand how to do that. I would like to learn these techniques eventually, and apply that to future programs, but I am not there yet. Also, rewriting my program to a new design pattern would likely take me a few weeks or so, which I can't spare. I am looking more for tips that I can implement in a day or two of modifying my block diagrams without having to redo everything from scratch.

Thanks. Here are a few screen shots of spaghetti code.

[crossrulz]

State Machines are your friends here.  You can easily make a state for each of your steps.  You can maintain your state list with a queue or you can have a good study of the JKI State Machine.

[hooovahh]

Yup State Machines.  They aren't the solution to all things but do worlds to organizing your code and for readability.  You can create one single BFC (large cluster) that is in a shift register with the things you want to read and write to like variables.  Except you know where these variables are being read and written to, and you have state order to ensure one thing happens after another.  Once your code gets so large that a state machine can't help you, you should learn about actor based software designs.  Not necessarily NI's Actor Framework but actors in general.  Independent parallel running loops that do dedicated tasks.  This helps modularize your software so all the File IO stuff is handled in one place, and all your DAQ stuff is handled in another, and all your UI stuff is in another.  Breaking up larger problems into smaller more manageable ones is something LabVIEW is good at.

[smithd]

I have two answers which might help:

Answer A: Purchase Teststand and learn it. Without specific details I cant be sure, but it looks like the sort of application teststand is built for. If someone gives you crap about the cost, I'd argue that for sequential things like you've shown here teststand is a lot more maintainable (the bus factor for the code above is likely 1, and potentially 0 after a few months away from it). I'd also add that it has a lot of standardized reporting stuff built in...and if you're calibrating stuff, this would seem to be critical. So seriously, at least let someone from NI spend an hour giving you a demo.

Answer B: Looks to me like your first step might be to take some of those 50 front panel controls and put them into subVIs that are set to run as dialogs (in VI options, show front panel when called and close front panel when done). Those dialogs can take some of the logic thats in your main loop and organize it a bit -- "this event case is associated with this user input", and so on.  Those dialogs can return small clusters with the configuration (Monitor is a cluster of model[str] and serial[str] and resistance[u16]). If at the end of this you still have a ton of wires, as hooovahh says you can turn that into a clustersaurus/BFC, but its still better organized (clustersaurus->monitor->model vs just having a control called "monitor model" sitting out there).

Once you have some of what I'm going to call the "non-business logic" (eg the UI logic) out of the way, I think a state machine is a reasonable migration point to start with. I would add a caveat to this, which is you should also learn about different communication tools within labview -- in this specific case, queues and user events (a type of queue which works with the event structure) or "channel wires" which are intended as a simpler wrapper around the queue/event concept). I say this because it looks like there are several long-running tasks without user interaction, so creating parallel loops to run different tasks seems like the next logical step. In general you would use a queue to send data (eg "start running standard cal") from the UI loop to the tasks loop, and use a user event to send information (eg "I'm done" or "an error occurred") from the task loop back to the UI loop. drjdp has a video on some of the considerations for this here although it may be too much for you now -- hes coming at it from the other end "I've been using this pattern for a while and heres where bad stuff happens".

Once you've mastered this version, and if you feel like its still complex, the next step would be to dive further into frameworks (things like delacor's QMH or drjdp's messenger library  or ni's actor framework which are arguably in order of increasing abstract-ness) -- in these frameworks more stuff happens asynchronously from one another which can make the code more modular (the "standard cal process" is always over in this one library while the user dialog input is always over in this other library) but theres obviously a big learning curve and frameworks tend to require you fit into them rather than the reverse.

 

[LogMAN]

@smithd has stated very important points. It's also very important to get accustomed to techniques like the state machine in order to keep the code readable.

However, in my experience it is very unlikely that

a) shifting to a new system (like Teststand) solves the problem. That's for the same reason the previous one failed: A lack of understanding, planning and structure.

b) using "better" techniques improve the application. Again for the same reason.

c) replacing the current structure is even remotely possible because either the lead programmer left (or rather ran away) or it is too complex to comprehend.

Edit: To clarify, you should take those solutions into consideration of course!

This is of course my personal experience. Your situation may be entirely different.
That being said, I'm sure we all have made these mistakes and learned from it. You should do the same.

At first glance your VI looks confusing and hardly maintainable. On a closer look, however, it strikes me as a rather simple VI (i.e. not many different jobs going on at the same time) that only lacks structure and uses way too many indicators that presumably serve no real purpose other than being there (and maybe occasionally being viewed). I think the word we agreed on for such a VI is "paintainable", right?

You asked for suggestions to improving your current situation, so find below a summary of what you can do, based on the code you shared:

1. Remove code that is only meant for debugging purposes.
   
   With this I mean indicators such as the ones in the inner while loop. If the loop runs at max. speed, it updates the indicators every 20 ms. That is quite a bit faster than any human being can physically comprehend. Therefore, you have no reason to keep the indicators updating inside the loop. If you need to display the last value, simply move them outside the loop and use the last value.
   
   Once you removed the indicators, remove any code that isn't connected anymore. That should get rid of "dead" code.
    
2. Take a closer look at the inner while loop and replace it by a for loop
   
   If you look closely, you'll notice that the inner while loop is actually a for loop. The conditions are:
   
   a) The boolean control "Stop Loops and Save Data 2" must be True, or
   b) The number of iterations is equal to the length of the "Frequency" array.
   
   Condition b) is clearly a perfect fit for for loops.
   Once you have made it a for loop, the second condition can simply be removed, which makes it a bit easier to understand.
    
3. Move static functions outside the loop
   
   Inside the inner while loop are some multiplication functions that multiply values that come from outside the loop. These functions will always produce the same result inside the loop and therefore should be placed outside.
   It's a small change, but stuff like this makes the diagram easier to comprehend.
    
4. Initialize the cluster outside the loop
   
   This is probably the biggest and easiest fix you can do.
   Take a close look at the "Bundle By Name" for your cluster in the loop. With the exception of two indexed arrays, all parameters are statically provided from outside the loop.
   
   You can easily clean up this code by initializing the cluster outside the loop and only add the indexed arrays in the loop.
   This should reduce the size of this loop considerably. It will also allow to see the purpose of the loop.
    
5. Rearrange wires and don't hide wires behind structures or VIs
   
   This may sound like a lot of work (because it is), but one very important factor in LV is managing wires. There is actually a thread on LavaG dedicated to handling wires. I suggest taking a look, you can learn how other people do it:Take your time to remove wire bends whenever possible. I know it sounds ridiculous, but once you get accustomed to it you do it by reflex at which point you can start getting lazy again 😉

Edited February 7, 2019 by LogMAN

[smithd]

11 hours ago, LogMAN said:

1. Move static functions outside the loop
   
   Inside the inner while loop are some multiplication functions that multiply values that come from outside the loop. These functions will always produce the same result inside the loop and therefore should be placed outside.
   It's a small change, but stuff like this makes the diagram easier to comprehend.

Its worth mentioning that LabVIEW should do this for you, its called loop invariant code motion, so don't worry about it for performance reasons.You should put the code wherever its more readable.

In this case, I'd argue the most readable form is leaving the math where it is, but moving the source values (array size, iteration terminal, control) closer in. I'd also say use a local variable for the control -- the downside to this is that, in fact, using a local variable will stop the loop invariant code motion from happening 😕

In this particular case, I'd like to mention that you can make code more readable through various means -- one is code movement, another is labelling wires or using self-labelled elements like clusters. You can also add comments, but before you go adding comments I would always always look at the code and see if it can be changed to use a different tool to make the meaning more self-evident. To stick with the current section of code:  personally, I find pretty much all iteration-based math to be incomprehensible. I sat here for like a full minute trying to figure out why on earth you were taking max(0, i-10) ...but stepping back for a second, it looks like it should probably just be a case structure with two cases, "..10" and "11..". There is always a case structure in this code, but in your version its hidden inside the max() function and if you use a case structure its explicit. Then you have the problem of "why 10?", but at least anyone reading the code is not stuck back at "why maximum?".

Along these lines, one thing that might really help you is if you find yourself a buddy and try to explain the code to them (bit by bit, i mean, definitely not all at once). As logman said the application isn't really all that complicated, and I would bet that anyone with a semi-softwarish or logical background could follow along if you walked them through it. The purpose would be to rethink code like the above. If you're sitting there explaining to another human being "well I want the index of this listbox to be 0 up until iteration 10 and then I want it to start incrementing by 1, so what I did is I took the maximum of 0 and the iteration terminal and then built that into an array with 0 and used that as the index" you might start thinking to yourself "man I really need to make this easier to follow".

Edited February 8, 2019 by smithd

[LogMAN]

 

1 hour ago, smithd said:

Its worth mentioning that LabVIEW should do this for you, its called loop invariant code motion, so don't worry about it for performance reasons.You should put the code wherever its more readable.

Good point.

Now that you mention it, the output of the multiplication functions are connected to a VI that (as far as I can tell) builds status messages and updates the progress bar (see the references connected to it). The output of that VI is displayed on the FP at a rapid pace.

The VI obviously has too many responsibilities and should be split into multiple VIs. For example:

• Calculate Progress
• Calculate Estimate Time To Complete
• Calculate Estimate Time To Next User Action
• Display Progress
• Display Status Message
• Display Time To Complete
• Display Time To Next User Action

These VIs can be put in sequence, which makes it very easy to follow. It's also much more reusable than the current solution.

I'm well aware that the example above is probably too fine grained for the purpose of this application, but there is a high chance that each of these VIs can be reused multiple times or even omitted if not needed. That is not possible with the current solution. That is probably why there are two Boolean constants attached to the VI right now (i.e. too enable features that are only required in this particular section of code).