A better way

[pcleats]

The code I have listed below is used to check the status of the battery. It works ok. I am just wondering if there is a better way to do it. Maybe roll it into a function or something like that.

Code: Select all

' definitions for measuring Battery Parameters
Private Const VoltagePin as Byte = 13
Private Const CurrentPin as Byte = 14
Dim Voltage As Single
Dim OutVolt as Single
Dim Current as Single
Dim OutCurrent as Single

' Get the voltage measurement

Private Sub ReadVoltage()
	Call Getadc(VoltagePin, Voltage)
	OutVolt = Voltage * 14.75
	Call BatteryStatus
	If ((Temp = 1) and (Outvolt > 11.70) _
			and &#40;OutVolt < 13.77&#41;&#41; then
		Call ClearLine&#40;&#41;
	End If
	call Delay&#40;0.25&#41;
End sub

' Get the current measurement

Private Sub ReadCurrent&#40;&#41;
	Call Getadc&#40;CurrentPin, Current&#41;
	OutCurrent = Current * 2.188
	call Delay&#40;0.25&#41;
End Sub

' Check the current status of the batteries 

Private Sub BatteryStatus&#40;&#41;
	If Outvolt < 11.70 Then 
		call LowBattery
	elseif OutVolt >13.77 then
		call BatteryCharging
	End If
End Sub

Private Sub LowBattery&#40;&#41;
	Call LCD_DisplayStrAt&#40;"Low Batt", 2, 1&#41;
	Temp = 1
End Sub

Private Sub BatteryCharging&#40;&#41;
	Call LCD_DisplayStrAt&#40;"Charging", 2, 1&#41;
	Temp = 1
End Sub

'Clear line 2 of the LCD display

Private Sub ClearLine&#40;&#41;
Dim i as Byte
	i = 10
	Do While &#40;i > 0&#41;
		Call LCD_DisplayStrAt&#40;" ", 2, i&#41;
		i = i -1
	Loop
	Temp = 0
End Sub

I know it looks ulgy, but any help cleaning it up would be GREAT!.

Thanks
Patrick

[dkinzer]

First off, note that I reformatted your post, using the code tags. This makes the code much easier to read because it presents it in monospace font and preserves whitespace indentation. You can do this in your posts in one of several ways. Perhaps the simplest way is to paste your code in the post, highlight the code and then click the "Code" button (below the Subject edit box). This action adds the words "code" and "/code" enclosed in square brackets at the beginning and end, respectively, of the highlighted text. You can also click the "Code" button first, paste your code and then click the "Code" button again. Finally, you can type the code tags yourself.

As to your request for suggestions, it is not clear exactly what your goal is. It is common to encapsulate functionality like this in a module (a separate .bas file). However, all of the routines that you have shown are private. A module must have at least 1 public entity in order to be useful.

Secondly, it looks like the 'temp' variable is being used as a flag to cause or suppress updating the display. If that is the case, surely a better name can be divined that is suggestive of its purpose. You might be able to restructure the code so that such a flag is not required at all. If it can be done without other undesirable side effects (e.g. difficult to understand, too slow, too large, etc.), it is advisable to do so.

Thirdly, it would be an improvement to replace the use of "magic numbers" like 2.188 and 11.70 with defined constants. If the constant names are suggestive of their role, it makes the code easier to read. Moreover, it makes the code easier to maintain, especially when the magic number is used in multiple places, e.g. your low/high battery voltage thresholds.

Unless your input signals for the current/voltage have external filtering (or even if you do), you might want to add a moving average calculation. One way to do this without much fuss is to use a queue to hold the readings.

[pcleats]

As to your request for suggestions, it is not clear exactly what your goal is. It is common to encapsulate functionality like this in a module (a separate .bas file).

What I am trying to do is monitor the status of the batteries in the video distribution box that is placed 500 to 1000 ft away from my surveillance van. The lcd display is in the van and data is sent to it see attached schematic.

However, all of the routines that you have shown are private. A module must have at least 1 public entity in order to be useful.

Secondly, it looks like the 'temp' variable is being used as a flag to cause or suppress updating the display. If that is the case, surely a better name can be divined that is suggestive of its purpose.

Bad name choice for the variable this has been fixed. The flag is used to determine if the 2nd line of the display needs to be cleared.

You might be able to restructure the code so that such a flag is not required at all. If it can be done without other undesirable side effects (e.g. difficult to understand, too slow, too large, etc.), it is advisable to do so.

Thirdly, it would be an improvement to replace the use of "magic numbers" like 2.188 and 11.70 with defined constants. If the constant names are suggestive of their role, it makes the code easier to read. Moreover, it makes the code easier to maintain, especially when the magic number is used in multiple places, e.g. your low/high battery voltage thresholds.

I have changed them to defined constants. The numbers are used in conjunction with the voltage deviders I have to ensure that the voltage at the ZX-24 pins do not exceed 5 volts.

Unless your input signals for the current/voltage have external filtering (or even if you do), you might want to add a moving average calculation. One way to do this without much fuss is to use a queue to hold the readings.

What I need is the actual voltage and current reading at any given moment at least for now, until I know how long the batteries will run the items connected to it. Also I don't know how to do a moving average calculation.

[dkinzer]

In your schematic, D2 is shown backwards, assuming that you want it to illuminate when the relay is activated to interrupt the power.

I don't know how to do a moving average calculation.

A moving average is simply an average over a set of values that changes over time. For example, you might average the last 10 readings taken. This would be a 10-point moving average.

To implement a moving average you need a way to store the last N readings, adding a new reading and removing the oldest reading on each iteration. You can do this with an array that holds N data points but you also need a way to add the new and drop the oldest one. The straightforward but inefficient way to do this is to move all of the existing entries to the next higher index and always add the newest reading at the beginning of the array. A more efficient but more complicated way is to maintain an index where the new entry should be added, wrapping it back to the beginning when it reaches the end.

You also need a counter to indicate how many entries are in the list. This helps in getting the moving average "primed". The first N-1 readings will result in a moving average over a smaller number of readings.

Since a queue already embodies the logic to wrap the insertion and deletion indices, it can be simpler to use than to write your own code. Although queues are probably most often used with Byte data, they work just as well with multi-byte data like Single and Integer values. One caveat, of course, is that GetQueueCount() always returns the number of bytes in the queue. If you're using the queue for multi-byte data you have to divide that return value by the data element size in order to determine how many elements are in the queue.

The code for my Reflow Toaster Oven Controller uses a queue to implement a moving average for the oven temperature readings.