Good coding practices

[ojohnsen]

Good day, everyone.

Although I have dabbled a (tiny) bit with LabView in the past, I am quite new with the concept of graphical programming.  After testing and trying for a period, I've made my first attempt on a "serious VI".  Given that I have not seen a FAQ saying that such requests are unwanted, I wondered if someone could take a minute or two to check out my VI and see if they can spot any major disadvantages with my method of doing stuff.  After seeing the "Grumpy Old Men" thread, I know I might be taking a risk here, but I hope you'll be gentle. 

There are especially a couple of points that I wonder about:

- The VI is obviously meant to send a command to a recipient over an RS232 interface, in this case a TV set.  I'm using a byte array to set up the command constants and command length, and changing the initially zeroed bytes afterward according to the TV protocol.  Is this a good or bad method?  If bad, why?  And how else should I do it?

- I'm using quite a bit of data type converters, to avoid this automatic data type conversion that else would be taking place (identified by a red dot on the symbol input).  Is it really necessary to keep such a rigid control over the data types (like you would when programming in e.g. C), or is overflow never going to happen and I'm just making more work for myself?

Thanks a lot for any pointers!

Message Edited by ojohnsen on 03-02-2010 09:41 AM

[smercurio_fc]

Well, I'm sometimes grumpy, and some people would probably say I'm old, but I'll take a shot at being nice. Hey, it's been known to happen!

Overall, a pretty good start. Much better than some of the code we've seen on these boards from those new to LabVIEW. 

General comments/questions:

• Is this supposed to be used as a subVI or as a top-level VI? You indicated that it's an RS232 interface and I didn't see a VISA Configure Serial Port or a VISA Close.
• I don't think it makes a difference in this case whether you pre-allocate the array and use Replace Array Subset or just build up the array using Build Array.
• Get rid of the sequence frame - it's not necessary and just makes wiring annoying in the second frame.
• In the while loop don't rely on the loop iteration as a timer. Since you are under software control you can't guarantee the loop will run every 10 msec. A better way is to get the msec count before you enter the loop and get it with each loop iteration and see if it has exceed the timeout. This eliminates all the calculations you're doing for the timeout check.
• Don't stuff the connector pane. Related to the first point, since you've got the controls wired to the connector pane, the assumption is that this is a subVI. You should consider using a cluster to pass in the TV parameters. This way you can easily use the common and recommended 4-2-2-4 connector pane layout rather than the 5-2-2-2-5 one that you're using now.
• As far as connector pane assignments it is generally considered bad practice to connect controls on the right hand-side of the connector. Stick with controls on the left and indicators on the right.
• In you while loop you can use a string constant set to Hex Display mode instead of a byte array. Saves you having to do the conversion. Even with the way you're currently doing it that doesn't need to be inside the loop since you don't need to perform the same exact conversion each and every loop iteration.
• Currently your loop only stops on a timeout or a success. It does not stop if there's an error with the VISA functions. This may or may not be desired in your case.

Well, hopefully that wasn't too grumpy. Have to get back to work now, so I have to stop. Others can chime in as well.

[Ray.R]

