Building an OPC client in Excel

Question

I am pretty deep into building a very multi-faceted little app in Excel with VBA. It does a number of tasks, all centered around using OPC to get tag values from several PLC's and doing various things with the info, like publishing a webpage (using a module I found, not mine), creating log files and tables and sounding some audible alarms for the office.

What I'm doing now is, upon a button push, connecting to the server (RSLinx), then entering a loop that first reads the tag values, then does each of the above tasks if the associated checkbox is checked. This will run fine indefinitely so long as the user doesn't screw with it or Linx or let the computer lock.

I am a beginner coder so, please give me some feedback on the code itself, but what I'm really trying to do is make this bulletproof, so that it will run without fail. I've added some things like on a selectionchange event, if you're connected, give a messagebox that says not to make changes while running. But, I know this can be much better.

Also, I already know this might be done better in various other ways, but this is a little beginner pet project for me and I'm going to see it through before I move on. All criticism is welcome.

Option Explicit  ' variables must be declared
Option Base 1    ' array starts at index 1

'Dim OPCServer1 As OPCServer
Dim WithEvents OPCGroup1 As OPCGroup
Dim MyOPCItems() As OPCItem
Dim NumberOfTags As Integer
Dim ReadInterval As Double



Sub OPC_Connect()
    debugEvent "OPC - OPC_Connect()"
    status_update ("Connecting...")
    On Error GoTo Error_OpcConnectionFailure
    Dim GrpName As String
    Dim i As Integer
    Dim g As Integer
    Dim h As Integer
    Dim row_tag_name As String

    For h = 1 To 7
        Module1.SavedThisTime(h) = False
    Next h

    ThisWorkbook.connected = True
    GrpName = Cells(5, 2)
    'NumberOfTags = Cells(6, 2)
    NumberOfTags = 0
    status_update ("Adding Tags...")
    For g = 1 To 10000
        If Not IsEmpty(Cells(4 + g, 4).value) Then
            NumberOfTags = NumberOfTags + 1
        End If
    Next g

    If Not ThisWorkbook.OPCServer1 Is Nothing Then       'safety
         Exit Sub

    End If

    Set ThisWorkbook.OPCServer1 = New OPCServer
    Call ThisWorkbook.OPCServer1.Connect(Cells(4, 2))                'connect to the OPC Server
    Set OPCGroup1 = ThisWorkbook.OPCServer1.OPCGroups.Add(GrpName) 'add the group

    'add the 6 items
    ReDim MyOPCItems(NumberOfTags)


    For i = 1 To NumberOfTags
        On Error GoTo Error_TagNotFound
        If Cells(4 + i, 6) = 1 Then
            row_tag_name = Cells(4 + i, 4) & "." & Cells(4 + i, 5) & ",L1,C1"
            Set MyOPCItems(i) = OPCGroup1.OPCItems.AddItem(Cells(4 + i, 4) & "." & Cells(4 + i, 5) & ",L1,C1", i)

        Else
            row_tag_name = Cells(4 + i, 4) & ",L1,C1"
            Set MyOPCItems(i) = OPCGroup1.OPCItems.AddItem(Cells(4 + i, 4) & ",L1,C1", i)
        End If
    Next i
    status_update ("Connected")
    ThisWorkbook.LoggedThisTime = False
    Time_Delay (1)

Exit Sub
    'Debug.Print "OPCServer1 is " & OPCServer1 & " at end of Connect sub"
Error_OpcConnectionFailure:

    OPC_Disconnect
    MsgBox ("Connection Failed:" & vbNewLine & vbNewLine & "Check Your server name and ensure RSLinx is Running.")
    Exit Sub


Error_TagNotFound:

    OPC_Disconnect
    MsgBox ("Connection Failed. Check tag: " & vbNewLine & vbNewLine & row_tag_name)
    Exit Sub

End Sub



Sub OPC_Disconnect()

    ThisWorkbook.connected = False
    debugEvent "OPC - OPC_Disconnect()"
    On Error Resume Next
    If ThisWorkbook.OPCServer1 Is Nothing Then
        Exit Sub
    End If
    Call ThisWorkbook.OPCServer1.OPCGroups.RemoveAll 'free all the items and groups
    Call ThisWorkbook.OPCServer1.Disconnect          'disconnect from the OPC Server
    Set ThisWorkbook.OPCServer1 = Nothing
    ThisWorkbook.OPC_StopReadLoop = True
    status_update ("Not Connected")

End Sub

Sub OPC_RefreshServer()

        debugEvent "OPC - OPC_RefreshServer()"
        OPC_Disconnect
        Time_Delay (1)
        OPC_Connect

