Comments on Code

[pierroil]

Hi guys,

I'm a 2 weeks of simple DAQ + 1 month forums self taught labview user and I'm working on a rather large control application.

Before I continue, I think I've produced a pretty large chunk of one of the components of my application - it is responsible for controlling the motors.

I want to make sure it's robust and uses good style.

Can anyone out there can quickly point out some stylistic errors? Don't waste your time on this if you're busy - just do it for fun. 

Notes:

--> The references come from 1 main VI file which uses CTR SET. 

--> I have used a VI template since I would like to call this function 4 times - for 4 different motors with a different Axis (or Address).

--> I'm still pretty confused about the events node - in that I can't seem to figure out a way to get an event to trigger when a reference value changes. Signalling triggers on every write - which is not my intended purpose.

--> Also, I haven't yet tested the queues, nor have I ever used queues before, labview help just got me through it. 

--> This is 1 VI that calls a few SUB Vi's, some of them extremely simple, others that are a few layers deep. All of the SUB Vis do not use references, events or anything of the sort, they are simple data flow and loops to iterate code in a structured maner (setting data up in a readable format, read write, etc etc)

I'm worried I just have bad style or am doing certain things that can cause fatal errors.

Thanks!

PS: I remember Altenbach giving me a bunch of tips earlier on my code - it really helped push me forward.

 

[pierroil]

Forgot to mention: I hate labview clean up wiring... It has made my wiring extremely complicated to the point where I just spaced things out instead of cleaning up the wiring...

Random kinks...

[nathand]

It would help if you attach the actual VI instead of screenshots.  Also, it is worth cleaning up your wiring.  Experienced LabVIEW programmers expect wires to run left to right with minimal bends.  The unbundle/bundle where you copy data from one cluster to another should have space to see the wires between the nodes.

[pierroil]

Here's the VI.

On Wires: Yeah I cleaned them up right now for the bottom most loop. I just lumped the chunk of code in a sub VI. Looks neater.

Do you want all the SUB vi's too?

[pierroil]

Why is it neccesary to have space? I mean, I take it for granted they are connected if they are in contact.

Also,

Is that an appropriate use of Local Variables?

I'm not even sure if that works - I haven't tested my code since I added the user events.

[nathand]

It's not a requirement to have space, but I like to see wires explicitly connecting things.  Also it's very hard to delete a wire you can't see (if you remove an element), or add a new wire (if you add an element) when there's no space for the wire.

Why would you ask for comments on VI you haven't tested and might not work?

Your VI is almost definitely too complicated.  It should not be a VIT.  Use a standard subVI and make it reentrant with the pre-allocate clones option.

I don't see any local variables, but you're definitely abusing references.  Also I don't see any need for the event structure at the top with the user events, why not just use a queue or notifier so you have more control?

[apok]

• you do not need to show the different values of bundle/unbundle if the values being passed are not needed.
• error clusters are not wired and merged and how they are handled
• your use of a stop ref to a local is questionable
• i dont know if your architecture is a producer /consumer or some kind of hybrid multiple loop control

use labview 101... please get some training, read "the labview style book" by blume 

[pierroil]

Reentrant VI?

Will that allow me to change the control values while it is stuck in an execution loop? 

I would feed it references to control?

[pierroil]

The VI actually does work, just not the user events - they come from a counter reference from a piece of DAQ I cannot get started yet.

 How else can I synchronize all the stops? I used a local since I figured changing it in one loop would just change it everywhere.

I'll also look into using a notifier. Haven't explored that yet. 

[crossrulz]

---

@pierroil wrote:

Why is it neccesary to have space? I mean, I take it for granted they are connected if they are in contact.

---

In my experience, you can't take anything for granted.

As far as the bundle and unbundling is concerned, always bundle by name and you can get away with something like the following.  Notice that you don't need to wire everything straight through.  Only the values you are specifically bundling will be overwritten in the cluster.  I also showed the alternative of using the Inplace Element Structure, which seems more intuitive to me.