de-bloating
Erlend
Posts: 612
First of all - I really do like Spin. Converting to this language and the architecture of prop takes some time, but is is fun!
I believe in well documented and leglibly written code. In my opinion a non-spin should be able to read it and understand it. Which, I guess, makes it bloated (in this forum's opinion).
I have written this little object (stolen a bit from others) which I am happy with. It is a step on my way to a solution where all I/O goes on in separate cogs, sparing the Main code for having to think about the housekeeping.
It is also a step towards getting to know Spin.
Below is the code. Please give me feedback, hints, tips, threats, whatever - to help me getting better.
The parent code is so simple and obvious that I have not included it.
Erlend
I believe in well documented and leglibly written code. In my opinion a non-spin should be able to read it and understand it. Which, I guess, makes it bloated (in this forum's opinion).
I have written this little object (stolen a bit from others) which I am happy with. It is a step on my way to a solution where all I/O goes on in separate cogs, sparing the Main code for having to think about the housekeeping.
It is also a step towards getting to know Spin.
Below is the code. Please give me feedback, hints, tips, threats, whatever - to help me getting better.
{
RFIDspin - Spin only, educational-style code (for my own education) of a RFID reader.
--------------------------------------------------------------------------------------
This object reads the Parallax RFID card reader, converts the 10byte sequence code to a string,
compares this string with a number of string-format codes in the DAT of the parent object, return-writes which number in the sequence had a match (0 if none),
as well as the read string-formatted code itself.
Optionally resets the number-match to zero after a time delay.
Optionally displays the codes to the pst (if tpeDisplay=1).
I have taken most of the Read code from RFID_Demo.spin by Gavin Garner, thanks.
}
CON
_clkmode = xtal1 + pll16x 'Standard clock mode * crystal frequency = 80 MHz
_xinfreq = 5_000_000
VAR
BYTE cog, dataRFID[12], textRFID[11]
LONG stack[32]
OBJ
Debug : "FullDuplexSerial" 'Don't really know what's best - the pst or the fds?
PUB start(serPIN, enaPIN, strngCodes{ptr}, noCodes, secTimeout, tpeDisplay, nmbMatch{ptr}, strngRFID{ptr})
cog:= cognew(RFID(serPIN, enaPIN, strngCodes{ptr}, noCodes, secTimeout, tpeDisplay, nmbMatch, strngRFID{ptr}), @stack[0])
PUB stop
if (cog) 'if running
cogstop(cog - 1) 'stop
cog := 0
PUB RFID(serPIN, enaPIN, strngCodes{ptr}, noCodes, secTimeout, tpeDisplay, nmbMatch, strngRFID{ptr}) | deltaT, time, i, j, value, match
'Prepare terminal procedure
'---------------------------
IF tpeDisplay == 1 'Only if display is wanted by the parent object does the terminal start
DEBUG.start(31, 30, 0, 9600) 'Slow baud because I hoped the terminal comms would behave
waitcnt(4*clkfreq + cnt) 'Give the pst a break
Debug.str(string("Ready to read card... ")) 'This only shows up with a 4 sek wait or more - some mystery in the comms I guess
repeat
'Read procedure
'--------------
dira[serPIN]~ 'Set direction of serPIN to be an input
dira[enaPIN]~~ 'Set direction of enaPIN to be an output
deltaT:=clkfreq/2400 'Set deltaT to 1/2400th of a second for 2400bps "Baud" rate
outa[enaPIN]~ 'Enable the RFID reader
repeat i from 0 to 11 'Fill in the 12 byte arrays with data sent from RFID reader
waitpeq(1 << serPIN,|< serPIN,0) 'Wait for a high-to-low signal on RFID's serPIN pin, which
waitpeq(0 << serPIN,|< serPIN,0) 'signals the start of the transfer of each packet of 10 data bits plus head and tail
time:=cnt 'Record the counter value at start of each transmission packet
waitcnt(time+=deltaT+deltaT/2) 'Skip the start bit (always zero) and center on the 2nd bit
repeat 8 'Gather 8 bits for each byte of RFID's data
dataRFID[i]:=dataRFID[i]<<1 + ina[serPIN] 'Shift dataRFID bits left and add current bit (state of serPIN)
waitcnt(time+=deltaT) 'Pause for 1/2400th of a second (2400 bits per second)
dataRFID[i]><=8 'Reverse the order of the bits (RFID scanner sends LSB first)
outa[enaPIN]~~ 'Disable the RFID reader
repeat i from 0 to 10 'For each byte, except first and last,
textRFID[i] := dataRFID[i+1] '-copy to local string variable
BYTE [strngRFID] [i] := dataRFID[i+1] '-and copy to the parent object
textRFID[10] := 0 'Convert to zero-terminated string
BYTE [strngRFID] [10] := 0 'Convert to zero-terminated string
'Find match procedure
'-------------------
match := 0
repeat j from 0 to noCodes - 1 'For each of the codes in DAT block of strings,
IF STRCOMP(@textRFID, strngCodes+11*j) '-compare read RFID with the j'th code
match := j+1 'The number of the one code which matches
LONG [nmbMatch] := match 'Copy the match number to the parent object
'Display procedure
'-----------------
IF tpeDisplay == 1 'Only if display is wanted by the parent object
Debug.tx(13)
Debug.str(string("Card number: "))
Debug.str(@textRFID) 'Print the read card code
Debug.tx(13)
Debug.str(string("Match: "))
Debug.dec(match) 'Print the match number in the row of codes in the parent object, zero if none
'Timeout reset procedure
'----------------------
'I've no idea how to code a timeout function. Because of the waitpeq in the Read procedure there will be no idle activity.
'Probably have to change the Read procedure to allow a sidestep to check a counter, but haven't succeeded.
WAITCNT(clkfreq + cnt) 'Read card again after one second
PRI none
DAT
'nothing
The parent code is so simple and obvious that I have not included it.
Erlend
Comments
2. I find that the most useful documentation is at the function header level. To that end, I'd make sure to document the parameters to each function. I'm partial to the Javadoc format of documentation.
3. I usually run debug serial at 230400 baud, and it works fine.
4. Your stop method is missing the call to debug.stop.
5. Your start method is missing a call to stop (what happens if start is called twice in a row?)
6. Why have the one second delay at the end of the loop? I'd just get ready to read the next card.
7. The PRI and DAT labels are not needed if they do not have any content.
8. From an organizational point of view, I don't think it's that useful to have the debug code in with the RFID code. That prevents you from having any other USB communications, since the bus is taken. I think a better RFID object would worry about interfacing to the reader, and some glue logic would attach that to the USB serial port for the output.
Thanks for the feedback.
1. I will do that. Is it only due to good coding practice conventions, or does it actually improve the code? The only thing I can see is that it makes it impossible for the parent object to call the RFID method direct.
2. Way back when I did some software development, I began by writing the User Guide, got it in line with user expectations, and only then started coding. I think that's a good practice. Nowadays I am just programming for fun, so the user guide is in my brain only. As for commenting I agree with you, except when education is also the purpose - which requires generous commenting.
3. The baud rate seems to make no difference. By experiment I have found that a 4 sec wait is required between initializing and writing. It should not be that way.
4. Will do.
5. Will do.
6. I inherited that part of the code, so I just assumed it was to 'debounce' a card read, but maybe the card reader takes care of that. Will test.
7. Just left it in (from the file template) in case I find I want to put something there. (E.g. pt.1)
8. In principle I agree, but keeping it in allows an easy way to debug the RFID card hardware as well as determining the code of the cards - without needing to write any amount of Main code. For normal use the tpeDisplay should be set to 0. It's a 'messy' versus 'handy' decision.
After learning more about Spin byte functions, I believe I can replace the
repeat i from 0 to 10
textRFID := dataRFID[i+1]
BYTE [strngRFID] := dataRFID[i+1]
with BYTEMOVE commands - avoiding the loop.
Erlend
Erlend
3. The delay is the time that it takes you to switch from downloading the code to opening a serial window. For my work with Spin, I use a simple script to automate the code downloading and opening a terminal, so the delay can be very short (or none at all? I don't remember).
8. Usually, in cases like this, I like to make a "main" type function at the top of the program. That relies on the handy feature of Spin files: whatever file you load, the first function is the initial function. I've used this to add tests to a "driver" type file: the tests are all created and run from the first function, which is only called when the object is the top level. When it's not, then that method is ignored.
3. The weird thing is that if I set the wait to 2 sec or thereabout, then download and switch to terminal fast, I have to wait a second before something prints - and then the first print line has been skipped - even when I 'was there in time'. If the wait is increased to 4 sec, the first line does not get skipped.
8. I know about the whoever-comes-first behavior, but I am not sure I understand you- do you mean to have a PUB debug code at the top, and then comment it out when not needed?
Erlend
PUB Debug serial.Start(31, 30, 0, 9600) Start RunTestsFunction(100) PUB Start cognew(...) PRI RunTestsFunction(loop_number) if(a != b) serial.str(string("Error!!!")) else serial.str(string("Ok."))The Debug method is automatically "called" whenever the Spin object is run as the top level. When it's not top level, it's simply baggage. To save space you could comment it out if you like, or use BST which has unused function elimination (I went for the latter).
Erlend
I even managed to find a way to work around the waitpeq to fit in a timeout-check loop. A question though; wouldn't using cnt fail if it happens to flip over just after count := cnt ? Is there any other way?
{ RFIDspin - Spin only, educational-style code (for my own education) of a RFID reader. -------------------------------------------------------------------------------------- This object reads the Parallax RFID card reader, converts the 12byte sequence code to a string of 10, head and tail removed, compares this string with a number of string-format codes in the DAT of the parent object, return-writes which number in the sequence had a match (0 if none). Retur-write the string-formatted code regardless of match or not. Optionally resets the number-match to zero after a time delay. Optionally displays the codes to the pst (if tpeDisplay=1). Should only be used for debugging or when obtaining card codes, because it claims the usb comms channel. I have taken some of the Read code ideas from RFID_Demo.spin by Gavin Garner, thanks. } CON _clkmode = xtal1 + pll16x 'Standard clock mode * crystal frequency = 80 MHz _xinfreq = 5_000_000 VAR BYTE cog, dataRFID[12], textRFID[11] LONG stack[32] OBJ Debug : "FullDuplexSerial" 'Don't really know what's best - the pst or the fds? PUB start(serPIN, enaPIN, strngCodes{ptr}, noCodes, secTimeout, tpeDisplay, nmbMatch{ptr}, strngRFID{ptr}) stop 'If running already, stop cog:= cognew(RFID(serPIN, enaPIN, strngCodes{ptr}, noCodes, secTimeout, tpeDisplay, nmbMatch, strngRFID{ptr}), @stack[0]) PUB stop if (cog) 'If running cogstop(cog - 1) 'stop cog := 0 debug.stop 'and ask debug cog to stop PRI RFID(serPIN, enaPIN, strngCodes{ptr}, noCodes, secTimeout, tpeDisplay, nmbMatch, strngRFID{ptr}) | deltaT, time, i, j, value, match, count 'PRI instead of PUB to prevent parent code to call .RFID direct (silly) 'Prepare terminal procedure '--------------------------- IF tpeDisplay == 1 'Only if display is wanted by the parent object does the terminal start DEBUG.start(31, 30, 0, 9600) 'Slow baud because I hoped the terminal comms would behave waitcnt(4*clkfreq + cnt) 'Give the pst a break Debug.str(string("Ready to read card... ")) 'This only shows up with a 4 sek wait or more - some mystery in the comms I guess count := 10*cnt 'Allow Read 10 sec to read card to avoid Timeout to trigger first time repeat 'Infinitely do 'Read procedure '-------------- dira[serPIN]~ 'Set direction of serPIN to be an input dira[enaPIN]~~ 'Set direction of enaPIN to be an output deltaT:=clkfreq/2400 'Set deltaT to 1/2400th of a second for 2400bps "Baud" rate outa[enaPIN]~ 'Enable the RFID reader repeat i from 0 to 11 'Fill in the 12 byte arrays with data sent from RFID reader repeat while i == 0 AND (ina[serPIN] == 1) 'Detour for Timeout procedure, while card has not yet started outputting if cnt > count + secTimeout*clkfreq AND match > 0 'If time is exceeded and match not alredy reset match := 0 'Reset local LONG [nmbMatch] := 0 'Reset global IF tpeDisplay == 1 'If to display, announce timeout Debug.str(string(" Timeout ")) '-continue reading if i > 0 'Waitpeq for the first signal is taken care of by Timeout loop waitpeq(1 << serPIN,|< serPIN,0) 'Wait for a high-to-low signal on RFID's serPIN pin, which waitpeq(0 << serPIN,|< serPIN,0) 'signals the start of the transfer of each packet of 10 data bits plus head and tail time:=cnt 'Record the counter value at start of each transmission packet waitcnt(time+=deltaT+deltaT/2) 'Skip the start bit (always zero) and center on the 2nd bit repeat 8 'Gather 8 bits for each byte of RFID's data dataRFID[i]:=dataRFID[i]<<1 + ina[serPIN] 'Shift dataRFID bits left and add current bit (state of serPIN) waitcnt(time+=deltaT) 'Pause for 1/2400th of a second (2400 bits per second) dataRFID[i]><=8 'Reverse the order of the bits (RFID scanner sends LSB first) outa[enaPIN]~~ 'Disable the RFID reader BYTEMOVE(@textRFID, @dataRFID+1, 10) 'Copy second to tenth byte into new variable to get rid of the header and tail BYTEMOVE(strngRFID, @dataRFID+1, 10) 'Copy same to parent object variable, i.e. report RFID read textRFID[10] := 0 'Make it a zero-terminated string BYTE [strngRFID] [10] := 0 'Make it a zero-terminated string 'Find match procedure '------------------- match := 0 repeat j from 0 to noCodes - 1 'For each of the codes in DAT block of strings, IF STRCOMP(@textRFID, strngCodes+11*j) '-compare read RFID with the j'th code match := j+1 'Set the number of the one code which matches LONG [nmbMatch] := match 'Copy the match number to the parent object count := cnt 'After successful read and match, reset timeout 'Display procedure '----------------- IF tpeDisplay == 1 'Only if display is wanted by the parent object Debug.tx(13) Debug.str(string("Card number: ")) Debug.str(@textRFID) 'Print the read card code Debug.tx(13) Debug.str(string("Match: ")) Debug.dec(match) 'Print the match number in the row of codes in the parent object, zero if none WAITCNT(clkfreq + cnt) 'Wait one second to read the card again - to 'debounce'Erlend
I assume you mean on the line time:=cnt? No, there is not a bug there. If CNT were to rollover after the assignment to time and before the waitcnt, then variable "time" would roll over as well when deltaT is added to it.
There is, however, a bug on the first line I highlighted: 10*cnt is not 10 seconds. What you want is 10*CLKFREQ + CNT.
This expression suffers from overflow. Use the delta between current time and time reference, e.g. cnt - count, and compare it against the timeout (@80MHz try to stay below 26.8sec for delta and timeout, i.e. POSX cycles). Also, note that match is a local variable and therefore not initialised (result being the only exception). IOW you may use a random value here.
Next one out is a RTC reader object, same principle and style.
{ RFIDspin - Spin only, educational-style code (for my own education) of a RFID reader. -------------------------------------------------------------------------------------- This object reads the Parallax RFID card reader, converts the 12byte sequence code to a string of 10, head and tail removed, compares this string with a number of string-format codes in the DAT of the parent object, return-writes which number in the sequence had a match (0 if none). Retur-write the string-formatted code regardless of match or not. Optionally resets the number-match to zero after a time delay. Optionally displays the codes to the pst (if tpeDisplay=1). Should only be used for debugging or when obtaining card codes, because it claims the usb comms channel. I have taken some of the Read code ideas from RFID_Demo.spin by Gavin Garner, thanks. Naming convention: Variable naming: stn: String, int: Integer, flt: Floating point, cmp: Composite, chr: Character, dat: Consecutive bytes containing data sets Reserved words are always in capitals Called objects are three letter lower case PUB and PRI have lower case names. Local variables have no convention. } CON _clkmode = xtal1 + pll16x 'Standard clock mode * crystal frequency = 80 MHz _xinfreq = 5_000_000 VAR BYTE intCog, datRFID[12], txtRFID[11] LONG stack[32] OBJ dbg : "FullDuplexSerial" 'Don't really know what's best - the pst or the fds? PUB start(intSerPIN, intEnaPIN, stnCodes{ptr}, intNumberofCodes, intSecTimeout, intTypeDisplay, intNumberMatch{ptr}, stnRFID{ptr}) stop 'If running already, stop intCog:= COGNEW(rfid(intSerPIN, intEnaPIN, stnCodes{ptr}, intNumberofCodes, intSecTimeout, intTypeDisplay, intNumberMatch, stnRFID{ptr}), @stack[0]) PUB stop if (intCog) 'If running COGSTOP(intCog - 1) 'stop intCog := 0 dbg.stop 'and ask dbg intCog to stop PRI rfid(intSerPIN, intEnaPIN, stnCodes{ptr}, intNumberofCodes, intSecTimeout, intTypeDisplay, intNumberMatch, stnRFID{ptr}) | deltaT, time, i, j, value, match, count 'PRI instead of PUB to prevent parent code to call .RFID direct (silly) match := 0 'Prepare terminal procedure '--------------------------- IF intTypeDisplay == 1 'Only if display is wanted by the parent object does the terminal start dbg.start(31, 30, 0, 115200) 'Slow baud because I hoped the terminal comms would behave WAITCNT(4*CLKFREQ + CNT) 'Give the pst a break dbg.str(STRING("Ready to read card... ")) 'This only shows up with a 4 sek wait or more - some mystery in the comms I guess count := 10*CLKFREQ + CNT 'Allow Read 10 sec to read card to avoid Timeout to trigger first time repeat 'Infinitely do 'Read procedure '-------------- DIRA[intSerPIN]~ 'Set direction of intSerPIN to be an input DIRA[intEnaPIN]~~ 'Set direction of intEnaPIN to be an output deltaT:=CLKFREQ/2400 'Set deltaT to 1/2400th of a second for 2400bps "Baud" rate OUTA[intEnaPIN]~ 'Enable the RFID reader repeat i from 0 to 11 'Fill in the 12 byte arrays with data sent from RFID reader repeat while i == 0 AND (INA[intSerPIN] == 1) 'Detour for Timeout procedure, while card has not yet started outputting if CNT - count > intSecTimeout*CLKFREQ AND match > 0 'If time is exceeded and match not alredy reset match := 0 'Reset local LONG [intNumberMatch] := 0 'Reset global IF intTypeDisplay == 1 'If to display, announce timeout dbg.str(STRING(" Timeout ")) '-continue reading if i > 0 'Waitpeq for the first signal is taken care of by Timeout loop WAITPEQ(1 << intSerPIN,|< intSerPIN,0) 'Wait for a high-to-low signal on RFID's intSerPIN pin, which WAITPEQ(0 << intSerPIN,|< intSerPIN,0) 'signals the start of the transfer of each packet of 10 data bits plus head and tail time:=CNT 'Record the counter value at start of each transmission packet WAITCNT(time+=deltaT+deltaT/2) 'Skip the start bit (always zero) and center on the 2nd bit repeat 8 'Gather 8 bits for each byte of RFID's data datRFID[i]:=datRFID[i]<<1 + INA[intSerPIN] 'Shift datRFID bits left and add current bit (state of intSerPIN) WAITCNT(time+=deltaT) 'Pause for 1/2400th of a second (2400 bits per second) datRFID[i]><=8 'Reverse the order of the bits (RFID scanner sends LSB first) OUTA[intEnaPIN]~~ 'Disable the RFID reader BYTEMOVE(@txtRFID, @datRFID+1, 10) 'Copy second to tenth byte into new variable to get rid of the header and tail BYTEMOVE(stnRFID, @datRFID+1, 10) 'Copy same to parent object variable, i.e. report RFID read txtRFID[10] := 0 'Make it a zero-termINAted STRING BYTE [stnRFID] [10] := 0 'Make it a zero-termINAted STRING 'Find match procedure '------------------- match := 0 repeat j from 0 to intNumberofCodes - 1 'For each of the codes in DAT block of STRINGs, IF STRCOMP(@txtRFID, stnCodes+11*j) '-compare read RFID with the j'th code match := j+1 'Set the number of the one code which matches LONG [intNumberMatch] := match 'Copy the match number to the parent object count := CNT 'After successful read and match, reset timeout 'Display procedure '----------------- IF intTypeDisplay == 1 'Only if display is wanted by the parent object dbg.tx(13) dbg.str(STRING("Card number: ")) dbg.str(@txtRFID) 'Print the read card code dbg.tx(13) dbg.str(STRING("Match: ")) dbg.dec(match) 'Print the match number in the row of codes in the parent object, zero if none WAITCNT(CLKFREQ + CNT) 'Wait one second to read the card again - to 'debounce'Erlend
Erlend
http://www.parallax.com/tabid/832/Default.aspx
Everything that has been printed ( or web'ed) on Propeller I have read, except Harprit's PASM book, which I will order tonight.
There is nothing left but to read and write code.
Erlend