End Sub

Sub OPC_Read()

    debugEvent "OPC - OPC_Read()"
    status_update ("Reading Tags...")
    On Error GoTo Error_TagRead
    Dim Temp_Buffer1
    Dim ServerHandles() As Long
    ReDim ServerHandles(NumberOfTags) 'item ID (server side)
    Dim Values() As Variant         'return values
    Dim Errors() As Long

    'these next two are different. They are variant arrays, not arrays of type variant. A variant that is an array of a type.
    Dim Qual As Variant     'not using but must provide for function call
    Dim TimeValue As Variant

    Dim i As Integer

    If ThisWorkbook.OPCServer1 Is Nothing Then 'safety
        If Sheets("Setup").AutoReconnect_ChkBx.value = True Then
            OPC_RefreshServer
        Else
            Exit Sub
        End If
    End If

    If Not (ThisWorkbook.OPCServer1.ServerState = OPCServerState.OPCRunning) Then  'safety
        Exit Sub
    End If

    'set up which items to be read
    For i = 1 To NumberOfTags
        ServerHandles(i) = MyOPCItems(i).ServerHandle
    Next i

    Call OPCGroup1.SyncRead(OPCCache, NumberOfTags, ServerHandles, Values, Errors, Qual, TimeValue)

   'put the value and time stamp in cells

    For i = 1 To NumberOfTags
        Cells(4 + i, 3) = Values(i)
   '    Cells(4 + i, 2) = TimeValue(i)
    Next i
   'free the memory
   Erase Values()
   Erase Errors()

Exit Sub
Error_TagRead:
    OPC_Disconnect
    MsgBox ("Tag Read Error." & vbNewLine & "Please reconnect to server.")
    Exit Sub
End Sub

Sub OPC_Write()

    debugEvent "OPC - OPC_Write()"
    Dim ServerHandles(6) As Long    'item ID (server side)
    Dim Values(6) As Variant        'values
    Dim Errors() As Long
    Dim i As Integer

    If ThisWorkbook.OPCServer1 Is Nothing Then       'safety
     Exit Sub
    End If

    If Not (ThisWorkbook.OPCServer1.ServerState = OPCServerState.OPCRunning) Then  'safety
     Exit Sub
    End If

    'set up which items to be write
    For i = 1 To 6
     ServerHandles(i) = MyOPCItems(i).ServerHandle
    Next i

    'fetch the values from the cells
    For i = 1 To 6
     Values(i) = Cells(8 + i, 3)
     If Values(i) = "" Then
      Values(i) = 0
     End If
    Next i

    Call OPCGroup1.SyncWrite(6, ServerHandles, Values, Errors)

End Sub

Sub ReadBtn_Click()
    debugEvent "OPC - ReadBtn_Click()"
    OPC_Read
End Sub

Sub OPC_LoopRead()

    debugEvent "OPC - OPC_LoopRead()"
    On Error GoTo Error_Reconnect
    ReadInterval = Sheets("Setup").Range("C15")
    ThisWorkbook.enable_audible_alarm = False
    Module1.logged_this_time = False
    Do While ThisWorkbook.OPC_StopReadLoop = False
        Application.Cursor = XlMousePointer.xlDefault
        status_update ("Reading Tags")
        OPC_Read
        status_update ("In Time Delay")
        If Not ThisWorkbook.OPCServer1 Is Nothing Then       'safety

            Time_Delay (ReadInterval)
            If Sheets("Setup").audible_alarms_enable_ChkBx.value = True Then
                status_update ("Sounding Alarms")
                Sheets("Audible Alarms").LoopAudibleAlarms
                status_update ("Done Sounding Alarms")
            End If
            If Sheets("Setup").NewLogRow_ChkBx.value = True Then
                Sheets("Setup").NewLogRow_OnInterval
            End If
            If Sheets("Setup").website_publish_enable_ChkBx.value = True Then
                status_update ("Publishing")
                Publish
                status_update ("Done Publishing")
            End If
            If Sheets("Setup").LogData_ChkBx.value = True Then
                status_update ("Saving Log File")
                LogData
                status_update ("Done Saving Log File")
            End If

        End If
    Loop
    ThisWorkbook.OPC_StopReadLoop = False

Exit Sub
Error_Reconnect:
    If ThisWorkbook.OPCServer1 Is Nothing Then 'safety
        If Sheets("Setup").AutoReconnect_ChkBx.value = True Then
            OPC_RefreshServer
        Else
            Exit Sub
        End If
    End If

End Sub

Private Sub OPC_ConnectBtn_Click()

    debugEvent "OPC - OPC_ConnectBtn_Click()"
    OPC_Connect
    OPC_LoopRead

