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 declaredOption Base 1    ' array starts at index 1'Dim OPCServer1 As OPCServerDim WithEvents OPCGroup1 As OPCGroupDim MyOPCItems() As OPCItemDim NumberOfTags As IntegerDim ReadInterval As DoubleSub 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 subError_OpcConnectionFailure:    OPC_Disconnect    MsgBox (Connection Failed: & vbNewLine & vbNewLine & Check Your server name and ensure RSLinx is Running.)    Exit SubError_TagNotFound:    OPC_Disconnect    MsgBox (Connection Failed. Check tag:  & vbNewLine & vbNewLine & row_tag_name)    Exit SubEnd SubSub 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 SubSub OPC_RefreshServer()        debugEvent OPC - OPC_RefreshServer()        OPC_Disconnect        Time_Delay (1)        OPC_ConnectEnd SubSub 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 SubError_TagRead:    OPC_Disconnect    MsgBox (Tag Read Error. & vbNewLine & Please reconnect to server.)    Exit SubEnd SubSub 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 SubSub ReadBtn_Click()    debugEvent OPC - ReadBtn_Click()    OPC_ReadEnd SubSub 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 = FalseExit SubError_Reconnect:    If ThisWorkbook.OPCServer1 Is Nothing Then 'safety        If Sheets(Setup).AutoReconnect_ChkBx.value = True Then            OPC_RefreshServer        Else            Exit Sub        End If    End IfEnd SubPrivate Sub OPC_ConnectBtn_Click()    debugEvent OPC - OPC_ConnectBtn_Click()    OPC_Connect    OPC_LoopReadEnd SubSub OPC_DisconnectBtn_Click()    debugEvent OPC - OPC_DisconnectBtn_Click    OPC_DisconnectEnd SubSub OPC_Read_Once_Btn_Click()    debugEvent OPC - OPC_Read_Once_Btn_Click()    OPC_ReadOnceEnd SubSub status_update(status As String)    debugEvent OPC - status_update( & status & )    Cells(8, 2) = statusEnd SubSub OPC_ReadOnce()    debugEvent OPC - OPC_ReadOnce()    OPC_Connect    OPC_Read    Time_Delay (1)    OPC_Disconnect    status_update (Updated Tag Values Successfully)End SubPrivate 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 IfEnd Sub
Building an OPC client in Excel
beginner;vba;excel
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.ReadabilityUse of Whitespace and CommentsIt 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 IfNext gIf Not ThisWorkbook.OPCServer1 Is Nothing Then       'safety     Exit SubEnd IfSet ThisWorkbook.OPCServer1 = New OPCServerCall ThisWorkbook.OPCServer1.Connect(Cells(4, 2))                'connect to the OPC ServerSet OPCGroup1 = ThisWorkbook.OPCServer1.OPCGroups.Add(GrpName) 'add the group'add the 6 itemsReDim MyOPCItems(NumberOfTags)For i = 1 To NumberOfTags    On Error GoTo Error_TagNotFound    If Cells(4 + i, 6) = 1 ThenThe 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 IfNext gIf Not ThisWorkbook.OPCServer1 Is Nothing Then    Exit SubEnd If'connect to the OPC ServerSet ThisWorkbook.OPCServer1 = New OPCServerCall ThisWorkbook.OPCServer1.Connect(Cells(4, 2))   Set OPCGroup1 = ThisWorkbook.OPCServer1.OPCGroups.Add(GrpName) 'add the 6 itemsReDim MyOPCItems(NumberOfTags)For i = 1 To NumberOfTags    On Error GoTo Error_TagNotFoundScoping (kind of)This is not a scoping issue with your code per se, but still a readability issue.'Dim OPCServer1 As OPCServerDim WithEvents OPCGroup1 As OPCGroupDim MyOPCItems() As OPCItemDim NumberOfTags As IntegerDim ReadInterval As DoubleDont' 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 IntegerDim g As IntegerDim h As IntegerHere 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 cellsFor i = 1 To 6 Values(i) = Cells(8 + i, 3) If Values(i) =  Then  Values(i) = 0 End IfNext iCells 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 codeIf Sheets(Setup).audible_alarms_enable_ChkBx.value = True ThenIs 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.NamingNames 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 CodeDead 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 ResponsiblityYou'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 hEnd SubA 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 readFor i = 1 To NumberOfTags    ServerHandles(i) = MyOPCItems(i).ServerHandleNext iOther things   'free the memory   Erase Values()   Erase Errors()Exit SubIt'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.