I need feedback on my program
avionikeren
Posts: 64
Hi folks,
I`ve been usin the propeller for a while now on different projects, but I need some feedback on my programs to learn more. I think my programs could be more effective and smaller.
Will you have a look at the attached program and give me some hints?
The program is used to control a ROV by reading serial data from a visual basic program on a computer and refresh 2ea 4channel DAC`s (light dimming), Tristate outputs (MCZ33880 for zoom and focus), serial to hydraulic valve drivers(3ea valvepacks with 8ea proportional valves in each pack), relays and reading ADC`s on the 1st valve driver and send them to another propeller (called proplink in the code).
The program works perfect, I would just like to know how to do the same with less code.
I`ve been usin the propeller for a while now on different projects, but I need some feedback on my programs to learn more. I think my programs could be more effective and smaller.
Will you have a look at the attached program and give me some hints?
The program is used to control a ROV by reading serial data from a visual basic program on a computer and refresh 2ea 4channel DAC`s (light dimming), Tristate outputs (MCZ33880 for zoom and focus), serial to hydraulic valve drivers(3ea valvepacks with 8ea proportional valves in each pack), relays and reading ADC`s on the 1st valve driver and send them to another propeller (called proplink in the code).
The program works perfect, I would just like to know how to do the same with less code.
spin
25K
440 x 292 - 95K
Comments
Look up arrays in the Propeller Manuel (click Help in Propeller Tool, then "Propeller Manuel (pdf)") for descriptions in detail of how to use them.
Also, if you used arrays, this:
npropvalve11 := serial0.Rx 'valve 11 npropvalve12 := serial0.Rx 'valve 12 npropvalve13 := serial0.Rx 'valve 13 npropvalve14 := serial0.Rx 'valve 14 npropvalve15 := serial0.Rx 'valve 15 npropvalve16 := serial0.Rx 'valve 16 npropvalve17 := serial0.Rx 'valve 17 npropvalve18 := serial0.Rx 'valve 18 npropvalve21 := serial0.Rx 'valve 21 npropvalve22 := serial0.Rx 'valve 22 npropvalve23 := serial0.Rx 'valve 23 npropvalve24 := serial0.Rx 'valve 24 npropvalve25 := serial0.Rx 'valve 25 npropvalve26 := serial0.Rx 'valve 26 npropvalve27 := serial0.Rx 'valve 27 npropvalve28 := serial0.Rx 'valve 28 npropvalve31 := serial0.Rx 'valve 31 npropvalve32 := serial0.Rx 'valve 32 npropvalve33 := serial0.Rx 'valve 33 npropvalve34 := serial0.Rx 'valve 34 npropvalve35 := serial0.Rx 'valve 35 npropvalve36 := serial0.Rx 'valve 36 npropvalve37 := serial0.Rx 'valve 37 npropvalve38 := serial0.Rx 'valve 38 rxchecksum := serial0.Rx 'checksumCould be reduced to this:
You know, this inspires me to write a "code optimizing" tutorial. Maybe I should do that some time....
Anyway, I hope this helps.
Microcontrolled
Here is updated version of the program, please let me know if there is more I could do to the code.
The "SetValveDefaults" section is atrocious. Take advantage of the DAT block and arrange your data there, then output it by using a "repeat i from 0 to x" statement. X being the number of characters and i being the variable. You may also want to look at the data block in the Propeller Manuel.
Hope this helps,
Microcontrolled
repeat i from 1 to 24 npropvalve[i] := serial0.Rx 'valvesYour variable is defined as "byte npropvalve[24]". The index always starts at 0 (zero). So, actually you overwrite whatever comes after the array because starting with 0 the allowed range for the array with 24 elements is 0-23! This seems to be a general problem in your code ;o)
'UpdateMCZ33880 if channel == 1 mc33880cs := mc33880cs1 if channel == 2 mc33880cs := mc33880cs2 if channel == 3 mc33880cs := mc33880cs3 if channel == 4 mc33880cs := mc33880cs4 if channel == 5 mc33880cs := mc33880cs5 if channel == 6 mc33880cs := mc33880cs6Store your constants in a dat section. Then you can access the constants like an array, which removes the need of all the IF-statements: Of course channel has to start from 0 here as well.
For this:
adc11 := nadc11 adc12 := nadc12 adc21 := nadc21 adc22 := nadc22 adc31 := nadc31 adc32 := nadc32you can use the bytemove-instruction - which gives shorter and faster code, as bytemove is implemented in PASM.I`m trying to shorten the disabled code below with the first five lines, but it doesn`t work.
Is it not possible to do negative steps?
repeat m from 5 To 0 Step -1 outa[digouts[m]] := 0 if (1 << m) < nrelay nrelay := nrelay - (1 << m) outa[digouts[m]] := 1 { if nrelay > 63 'set valve defaults setvalvedefault := 1 nrelay := nrelay - 64 if nrelay > 31 'bypass outa[digouts[5]] := 1 nrelay := nrelay - 32 else outa[digouts[5]] := 0 if nrelay > 15 'relay5 outa[digouts[4]] := 1 nrelay := nrelay - 16 else outa[digouts[4]] := 0 if nrelay > 7 'relay4 outa[digouts[3]] := 1 nrelay := nrelay - 8 else outa[digouts[3]] := 0 if nrelay > 3 'relay3 outa[digouts[2]] := 1 nrelay := nrelay - 4 else outa[digouts[2]] := 0 if nrelay > 1 'relay2 outa[digouts[0]] := 1 nrelay := nrelay - 2 else outa[digouts[0]] := 0 if nrelay == 1 'relay1 outa[digouts[1]] := 1 nrelay := nrelay - 1 else outa[digouts[1]] := 0 }Let's improve "updateTristate".
1. First of all the indentation seems to be wrong in most of the if channel>1 cases. But ... to be honest I don't see a difference in the =1 and >1 versions. You added some 0 in the =1 ifs, but adding 0 digits in front of a number does not change the value at all. So, remove the if channel-statements and simply do the shiftout.
2. The other if statements can be replaced by a case statement by combining zoom and focus. You simply shift zoom 8 bits to the left and or the result with focus. The result will then contain zoom in bits 8-15 and focus in bits 0-7. If you use hex-values it looks like this: $zz_ff, where zz is the zoom value and ff is the focus value. The code then looks like:
case zoom<<8 | focus $00_01: BS2.SHIFTOUT(mc33880di,mc33880clk,%00000000_1010_0000,BS2#MSBFIRST,16) $02_01: BS2.SHIFTOUT(mc33880di,mc33880clk,%00000000_0000_0011,BS2#MSBFIRST,16) $01_00: BS2.SHIFTOUT(mc33880di,mc33880clk,%00000000_0000_1100,BS2#MSBFIRST,16) $01_02: BS2.SHIFTOUT(mc33880di,mc33880clk,%00000000_0101_0000,BS2#MSBFIRST,16) ....If you want, you can replace the $zz_ff-values with some self explanatory constants, which makes the code more readable.The updateValveXX functions can also be replaced by only one function with one parameter telling which calculation to do (1A,1B,2A or 2B) using different values defined in DAT arrays. The general function is:
return := (((newvalue * c1[ which ]) + c2[ which ]) * 25) / 10
where
DAT
c1 byte 1,-1,1,-1
c2 byte -100,100,-100,100
In this case which=0 will give you the result of 1A, which=1 will give you the result of 1B ....
(Found that during writing: 1A=2A and 1B=2B, so you can even ommit that differentiation)
updateValvepack1 and updateValvepack2 can also be put together in one function.
next to come ... ;o)
About the updatevalvepack functions, I split it in two identical because I was afraid of running the same PUB simultaneous from different cogs. Or is that totally wrong thinking?
Below, I have merged the updatevalve as you suggested, but the code below will not work as I expected. Both valve A and B goes the same way at the same time. I want to move valve A if npropvalve > 100 and move valve B if npropvalve < 100. Valve A and B is actually two solenoids pulling one valve in different directions.
PUB updateValvepack1 | d repeat d from 0 to 7 propvalveA[d] := updateValve1(npropvalve[d],0) propvalveB[d] := updateValve1(npropvalve[d],1) PUB updateValve1(newvalue, valve) if newvalue > 100 and valve == 0 return (((newvalue * c1[valve]) + c2[valve]) * 25) / 10 elseif newvalue < 100 and valve == 1 return (((newvalue * c1[valve]) + c2[valve]) * 25) / 10 else return 0 DAT c1 byte 1,-1 c2 byte -100,100Well ... there is no easy answer. It depends on what the function is doing.
If the function only works with it's parameters and with local variables and maybe read access of static global variables, there is no problem calling it from 2 different COGs.
If the function also writes to global variables or has to read dynamic global variables changed by other COGs, you surely need to know what you do.
So, for the functions in question I don't see a problem.
For your current problem I don't have an answer yet, since I have to work first ;o)
But if you invert the logic you only need the calculation once:
if (newvalue <= 100 and valve ==0) or (newvalue >= 100 and valve == 1 )
return 0
else
return ((( newvalue .......
(will have a look this evening)
PUB updateValve1(newvalue, valve) if newvalue > 100 and valve == 0 return (((newvalue * c1[valve]) + c2[valve]) * 25) / 10 elseif newvalue < 100 and valve == 1 return (((newvalue * c1[[color=red]1-[/color]valve]) + c2[[color=red]1-[/color]valve]) * 25) / 10 else return 0Forget the last post (invert of if expression). You need to invert the index of the constants to make the cod work similar to the previous versions.It`s not possible to assign a negative value to a byte.
I first made it work by changing:
DAT
c1 byte 1,-1
c2 byte -100,100
To:
DAT
c1 byte 2,0
c2 byte 0,200
And of course subtracting this in the code.
But when I was thinking about this for a while I came up with this instead:
PUB updateValvepack(firstvalve, lastvalve) | e repeat e from firstvalve to lastvalve if npropvalve[e] > 100 propvalveA[e] := ((npropvalve[e] - 100) * 25) / 10 else propvalveA[e] := 0 if npropvalve[e] < 100 propvalveB[e] := (((npropvalve[e] * -1) + 100) * 25) / 10 else propvalveB[e] := 0I have attached the complete code, I`m very happy with this now, but still let me know if you see anything else I should fix. Thanks for your help!!
actChksum := 0 actChksum ^= channel := serial[0].Rx 'channel actChksum ^= lightvalue := serial[0].Rx 'lightvalue actChksum ^= focus := serial[0].Rx 'focus actChksum ^= zoom := serial[0].Rx 'zoom actChksum ^= relay := serial[0].Rx 'relay repeat a from 0 to 23 actChksum ^= npropvalve[a] := serial[0].Rx 'valves if rxchksom == actChksum ....The variable rxvalid is not used anywhere -> remove it.
repeat b from 0 to 7 'turn of lights light[b] := 250can be done with bytefillif rx_buff[0] == 1 and rx_buff[1] == 16 and rx_buff[2] == 14 'read ADC values from pwm16 if rx_buff[3] == 59 and rx_buff[4] == 54 and rx_buff[5] == 0this can be done with the strcomp instruction.can also be done in the loop which comes a little bit later.
repeat o from 0 to 1 repeat until busy == 0 'wait in case updating default values checksum2 := (53 ^ propvalveA[8 + (o * 8)] ^ propvalveA[9 + (o * 8)] .......how aboutrepeat o from 0 to 8 step 8 ... checksum2 := (53 ^ propvalveA[8 + o] ^ propvalveA[9 + o] .......... and again you could calculate the checksum while transmitting - would remove the need to loop over all values again.repeat until (Rxi == 36) or (timeout > 10) Rxi := serial[1].rxtime(100) timeout++this kind of repeat appears 4 times, so make it a function.Happy coding ;o)
PS: I was not aware of the byte-problem because I did not use negative bytes so far with the propeller. But you could stay with -1 and -100, because there is an operator which will extend the most significant bit as if it is a signed number. ~c1[ valve ] would extend the byte correctly. Just for the records, as you solved it differently.
CON
ADC_SIZE = 6
VAR
byte adc[ ADC_SIZE ]
...
repeat x from 0 to constant( ADC_SIZE-1 )
If you use this constant anywhere in your code, you can easily extend the size of the array without taking care of the loops that loop over the array. And it makes code just a little bit more readable.
What I'd do is change the order of the serial interfaces. If you use serial[0] and serial[1] for valveControl2 you can directly use o as index without adding 2.
Instead of o*8 you can use o<<3, which is much faster than the multiplication. *4 can be replaced by <<2 and /16 can be replaced by >>4. In other words, if you have to multiply/divide by constants with a value of 2^x you better use shift-operations.
You have a lot of loops, which simply transfer bytes out of an array. Again you could make a function for that:
PUB tranfer( io, bufferAdr, byteCount ) : checksum | i checksum:=i:=0 repeat byteCount checksum^=byte[bufferAdr][i] serial[ io ].tx( byte[bufferAdr][i] )Instead of
repeat f from 0 to 6
serial[1].Tx(pwm16setValves[f])
you can then call
transfer( 1, @pwm16setValves[f], 7 )
If you do not need the checksum, then ignore the result, otherwise the checksum returned can be used.
Another implementation of transfer would maybe contain an additional parameter, which tells the function to simply send the checksum if TRUE.
I have attached some pictures of the ROV control system in case you find it interesting.
The system will be in a 20mm thick aluminium tube to withstand water down to 3000meters.
At the surface the ROV is controlled from a computer and a USB joystick (which appears as a windows gaming device). The software is written in VB.Net.
The idea of having functions is to save memory for code you'd have to repeat several times otherwise. With the right function name it also increases readability of the code. You should keep in mind that some code will run for years until you HAVE to change it again. Then you'll be happy to find easy understandable code.
BTW, stack3 is not used, so you can remove it.
Thanks for sharing the photos! How are the commands transfered to the ROV?