End Sub

Sub OPC_DisconnectBtn_Click()

    debugEvent "OPC - OPC_DisconnectBtn_Click"
    OPC_Disconnect

End Sub

Sub OPC_Read_Once_Btn_Click()

    debugEvent "OPC - OPC_Read_Once_Btn_Click()"
    OPC_ReadOnce

End Sub


Sub status_update(status As String)
    debugEvent "OPC - status_update(" & status & ")"
    Cells(8, 2) = status
End Sub

Sub OPC_ReadOnce()

    debugEvent "OPC - OPC_ReadOnce()"
    OPC_Connect
    OPC_Read
    Time_Delay (1)
    OPC_Disconnect
    status_update ("Updated Tag Values Successfully")

End Sub

Private Sub Worksheet_SelectionChange(ByVal Target As Excel.Range)

    'User input for time interval for main loop
    If (Not Intersect(Target, Range("C5:G65000")) Is Nothing) Or _
       (Not Intersect(Target, Range("A4:B8")) Is Nothing) Then
        If ThisWorkbook.connected = True Then
            MsgBox ("Please disconnect before changing any settings.")
        Exit Sub
        End If
    End If


End Sub

Answer 1

There's no such thing as bullet proof code. It's a mythical legend, a unicorn. We can (and should!) certainly try though. The first step to doing that is writing extremely legible and understandable code. You've done a pretty good job of that, but there's some room for improvement.

Readability

Use of Whitespace and Comments

It seems like a silly thing to pick on, but it really does matter. Consider this snippet from your code. It does some things right, but it looks messy.

For g = 1 To 10000
    If Not IsEmpty(Cells(4 + g, 4).value) Then
        NumberOfTags = NumberOfTags + 1
    End If
Next g

If Not ThisWorkbook.OPCServer1 Is Nothing Then       'safety
     Exit Sub

End If

Set ThisWorkbook.OPCServer1 = New OPCServer
Call ThisWorkbook.OPCServer1.Connect(Cells(4, 2))                'connect to the OPC Server
Set OPCGroup1 = ThisWorkbook.OPCServer1.OPCGroups.Add(GrpName) 'add the group

'add the 6 items
ReDim MyOPCItems(NumberOfTags)


For i = 1 To NumberOfTags
    On Error GoTo Error_TagNotFound
    If Cells(4 + i, 6) = 1 Then

The indentation is good, but you seem to arbitrarily use new lines. Sometimes no empty line, sometimes one, sometimes two. It makes for hard to read code. Hard to read code is hard to maintain code. Hard to maintain code will have bugs.

The other thing going on here is the "right aligned" comments. Which, as you can see, aren't right aligned anymore the second you copy paste them anywhere else. I'm okay with end of line comments if they're short and helpful, but forget trying to align them. It's a waste of time. If they state the obvious, remove them. If they are more than a word or two, move them to the line before.

For g = 1 To 10000
    If Not IsEmpty(Cells(4 + g, 4).value) Then
        NumberOfTags = NumberOfTags + 1
    End If
Next g

If Not ThisWorkbook.OPCServer1 Is Nothing Then
    Exit Sub
End If

'connect to the OPC Server
Set ThisWorkbook.OPCServer1 = New OPCServer
Call ThisWorkbook.OPCServer1.Connect(Cells(4, 2))   

Set OPCGroup1 = ThisWorkbook.OPCServer1.OPCGroups.Add(GrpName) 

'add the 6 items
ReDim MyOPCItems(NumberOfTags)

For i = 1 To NumberOfTags
    On Error GoTo Error_TagNotFound

Scoping (kind of)

This is not a scoping issue with your code per se, but still a readability issue.

'Dim OPCServer1 As OPCServer
Dim WithEvents OPCGroup1 As OPCGroup
Dim MyOPCItems() As OPCItem
Dim NumberOfTags As Integer
Dim ReadInterval As Double

Dont' use Dim at the module level. Use Private. Yes, they both do the same thing, but using Private is a visual indication that these variables belong to the module scope.

You should also explicitly declare the scope of your Subs & Functions. Everything is public by default, but I find it much nicer to say so. (Particularly since in VB.Net, everything is private by default. It makes for less confusion when switching between the languages.)

So,

Sub OPC_Connect()

Should be

Public Sub OPC_Connect()

Or

Private Sub OPC_Connect()

Probably the latter. As a rule of thumb, declare everything private until you prove a need to call it from outside the current module or class.

Dim i As Integer
Dim g As Integer
Dim h As Integer

