First code - some bug included?

user82 · 2010-12-26 03:49

Hi!

I recently got my propeller and wrote a little "Prime number calculator"

Problem is, it does not work for some reasons, so perhaps one of you could give me a litttle hint what i messed up.

the comments are in c style, but just for the forum here not in the code

var
long primglobal 
long primcog[7]
long lastwritten[7]
long stack1[128]
long stack2[128]
long stack3[128]
long stack4[128]
long stack5[128]
long stack6[128]
long stack7[128]

obj
 sd: "fsrw"
 
PUB main
primglobal:=0 //start searching at Zero
sd.mount(0)
sd.popen(string("primes.txt"), "w")

cognew(worker, @stack1)
cognew(worker, @stack2)
cognew(worker, @stack3)
cognew(worker, @stack4)
cognew(worker, @stack5)
cognew(worker, @stack6)
cognew(worker, @stack7)


repeat while(primglobal<=60) //dump the found primes to sd until 60 is reached
  if(primcog[1]<>lastwritten[1])
    sd.sddec(primcog[1])
    sd.pputc(10)
    lastwritten[1]:=primcog[1]
  if(primcog[2]<>lastwritten[2])
    sd.sddec(primcog[2])
    sd.pputc(10)
    lastwritten[2]:=primcog[2]
  if(primcog[3]<>lastwritten[3])
    sd.sddec(primcog[3])
    sd.pputc(10)
    lastwritten[3]:=primcog[3]
  if(primcog[4]<>lastwritten[4])
    sd.sddec(primcog[4])
    sd.pputc(10)
    lastwritten[4]:=primcog[4]
  if(primcog[5]<>lastwritten[5])
    sd.sddec(primcog[5])
    sd.pputc(10)
    lastwritten[5]:=primcog[5]
  if(primcog[6]<>lastwritten[6])
    sd.sddec(primcog[6])
    sd.pputc(10)
    lastwritten[6]:=primcog[6]
  if(primcog[7]<>lastwritten[7])
    sd.sddec(primcog[7])
    sd.pputc(10)
    lastwritten[7]:=primcog[7]

cogstop(1)
cogstop(2)
cogstop(3)
cogstop(4)
cogstop(5)
cogstop(6)
cogstop(7)

sd.pclose

dira[17]:=1 //blink some led with gnd @pin17 and VCC  @pin16 when done
dira[16]:=1
  repeat
    outa[16] := 1
    waitcnt(clkfreq/2 + cnt)
    outa[16] := 0
    waitcnt(clkfreq/2 + cnt)