I agree with SMercurio,  (although he's grumpier than usual today.. 😄 )

I would actually create a sub-vi where you convert the inputs to a string.  That way, you can fit all your code within a nice small area of a single screen.

When programming, stay away from the Stacked Sequences. Otherwise, anyone (including you) will be cursing lijke crazy in the future.  Also, the nasty Stacked Sequences make you write code where the wire runs from right to left, which is a huge "NO-NO".  

Adding a simple STOP button would be a good start to control that while loop. 

Question:  the 10 ms delay, did you actually want 10ms?  Or was it a nice round number?  You can use 1 ms as a typical minimum.  0ms is still valid, but may push the CPU usage up the percentage scale, but it will allow other tasks to take CPU time, which is good.  I simply mention it because it is a good trick to know when looping as fast as possible is cruicial.

Good start.

Below is the code for the sub-vi should you go that route.

 

(I should have posted as jpg or bmp, just to get smercurio even more grumpy 😉 )

Message Edited by Ray.R on 03-02-2010 01:15 PM

[Mark_Yedinak]

You can bypass the Build Array entirely and use a Format Into String to build your message. If you will be generating many different messages that all have the same general format you could create a simple VI that will build the different message types. You could also go the route of creating a subVI for each message type. As others have said avoid using sequence structures, especially the stacked sequence structure.

[JÞB]

oj-

So there are some things you did right.

• No local variables
• no race contions
• good data flow through the vi
• readable block diagram with minimal wire bends
• No hidden objects
• no overlaping objects
• Shift registers (feedback loops) are initiallized
• Meaningful icon

Some bad habits too.

• Stacked sequence 
• No VI documention
• no discriptions or tips on the user controls
• Avoid renaming "error in(no error)"and "error out".  While even the Style guide states to use initial capitals for controls almost every programmer continues to live with this cosmetic bug introduced in LV before any style guide existed.

Overall C+ and do read the style guide I linked.  You'll do fine with some practice.

[Yamaeda]

Looks like a very good start to me.

Just skip the sequence, there's no hurt in having to scroll a little sideways in the code (it can be quite annoying if you have to scroll alot in both directions though).

Group up the inputs into a cluster. (Not a general tip, but when you start to get 5+ inputs it's time to look at it seriously)
Make sure you use error in and out in lower left and right corners, data flow, wire chart and logic is quite messed up otherwise.

I've attached a slightly changed .vi, basically just clumped the inputs as a cluster and cleaned it a little. I'm sure not all will agree with me, but that's the beauty of programming, there's lots of viable paths.

/Y

[ojohnsen]

Thanks a lot for the replies!  And I appologize for the late reply.

The feedback was great, with a lot I didn't know or hadn't thought about.  I've attached a new version of the VI, where I've tried to incorporate much of the feedback from you.  It would be great if you could take a 2nd look!

Some individual comments:

@smercurio_fc:

• Yep, it's a sub-VI.  It's meant to be one out of many similar building blocks that will be used in a larger testing environment.  Therefore there's no RS232 initiation or closure in this VI, but will be fed to it from outter VI.
• I've added the msec counter instead of using the iteration counter. But I read in the help text that I had to be careful using the msec counter for this purpose, as it could roll over and void the comparison. Will this even be a case in reality? Won't it just be zeroed when the clock is 00:00?
• I've cut out the stacked sequence now, as more or less everyone suggests (or flat out commands? ).  But can't this cause problems with the command flow?  I mean, lets say the TX part takes a while to perform, say several 100 ms.  What prevents the while loop from timing out before the TX part's finished?

@Ray.R:

• No real reason for using 10 ms. really.  Just a nice round number, as you say.  But my thought was that it might be a good idea to set this value as high as the environment allows, to not use more CPU resources than necessary.  The differences between 1 ms. delay and 10 ms. is probably insignificant, but stil...

 @Yamaeda:

• Thanks for the modified VI.  I've taken the liberty to steal a bit of your layout. 

Message Edited by ojohnsen on 03-05-2010 03:58 AM

[Guruthilak]

1) The constsnts like the array constant (0E 00 06) along with the "Byte Array To String" must be kept outside the while loop. Since for every iteration the same conversion is happenning so its a waste of the resource

2) same thing with constant connected to "Wait (ms)"

3) Wire appropriate data type to the "Wait (ms)" to take off that coercion data.

Guru

Message Edited by Guruthilak on 03-05-2010 04:16 PM

[Ray.R]

Some "additional" esthetics comments.

1.  Do not view  terminals (controls . indicators) as icons.  They are big and simply waste real-estate.

     You can go to the Tools menu, select Options, and to the left select Block Diagram and remove the checkmark next to "Place Front Panel Terminals as icon".

2.  Avoid wiring in or out of the Structure (While Loop in this case) from the top or bottom.

3.  Try to keep wiring line straight.  You've done a fairly good job.

[JÞB]

oj-

MUCH better.  You are correct that you have a potential timing issue.  If the ms Timer is near roll over (about every 27 days) When you add Timeout to Tick you will have a Small number in the u32 that feeds your read loop timeout condition.  The first itteration of the loop terminates on timeout in this condition.  Use "Get Date Time.vi" to avoid this infrequent problem (which has a frequency so low you'll never trace it down if you even ever see it).  The infrequency of the issue is why few programmers worry.  I got bit ONCE by a wait during ms rollover in 10 years (caused an infinate loop so I was able to debug and probe and understand why my vi hung)

You did a good job incorporating most of the suggestions.  Why did you avoid Documentation?