Here you're declaring 3 different counter variables and using them once. Don't do this. It's clutter. Just re-use the i variable as a counter for each loop. The eye will instantly recognize it as a loop counter. Or even better, extract some methods so you don't have to stress about re-using variables. (I'll come back to that in a bit.)

That was also more of a readability issue, but this "scoping" issue is a seriously dangerous thing. I can not think of a single situation where I ever should have legitimately called just plain old Cells.

'fetch the values from the cells
For i = 1 To 6
 Values(i) = Cells(8 + i, 3)
 If Values(i) = "" Then
  Values(i) = 0
 End If
Next i

Cells the way you have used it here is equivalent to calling ActiveSheet.Cells. Almost never should we be working with the active sheet because the user can change it on us while the code is executing. Avoid activate and select by explicitly setting a variable to the range you want to work with.

The same goes for Sheets. This code

If Sheets("Setup").audible_alarms_enable_ChkBx.value = True Then

Is equivalent to calling ThisWorkbook.Sheets("Setup"). If the user clicks on another workbook, your code is going to blow up because it can't find the "Setup" worksheet.

Naming

Names should not have underscores in them unless they are an event procedure. The underscore holds this special meaning in VBA. If it's not an event procedure, just don't do it. It's confusing. Particularly when you're making use of WithEvents. (Kudos for that by the way. I don't see many programmers take advantage of that.)

Remove Dead Code

Dead code is clutter. There is dead commented out code all over the place. Burn it with fire. If you're worried about needing that code again, I highly recommend that you start using source control. I wrote a blog about using Git with VBA that should get you started.

Single Responsiblity

You've done an okay job of using subroutines, but you could do better. Remember those alphabetic counters? Extract them into well named subroutines and call them from your "main" routine.

Private Sub NotSureWhatThisActuallyDoes()
    For h = 1 To 7
        Module1.SavedThisTime(h) = False
    Next h
End Sub

A corner stone of "bullet proof" code is that you can easily grok an entire routine at one time. It's often referred to as the single responsibility principle. Each routine or function should have one and only one job. Another way of thinking about it is the "Single Screen Principle". I should never have to scroll to see more of a routine. If I have to scroll, then the routine is most certainly breaking the SRP.

Sidenote: Speaking of "calling" routines, don't actually use the Call keyword. It's archaic and only available for back ward compatibility reasons.

Here is a really good example of an opportunity to extract a method. The comment is a dead give away (and a good start on a good name).

'set up which items to be read
For i = 1 To NumberOfTags
    ServerHandles(i) = MyOPCItems(i).ServerHandle
Next i

Other things

   'free the memory
   Erase Values()
   Erase Errors()

Exit Sub

It's nice that you're cleaning up after yourself, but I seriously don't understand why you're doing this. These arrays are locally scoped. Once they go out of scope the reference count will decrement to zero and the memory will be reclaimed.

Answer 2

Options

Option Explicit  ' variables must be declared

Kudos for Option Explicit - requiring variable declaration is the very first step toward writing clean VBA code. It shouldn't need a comment though: comments should say why, not what - any VBA programmer looking at an Option statement will know what it's for. And those who don't, can Google it.

Option Base 1    ' array starts at index 1

Same thing here, the comment basically explains what the statement does. Be careful with Option Base though, as it tends to make things confusing - arrays are known to start at index 0, and collections at index 1. Using Option Base encourages lazy array declarations - a better practice is to always specify both the lower and the upper bounds of an array, and to use LBound and UBound when iterating one. Shortly put, I consider Option Base 1 a code smell all by itself.

---

Naming

The VBA naming guidelines recommend PascalCase for everything except perhaps constants, which would be YELLCASE. Regardless of whether or not you follow these guidelines, what matters the most is consistency. Here are my own guidelines:

• PascalCase for procedures (Sub, Function, Property), module names (including class names), and in general, any public identifier.
• camelCase for parameters, local variables and private fields.

Now VBA is not case-sensitive, so appropriate and meaningful naming is crucial, otherwise you will constantly be fighting your IDE.

Speaking of meaningful names:

Dim i As Integer
Dim g As Integer
Dim h As Integer

For i = 1 To NumberOfTags

A better name for i could be currentTag. Well, i is indeed commonly used as a loop counter, but in a procedure that has 3 of them, it's better to give them a meaningful name.

For g = 1 To 10000
    If Not IsEmpty(Cells(4 + g, 4).value) Then

A better name for g could be currentRow.

For h = 1 To 7
    Module1.SavedThisTime(h) = False
Next h

This one is totally cryptic. One cannot infer the meaning of h, nor why it has to be iterated from 1 to 7. Also, SavedThisTime(h) looks like it's a public field (well, an array) defined in a standard module - in other words, a global variable that could just as well be referred to as SavedThisTime(h) without the Module1 qualifier... and Module1 isn't a useful name either.

Dsmvwlng

That's right: disemvoweling. GrpName has no reason not to be called GroupName; about 20% of programming consists in writing code. The other 80% is spent reading code - might as well make it worth the while.

---

Error Handling

Error_OpcConnectionFailure:

    OPC_Disconnect
    MsgBox ("Connection Failed:" & vbNewLine & vbNewLine & "Check Your server name and ensure RSLinx is Running.")
    Exit Sub


Error_TagNotFound:

    OPC_Disconnect
    MsgBox ("Connection Failed. Check tag: " & vbNewLine & vbNewLine & row_tag_name)
    Exit Sub

The only difference between these two subroutines, is the string they're passing to the message box.

A procedure should only have exactly 1 On Error GoTo statement, and exactly 1 error-handling subroutine.

Take Sub OPC_Connect(): catching an error because a connection failed, and another because a tag was not found, smells: there should be a procedure responsible for connecting, and another for finding tags.

Instead, On Error GoTo ErrHandler, or as I like to put it, On Error GoTo CleanFail:

Public Sub Foo()
    On Error GoTo CleanFail

    '...

CleanExit:
    Exit Sub

CleanFail:
    'handle error
    Resume CleanExit
End Sub

Now, this OPC_Connect procedure handles both cases by displaying a message box; I believe in this case, knowing exactly what to do with an error should be the responsibility of the calling code - what the handler can do, is make that error as informative as possible, by raising the same error number, with new Source and Description values.

Answer 3

It's too hard to understand what is going on without being able to see the full code so I can only advise based on what you have shared. Here are some things to consider:

if you write Cells(4, 2) that means ActiveSheet.Cells(4, 2), so if you have more sheets and user changes the active sheet -> another value -> error. Same with Sheets() but not such critical as Cells()/Range().

You can freeze Excel while a macro is executing; see how

You can add an error handler to every Button_Click (UI event handler like OPC_DisconnectBtn_Click()). It wouldn't matter what is actually going wrong by any user action that starts a macro, you have the control over what the user will see.

Answer 4

Great answers. I'm a little surprised the magic number issue wasn't pointed out. A magic number is pretty much any numerical constant in your code for which the significance isn't blatantly obvious, and by blatantly, I to mean to anyone looking at your code, not just to you. There aren't many times when a number in a code fits that description, with the exception (maybe) of the lower bound of an array , the staring value of a loop counter, or an integer increment of 1 (iRow = iRow + 1 is ok). Even a number like pi may not be obvious. Just assign the number to a variable or a constant. I.e.

const PI = 3.14159265358979

In your code you have

For h = 1 To 7
   Module1.SavedThisTime(h) = False
Next h

Why 1 to 7? Why not 2 to 17? Are the 7 channels of data? Will it always be 7? Instead, do something like this:

Dim firstChannel as Long, lastChannel as Long, iChannel as Long
firstChannel = 1: lastChannel = 7
 ...
For iChannel = firstChannel to lastChannel
   Module1.SavedThisTime(iChannel) = False
Next h

Same thing with these lines:

GrpName = Cells(5, 2)
'NumberOfTags = Cells(6, 2)
NumberOfTags = 0
status_update ("Adding Tags...")
For g = 1 To 10000
    If Not IsEmpty(Cells(4 + g, 4).value) Then
        NumberOfTags = NumberOfTags + 1

What is Cells(5, 2)? Does row 5 or column 2 have special significance? Is 10000 just an arbitrarily large number or doe it represent 10000 seconds?

The magic numbers representing row and column numbers are especially troubling because someday someone might (will, according to Murphy) insert an extra row or column on the worksheet. Even if you know that has happened, it will be a chore to dig out all the places you referenced row 5 and column 2.

You could create a Range variable,

Dim groupName as Range
Set groupRange = theSheet.Cells(5, 2)

An even better solution might be to create a named range, i.e. give it a name on the worksheet. If you give the named range workbook scope (the default), then the problem of someone pulling the rug out from under you by switching the active sheet or adding a row above row 5 will go away. So instead of

GrpName = Cells(5, 2)

you would write

set groupName = Range("GroupName")

If someone changes the active worksheet or adds a row, or even changes the name of the worksheet, groupName will still point to its intended target.

While I'm on the subject of users (including yourself) changing things that break your code, whenever you can, use CodeNames instead of regular names. You can set a worksheet CodeName to something meaningful in the VBA development environment, but the user cannot change it easily like he can the worksheet name. See [this] for more about CodeNames.