PUB worker | primlocal,x,ptrue,helper1
 primlocal:=primglobal
 primglobal:=primglobal+1
 helper1:=primlocal/2+1
 x:=2
 ptrue:=1

 repeat x from 2 to helper1 //if all divisions are not "working", its a prime
  if(primlocal//x==0)
    ptrue:=0
    quit

repeat while(primcog[cogid]<>lastwritten[cogid]) //wait until the last prime is dumped to sd card
  waitcnt(clkfreq/128 + cnt)

if(ptrue==1)
  primcog[cogid]:=primlocal //write to global ram for being dumped to sd card by cog0

MagIO2 · 2010-12-26 05:18

1st of all, your main program accesses primcog[7] and lastwritten[7], but these do not exist. As you definde primcog and lastwritten as an array of 7 elements, the index range is from 0 to 6!

You access global variables within several COGs. This is something you should always use the locks for! Imagine what happens during startup:
COG0 runs the main program
COG1 is started - this instruction does not wait until COG1 is running, it's immediately processing the next COGNEW
COG2 is started
Now let's only have a look at those 2 prime-workers. The difference with which they run is only one SPIN bytecode.
COG1 reads primglobal (content 0) - COG2 is still loading
COG1 reads promglobal again (2nd line of code/ content 0) - COG2 reads primglobal (content 0 as COG1 did not write yet)
COG1 adds 1 - COG2 reads primglobal again (2nd line of code / content 0 - still not written)
COG1 writes the result to primglobal (writeback 1) - COG2 adds 1
COG1 reads primlocal - COG2 writes the result to primglobal ( writeback 1 )
....

This means that several COGs will start with processing the same number.

What will the worker do after checking the first number? It will stop, as the worker does not have an outer loop to fetch the next number and check that.

So, my advice: read the section about the lock instructions and check the chapter in the Lab (see sticky). You need to make sure that only one COG is accessing primglobal at a time. (Which means during the whole read, modify, write)
You also need better synchronisation with the main COG which writes the numbers to SD-card. Better than the waitcnt you have in place, as this will in best case waste time or in worst case (for example if SD card needs to long to write) will loose data.

As the runtime of the check will change you might need some code which sorts the result.

JonnyMac · 2010-12-26 09:08

You don't have enough cogs for the code you're trying to run. The main code is consuming a cog, FSRW is consuming a cog, and then you attempt to load seven more, even though there are only six left. When you do cogstop(1) you're actually killing FSRW.

user82 · 2010-12-26 09:25

thanks for your awnser, so:

ok point one corrected, numbers now from 0-6

the second problem occurred to me but ill fix it later, double calculations dont matter for a first test

the outer loop is now also done, just forgot that

and i thought fsrw is on cog0, okay..file close now before killing fsrw cog and one less worker

so
cog0=Pub Main
cog1=fsrw
cog2-7=Pub worker

is there anything left to do to get it running as first test, because it does not seem to work after opening the primes.txt?

var
long primglobal
long primcog[8]
long lastwritten[8]
long stack1[128]
long stack2[128]
long stack3[128]
long stack4[128]
long stack5[128]
long stack6[128]


obj
 sd: "fsrw"
 
PUB main
primglobal:=0
sd.mount(0)
sd.popen(string("primes.txt"), "w")

cognew(worker, @stack1)
cognew(worker, @stack2)
cognew(worker, @stack3)
cognew(worker, @stack4)
cognew(worker, @stack5)
cognew(worker, @stack6)


repeat while(primglobal<=60)
  if(primcog[2]<>lastwritten[2])
    sd.sddec(primcog[2])
    sd.pputc(10)
    lastwritten[2]:=primcog[2]
  if(primcog[3]<>lastwritten[3])
    sd.sddec(primcog[3])
    sd.pputc(10)
    lastwritten[3]:=primcog[3]
  if(primcog[4]<>lastwritten[4])
    sd.sddec(primcog[4])
    sd.pputc(10)
    lastwritten[4]:=primcog[4]
  if(primcog[5]<>lastwritten[5])
    sd.sddec(primcog[5])
    sd.pputc(10)
    lastwritten[5]:=primcog[5]
  if(primcog[6]<>lastwritten[6])
    sd.sddec(primcog[6])
    sd.pputc(10)
    lastwritten[6]:=primcog[6]
  if(primcog[7]<>lastwritten[7])
    sd.sddec(primcog[7])
    sd.pputc(10)
    lastwritten[7]:=primcog[7]


sd.pclose

cogstop(1)
cogstop(2)
cogstop(3)
cogstop(4)
cogstop(5)
cogstop(6)
cogstop(7)



dira[17]:=1
dira[16]:=1
  repeat
    outa[16] := 1
    waitcnt(clkfreq/2 + cnt)
    outa[16] := 0
    waitcnt(clkfreq/2 + cnt)

PUB worker | primlocal,x,ptrue,helper1
 repeat
  primlocal:=primglobal
  primglobal:=primglobal+1
  helper1:=primlocal/2+1
  ptrue:=1

  repeat x from 2 to helper1
  if(primlocal//x==0)
    ptrue:=0
    quit

  repeat while(primcog[cogid]<>lastwritten[cogid])
  waitcnt(clkfreq/128 + cnt)

  if(ptrue==1)
  primcog[cogid]:=primlocal

MagIO2 · 2010-12-26 10:56

Indentation in the worker is wrong.

The "repeat x from 2 to helper1" will simply do nothing, if your indentation in your code is really as given in the post.
Same is true for your other repeat and for the if( ptrue==1 )
Remember, the indentation in SPIN is like the block structure { } in C. If you don't start a block in C then only one instruction will be executed, for example in a while-loop or in an if statement. In SPIN no indentation means do nothing.

You still stop COG 1 which is the SD driver. You should not do that without making sure that it successfully closed the file. If your output has less than 512 bytes you have a good chance to see an empty file, as putc will only write into an internal 512 bytes buffer ( one sector ) until the buffer is full.

user82 · 2010-12-26 11:18

intendation got lost somehow, is fixed now

still it does not really work....

primglobal is increaded to 14 in a test (led blinks) but not to 60....

plus the file is opened but not closed

MagIO2 · 2010-12-26 12:22

Archieve your latest code and attach it, so we can have a closer look.

user82 · 2010-12-26 12:35

var
long primglobal
long primcog[8]
long lastwritten[8]
long stack1[128]
long stack2[128]
long stack3[128]
long stack4[128]
long stack5[128]
long stack6[128]


obj
 sd: "fsrw"
 
PUB main
primglobal:=0
sd.mount(0)
sd.popen(string("primes.txt"), "w")

cognew(worker, @stack1)
cognew(worker, @stack2)
cognew(worker, @stack3)
cognew(worker, @stack4)
cognew(worker, @stack5)
cognew(worker, @stack6)


repeat while(primglobal<=60)
  if(primcog[2]<>lastwritten[2])
    sd.sddec(primcog[2])
    sd.pputc(10)
    lastwritten[2]:=primcog[2]
  if(primcog[3]<>lastwritten[3])
    sd.sddec(primcog[3])
    sd.pputc(10)
    lastwritten[3]:=primcog[3]
  if(primcog[4]<>lastwritten[4])
    sd.sddec(primcog[4])
    sd.pputc(10)
    lastwritten[4]:=primcog[4]
  if(primcog[5]<>lastwritten[5])
    sd.sddec(primcog[5])
    sd.pputc(10)
    lastwritten[5]:=primcog[5]
  if(primcog[6]<>lastwritten[6])
    sd.sddec(primcog[6])
    sd.pputc(10)
    lastwritten[6]:=primcog[6]
  if(primcog[7]<>lastwritten[7])
    sd.sddec(primcog[7])
    sd.pputc(10)
    lastwritten[7]:=primcog[7]


sd.pclose
waitcnt(clkfreq + cnt)
cogstop(1)
cogstop(2)
cogstop(3)
cogstop(4)
cogstop(5)
cogstop(6)
cogstop(7)



dira[17]:=1
dira[16]:=1
  repeat
    outa[16] := 1
    waitcnt(clkfreq/2 + cnt)
    outa[16] := 0
    waitcnt(clkfreq/2 + cnt)

PUB worker | primlocal,x,ptrue,helper1
 repeat
  primlocal:=primglobal
  primglobal:=primglobal+1
  helper1:=primlocal/2+1
  ptrue:=1

  repeat x from 2 to helper1
    if(primlocal//x==0)
      ptrue:=0
      quit

  repeat while(primcog[cogid]<>lastwritten[cogid])
    waitcnt(clkfreq/128 + cnt)

  if(ptrue==1)
    primcog[cogid]:=primlocal

is there anything like in c "if(sd.pclose)" to be sure it works

MagIO2 · 2010-12-26 13:58

check the manual what <= means in SPIN ;o)

MagIO2 · 2010-12-26 14:39

I changed it a bit and now it runs:

  repeat while(primglobal<10000)
    repeat y from 0 to 7
      if(primcog[y]<>0)
        term.str( string( "COG " ) )
        term.dec( y )
        term.str( string( " says: ") )
        term.dec(primcog[y])
        term.str( string( " / ") )
        term.dec( primglobal )
        term.tx($0d)
        primcog[y]:=0

PUB worker | primlocal,x,ptrue,helper1
 
 repeat
   repeat until not lockset( 0 )
   primlocal:=primglobal
   primglobal++
   lockclr( 0 )
   
   helper1:=primlocal/2+1
   ptrue:=1

   repeat x from 2 to helper1
     if((primlocal//x)==0)
       ptrue:=0

   if(ptrue==1)
     primcog[cogid]:=primlocal
     repeat while(primcog[cogid]<>0)

As you can see I got rid of the lastwritten. Instead primcog is set to the prime number by the worker and reset to zero by the code writing to serial terminal in my case.

Without the locking the count of numbers was wrong, so some numbers have been processed by different COGs due to the problem described in my first post.

Cluso99 · 2010-12-26 15:00

Firstly, welcome to the fantastic world of the prop. You have started with a fairly complex piece of code, so you must have some programming experience. I will let the others comment on your code.

Just an aside note.... Many of us who have done some complex things with the prop have never used locks as they are usually not required because of the way the prop works (determinism between hub access and cogs).

MagIO2 · 2010-12-26 15:29

If you have read, modify, write access to a global resource without any other rules, using locks is feasible and easy.

I agree that there are not very much problems that could not be solved without using locks - and I did not use locks before as well.

I don't know why user82 has choosen to write the code as it is, but as prooved by running the non-lock version, the code has problems with parallel processing, because runtime of each iteration changes as there are more divisions to check. So, without changing the written code to much this is the easiest way to fix the problem.

If you change the worker by giving it some parameters, you can avoid the lock. For example you can give the worker the number to start with, the max number and the number of workers.
COGNEW( worker(2,60,6), @stack2 )
COGNEW( worker(3,60,6), @stack3 )
....
Then the each worker has it's own amount of numbers to check:
worker1 would check 2,8,14,20 ....
worker2 would check 3,9,15,21 ....

Writing this shows that it does not make sense to start 6 COGs in this model, as each worker that starts with an even number will never find a prime-number again ;o) except worker1 which would find exactly 1 prime-number. So, starting 5 COGs is better.

But this shows a way how to avoid locks.

First code - some bug included?

Comments