Thanks for taking the time to look through the code and for the reply, I have been in this situation before and "coding around" it never fixes the problem permanently.
The main cog was originally going to be the debugger cog, since I have jm_freqin using 2 cogs and the simulation cogs sending to both jm_freqin objects/cogs using another 2 cogs. Do you think I should have a separate cog outside of the main cog for debugging? Normally I'm only sending a few lines of pst, as I understand what you are saying and how it slows things down, although I know in the unclean version of the code I posted there was commented out pst code everywhere.
As for number 2, I appreciate you pointing that out. I have not seen an issue from that yet, but I'm sure it would have come up. I will make that change tonight. I will also being putting the 2 calculation parts that are in the main loop, into methods and calling them from the main loop.
Let me know if I should allocate a separate cog entirely from the main cog for debug data and if you have any other "best practice" suggestions.
The things going on here are pretty complex. I've spotted the following:
1. You seem to work with timings in several areas of your program. But on the other hand you are very generous with usage of PST. But PST has a buffer of 64 bytes. So, when you extend the number of characters send this will slow down your whole program and the conversions will be done in the COG calling the PST functions anyway. So, I'd suggest to think about a debugger/logger COG which runs independently.
2. Chances are high, that these kind of if-statements don't do what you expect
if(cnt > (timeStamp + (clkfreq/l_updateRateDenominator)))
The problem is that the > operation is working with signed numbers. So, from $0000_0000 - $7fff_ffff it will see a positive number and from $ffff_ffff-$8000_0000 it will see negative numbers. In other words for the > operator $0000_0000 will be bigger than $8000_0000.
If you work with a difference you don't have this problem:
if((cnt-timeStamp) > (clkfreq/l_updateRateDenominator))
What I would do .... well ... I'd put together things that belong together.
For example create an object for that crank thing (sorry, but the following code is untested again - no PropTool available currently):
VAR
byte pin, presentTeeth, missingTeeth, cogID
long frequencyRate
long pulse, sync, timing, localIndex
long stack[10]
PUB start(p, initFrequency, pt, mt)
pin := p
presentTeeth := pt
missingTeeth := mt
if cogID
cogstop( cogID-1 )
cogID := cognew( crankPulser, @stack )+1
PUB setFrequency( newFrequ )
if( newFrequ <= 0 )
frequencyRate := 7000
else
frequencyRate := newFrequ <# 7000
timing := clkfreq / frequencyRate
PUB getFrequency
return frequencyRate
PRI crankPulser
'' Output 1 to 500Hz signal on pin
'' -- high-going pulse is fixed to 1ms
'' -- pntr is hub address of signal frequency (1 to 500Hz)
ctra := (%00100 << 26) | pin ' setup counter for pin
frqa := 1
phsa := 0
dira[pin] := 1 ' make output
pulse := clkfreq / 10_000 ' ticks in 1ms
sync := cnt ' sync with system clock
repeat
repeat localIndex from 1 to (presentTeeth- missingTeeth)
phsa := -pulse ' create 1ms pulse
waitcnt(sync += timing) ' hold for current hz setting
repeat localIndex from 1 to (missingTeeth)
'phsa := -pulse ' create 1ms pulse
waitcnt(sync += timing)
The point is that the number of locals has been shrinked dramatically. The compiler now has a chance to complain if the data size + code size extends the available RAM. Using local variables will not complain even if your stack is to small and can produce any kind of strange and very hard to find bugs!
You have this in a separate file now and can easily setup a small program which only tests this part. The test program also gives you a good impression on how to use the object without any code that's dealing with completely different stuff - like in your real project main program. This is very usefull if coming back to the program a few years later ;o)
Moving the variables to the VAR section of the object has one drawback: these variables are not accessible by the program using the object. So you have to add getter and setter functions as needed. But on the other hand the setter function can already do the validation and makes sure that only valid values are written into the variables.
Now it's also very easy to create 2 instances running with different frequencies on different PINs:
and let it run:
pulser1.start( 4, 6000, 36, 2 )
pulser2.start( 16, 500, 36, 2 )
and change the frequency
pulser1.setFrequency( 3000 )
Run a new COG for debugging ... my general answer would be YES.
It's only needed if you deal with timings because sending debug information to PST might destroy your timings if you do it in the program itself. It can happen that it does not work because of debugging - it can also happen that it works with debugging, but without it no longer works!
Adding debug statements to several COGs is also problematic!
Having simple blinking LEDs that show the status is sometimes also very helpfull and is much faster than sending strings and converted numbers via serial interface.
I think I found my mysterious issue. I'm trying to implement the changes you recommended and have a question.
When I do not have some local variables in main, one of my functions does not work. Can you tell me what this means? What does it mean when you put long[] in front of a pointer? pntr is a pointer to an @long variable in my code.
timing := long[pntr]
There is some sort of timing issue with my code I believe. If it just tells the code to read the long value, why would it be where you pass the address to the method parameter, and then read the long value of the address that the method parameter stores? Why not just pass the long to begin with?
What I would do .... well ... I'd put together things that belong together.
For example create an object for that crank thing (sorry, but the following code is untested again - no PropTool available currently):
VAR
byte pin, presentTeeth, missingTeeth, cogID
long frequencyRate
long pulse, sync, timing, localIndex
long stack[10]
PUB start(p, initFrequency, pt, mt)
pin := p
presentTeeth := pt
missingTeeth := mt
if cogID
cogstop( cogID-1 )
cogID := cognew( crankPulser, @stack )+1
PUB setFrequency( newFrequ )
if( newFrequ <= 0 )
frequencyRate := 7000
else
frequencyRate := newFrequ <# 7000
timing := clkfreq / frequencyRate
PUB getFrequency
return frequencyRate
PRI crankPulser
'' Output 1 to 500Hz signal on pin
'' -- high-going pulse is fixed to 1ms
'' -- pntr is hub address of signal frequency (1 to 500Hz)
ctra := (%00100 << 26) | pin ' setup counter for pin
frqa := 1
phsa := 0
dira[pin] := 1 ' make output
pulse := clkfreq / 10_000 ' ticks in 1ms
sync := cnt ' sync with system clock
repeat
repeat localIndex from 1 to (presentTeeth- missingTeeth)
phsa := -pulse ' create 1ms pulse
waitcnt(sync += timing) ' hold for current hz setting
repeat localIndex from 1 to (missingTeeth)
'phsa := -pulse ' create 1ms pulse
waitcnt(sync += timing)
The point is that the number of locals has been shrinked dramatically. The compiler now has a chance to complain if the data size + code size extends the available RAM. Using local variables will not complain even if your stack is to small and can produce any kind of strange and very hard to find bugs!
You have this in a separate file now and can easily setup a small program which only tests this part. The test program also gives you a good impression on how to use the object without any code that's dealing with completely different stuff - like in your real project main program. This is very usefull if coming back to the program a few years later ;o)
Moving the variables to the VAR section of the object has one drawback: these variables are not accessible by the program using the object. So you have to add getter and setter functions as needed. But on the other hand the setter function can already do the validation and makes sure that only valid values are written into the variables.
Now it's also very easy to create 2 instances running with different frequencies on different PINs:
and let it run:
pulser1.start( 4, 6000, 36, 2 )
pulser2.start( 16, 500, 36, 2 )
and change the frequency
pulser1.setFrequency( 3000 )
Run a new COG for debugging ... my general answer would be YES.
It's only needed if you deal with timings because sending debug information to PST might destroy your timings if you do it in the program itself. It can happen that it does not work because of debugging - it can also happen that it works with debugging, but without it no longer works!
Adding debug statements to several COGs is also problematic!
Having simple blinking LEDs that show the status is sometimes also very helpfull and is much faster than sending strings and converted numbers via serial interface.
timer := long[ someadr ]
is the same as
timer := value
The point is that in function calls the parameters are passed by value. Something like
VAR
long frequency
PUB main
frequency := 50
cognew( doSomething( frequency ), @stack )
...
PRI doSomething( f )
...
means: start a new COG and execute function doSomething by passing 50. You will find 50 in the local variable f. There is no link between frequency and f. When you change frequency in the main, f still has the value 50. By passing the address you actually have a link to frequency in doSomething and you can always read the actual value of frequency.
About your problem of not having a local variable: that's why there is a getter-function in the object. If you need the value of frequencyRate you'd simply call the getFrequency function.
By the way I found a bug in my example: in the start function initFrequency is not stored. Simply calling setFrequency solves the problem.
Thank you, I thought that was what that did but wanted to verify and not assume. Thank you for all you've shown me
I'm slowly trying to migrate to your code format.
I have a cog operating the com data now, but I'm not sure if I'll be able to use crankPulser because I was seeing some inconsistencies that I didn't understand.
Here is my current code, it works ok, but I cannot get one of the calculated values if I increase the line below to 10000, at 5000 it does fine.
I'm just not sure why the code below fails to read the low pulse timing gaps that will appear after each set of 34 successive "high/low" pulses, I don't think it's a speed issue but certainly appears that way??? Any thoughts as to what my mistake is here?
Well my problem definitely appears to be a timing issue, I think the variable is being written to when I'm trying to read it, so in the old cog setup, I had the calculations, the writing to and the read all being done in the same thread making it impossible for the variable to be written to while it was being read, because it was all in one process sequentially.
Now that I have separate threads, above a certain speed I cannot reliably read and write to the same long. How to fix this, I have no idea but this at least gives me some closure on the inconsistencies.
I think your timings don't fit. On one hand you allow a value of 5000 as frequency setting but your pulse should be 1ms. 1ms gives you a max frequency of 1000Hz.
Sometimes it's better to start from scratch again. Maybe you want to continue with my version. It uses the crank object I already posted before. Please have a closer look at the settings, as I used a LED-pin of my quickstart-board as output, so I can see what happens.
I also reduced the baudrate of PST ... don't know why, but it's my preferred setting ;o)
Such kind of parameters should rather be defined in the CON section at the very beginning, so they can easily be changed without changing something inside of the code.
Most important change was the setup of the counter. Your setup used PLLDIV of Vco / 128 which means that the counter runs much slower than the clock, but calculations are based on clkfreq.
What I do very often in terminal based projects is to wait until a key is pressed before really starting the program.
I will try with your code, I certainly appreciate the time you spent writing it.
Maybe I'll take a night off, I've been trying to get this working for 2 weeks now and I may need some clarity by taking a step back. I see what you mean about the counter now, that was due to my ignorance, I was modifying what someone else wrote and need higher than a 500hZ resolution. I'll try this and report back my results.
Most important change was the setup of the counter. Your setup used PLLDIV of Vco / 128 which means that the counter runs much slower than the clock, but calculations are based on clkfreq.
Why would an NCO (%00100) be affected by the PLLDIV field?
... what would make NCO at better or worse choice for this, than say POS (%01000)?
AFAIK you want to generate a pulse train which means you're limited to PLL/NCO/DUTY modes. All other modes don't produce outputs (not counting feedback which is just the inverted input).
I did not use counters too often so far!
In retrospective:
I used turbosupras code with the counter connected to a quickstart LED. What I saw was that the LED in fact was blinking, but veeery slow. That's why I thought the counter would maybe run with the wrong frequency. And I changed it without a re-read of the manual.
At the same time I spotted the problem with the frequency setting and changed that as well. Looks like the real fix was the frequency setting.
@turbosupra:
Did you run the code? Is that what you want crank to do?
If you want to go higher than 500Hz, you can't live with an pulse width of 1ms. Simply change this as well and you can go higher.
Thank you so much. It does simulate a crank trigger wheel and crank trigger sensor quite well according to the propscope.
My method that uses the jm_freqin object is even able to calculate rpm from it . I tried to write code that would count how many times a second, the cog was loop through the part of the code directly below, but I cannot get it to work?
repeat localIndex from 1 to (missingTeeth)
phsa := 0 ' create 1ms pulse
waitcnt(sync += timing)
I also noticed that my code would consider a "second" (as defined below) every 81600000 cycles instead of 80000000 which was also strange. I initially define timeStampCrank once the first time the method is called and then I try and enter the part of the code underneath of this, when cnt-l_timeStampCrank is greater than 80000000 clk cycles. I cannot for the life of me figure out why the timing would be so far off at 81.6 million cycles? Am I approaching this incorrectly?
if (l_timeStampCrank == 0)
l_timeStampCrank := (cnt) if((cnt-l_timeStampCrank) > clkfreq)
I did not use counters too often so far!
In retrospective:
I used turbosupras code with the counter connected to a quickstart LED. What I saw was that the LED in fact was blinking, but veeery slow. That's why I thought the counter would maybe run with the wrong frequency. And I changed it without a re-read of the manual.
At the same time I spotted the problem with the frequency setting and changed that as well. Looks like the real fix was the frequency setting.
@turbosupra:
Did you run the code? Is that what you want crank to do?
If you want to go higher than 500Hz, you can't live with an pulse width of 1ms. Simply change this as well and you can go higher.
I don't know why you want to measure the time for this loop! The time this loop needs is pre-defined by the timing variable.
sync := cnt
....
waitcnt( sync += timing )
goes on with execution exactly after timing clockticks, except you missed the cnt-value which adds ~53seconds to the runtime. So, in your loop you maybe have to add some cycles for the condition-check of the repeat .. from, but this should be some hundred clocks.
Well ... with the cnt - l_timestampCrank - method you don't wait for an exact time. You want to continue in your program and start something if the time has exceeded. How much it can exceed of course depends on the other code running in that loop. If you have some serial output this might easily extend 1/10th of a second. Makes sense to post the whole code here to get an idea which code delays the loop.
Again ... you are the one who defines how long to wait in the waitcnt! I currently can't see a good reason to MEASURE what you already know! Don't you trust your code?
clkfreq / (presentTeeth * timing ) gives you the answer. (Assuming you can live with integers)
Ok, no PST calls in between, but you still do a lot in between two calls of calculateCrankRpm. So, let's say you call calculateCrankRpm and the code reaches the if statement when cnt - l_timeStampCrank is equal to 79_999_999. Your code has to go another round:
1. return from calculateCrankRpm
2. call fcrank.freq - doing calculations
3. call fcrank.period
4. call calculateCrankRpm again which first runs through a lot of calculations
Pretty sure this can sum up to 1600000 additional clock cycles
It looks like you could simply add a function getRPM into Crank.spin, which simply returns
clkfreq / (presentTeeth * timing ) * 60
I don't know enough about your problem domain, so I can't easily estimate what the purpose of the code is. Maybe you can explain the next step and we can develop your application step by step.
I wanted to measure Rpm the second way because I wanted some redundancy, but this may be me being over cautious and may not be needed.
calculateCrankRpm allows me to take a rolling average of how many clk cycles it took to go from positive edge to positive edge, for the last 5 teeth. It also assumes that if the clk cycle count is bigger than the previous average multiplied by 1.1, that it is the missing teeth gap (which may also be a long solid tooth instead of a long gap, I have to verify this by disassembling something but it serves the exact same purpose either way), and it does not include that value in its rolling average. That thread was a good read.
If I added clkfreq / (presentTeeth * timing ) * 60, I could keep track of presentTeeth the way I keep track of l_Cnt, but I'm fuzzy on keeping track of the time I've done since last calculation (timing). The reason being, is that I should get l_Cnt to go to ~167 at 10000rpm, because this means the crank is rotating 167 times a second, and you get +1 to l_Cnt for each single full rotation. I'm not seeing anywhere near that, even when I go 81.6 million cycles, or about 1.02 seconds.
To help illustrate, here is an idea of what I my hall effect sensor trigger wheels look like, the "sensing" part is done by a hall effect sensor. My goal is to keep track of how fast I'm going from the positive edge of one tooth to the positive edge of another tooth and calculate rpm from it. I also want to be able to reference which tooth I am at in the rotation. Does that make sense? This sensor is the main component of a car/truck in telling the cars computer when to fire a fuel injector to a certain cylinder and when to fire a spark plug for a certain cylinder.
I made a few small code changes just now by adding some flags and the first time fully through the code you get 167 (l_CntPeak) for the 80,443,840 clk cycles, which is right within my numerical expectation. 80,443,840/80,000,000 = 1.005548 and 167/1.005548 = 166.0786 . After that it will read between 163 and 166, but it should be consistent I would think? Is my logic on or off with this?
Is there a standard way to time how long a method takes, or is my l_timeStampSubtracted sufficient?
Hi Turbosupra ... I have some little changes - not functional changes because I'm currently a bit to confused!
First of all thanks for the nice picture. But that's exactly what confuses me. With the code you write .. do you want to simulate these trigger wheels for external usage or do you want to read/measure the sensor signals produced by these wheels and the current crank-code is only for checking the measure-code?
I don't really remember where the code (cnt - lastMeasuredCnt) > timespan came from - was it in your original post. I remember that I corrected your first implementation ... however .... this kind of measurement is good for tasks that you want to perform somewhen around that given timespan. Or more precicely in the range of exactly this timespan or more. This kind of code is not good if you need exact results.
Allowing a deviation in the if statement (in time) means in the end that the numbers might also vary from one to the next try.
If you really need this measurement-stuff, I'd suggest to add the moving average as outlined by Beau in the thread I mentioned into the PASM-part of Johns frequency measurement object. There you should have plenty of time for summing up some values.
Only cosmetical changes showing you how I'd do the output.
Doing these changes I also got some new questions:
What does l_toothGapRpm mean?
What does l_toothRpm mean?
What is ((clkfreq/fcrank.period)*60)/36 ? Am I right with labeling it as RPM?
And finally why should the RPMs be different at all?
If the wheel turned one round each of it's teeth also turned one round and the gap turned one round?
I know that I am crossing two very different worlds with the mechanics and technical terms of automotive, and the electronics and technical terms of circuitry. I try to keep the automotive things out of the discussion to keep things less confusing, but if you have any questions about that I can explain that in as much detail if you would like.
As for the different methods. I have found that it is much easier troubleshooting and writing code from inside of my house, then outside in the car ... possibly even on the couch or in bed, so I decided to try and use the prop to simulate the sensors as best as possible, before I sat out in the car for hours in my driveway with the laptop, extension cord and wasting a lot of gasoline . Last summer, I did the opposite and wasted many gallons of gas, but I have now seen the beauty of the prop feeding itself signals and learned my lesson for this summer.
l_toothGapRpm is supposed to be the rpm calculated from the bridge or gap (the part I've circled in the picture below) . I believe the automotive manufacturers do this so that they can tell when an engine rotates past the tooth that they deem 0 or top dead center, but this is only speculation on my part. I do know however that it is there to differentiate when the 360 degree rotation starts.
l_toothRpm is supposed to be the rpm calculated from a single positive edge of one of those triggers, to the next positive edge of the following trigger. I have error checking in the code to throw out large variances such as the bridge/gap circled because the value would be way way off.
((clkfreq/fcrank.period)*60)/36 is rpm. It takes the amount of clk cycles between a rising edge and the previous rising edge before it and it then divides the clkfreq by that number as the denominator to calculate how many times a second that "period" would occur based on that single snapshot in time. Then it calculates the number of seconds in a minute, since Rpm is per minute and finally it divides by the number of times that "period" occurs before a single revolution of the trigger wheel has occurred. That sounded confusing when I wrote it, so I can explain better if it reads that way also.
I can't use straight frequency though, because of the delimiter that the automotive company has built into the hall effect trigger wheel, so I have to have logic that addresses that, which is why I have to take and average and insure that the new value is not outside of that average by a certain amount.
Ah, I did not know you could do that with the print screen, that is AWESOMEEEE! That's the coolest thing I have learned all day, does it by chance somehow save any overhead with bits? I like the constants too, that makes sense.
Only cosmetical changes showing you how I'd do the output.
Doing these changes I also got some new questions:
What does l_toothGapRpm mean?
What does l_toothRpm mean?
What is ((clkfreq/fcrank.period)*60)/36 ? Am I right with labeling it as RPM?
And finally why should the RPMs be different at all?
If the wheel turned one round each of it's teeth also turned one round and the gap turned one round?
"does it by chance somehow save any overhead with bits?"
I'm not sure wheather I understand this question correctly! You mean: Does it save bandwidth? ?
Currently it needs more bandwidth, but as you only update the screen once per second that's currently not an issue. The point is that this way the positioning information has to be send first and that old values need to be overwritten completely. For example if you output 83444 first and then the number decreases to 11 you have to overwrite 444 with spaces or you'd see 11444.
But some extra code could be added, which only updates the values that really changed. Depending on the stability of the values you might save bandwidth.
Ok ... so measuring is the essence of your program. So I'll concentrate on that tomorrow. Which max. RPM do we talk about?
Simulating the wheel and it's signals is of course a good idea for producing some code without being attached to the real hardware. The only problem could be to write some code for a clean digital signal without dropouts and spikes when the real signal later on is not that clean. How good is the quality of the real signal later on?
Is this device you build for display-only or is it meant to control other hardware related to the input?
With the statement you gave earlier that you want to know which tooth is actually passing the sensor I'd expect you want to control something based on this information.
I think that Johns frequency measurement is a good starting-point. But I also think that you have to extend exactly this PASM driver to do all the number juggling.
At 10000RPM we have a tooth frequency of 10000/60*36 = 6000 teeth/second, right?. I think that it will be hard to do all the calculations you do in calculateCrankRpm can't in SPIN in 1/6000s. Maybe you could start several COGs sharing this task, but Johns PASM-driver has a lot of spare time (in PASM you have >3000 instructions which can be executed in this time) and it's the place where the input-numbers come from, so why not process them there?
It may be time that I learn PASM, but I'll be honest with you, I know literally 0 about it and have avoided it up until this point. Spin was challenging for me, because it was strange compared to the OO language I am most comfortable with, c sharp. Your math is correct, and I believe you are saying that >3000 instructions can be executed in 13333 clk cycles? Wow.
I will say that I am ready if you are willing to teach/guide, I'll do my best and hope to not frustrate you while trying to learn.
I scoped the sensor a while back, and found the pictures. The first is what the OEM manufacturer says about the signal, the next picture is of the sensor at 1000rpm (600 teeth a second), the third picture is of the sensor at 5000rpm (3000 teeth a second). I was planning on tapping into this signal (the factory computer uses this signal) with a resistor bridge and using it for controlling other things, you are correct about that. I built an application in c sharp that allows me to receive the data and display it in real time, as well as log it to an excel spreadsheet line by line.
Ok, so it appears that the following portion of the jm_freqin object has the equivalent of the variables/constants section at the bottom, with the constants in all caps and the variable names in lowercase?
' --------------------------------------------------------------------------------------------------
POS_DETECT long %01000 << 26
NEG_DETECT long %01100 << 26
tmp1 res 1
tmp2 res 1
mask res 1 ' mask for frequency input pin
cyclepntr res 1 ' hub address of cycle count
cycles res 1 ' cycles in input period
fit 492
Then method frcntr is started with its own cog, I don't understand why the pin is passed as a stack address long though. Or is cognew different in PASM because it automatically has a 492 long stack, so it doesn't need the stack defined?
Then the pin number is copied to tmp1, and after that the program will read the value of tmp1 and store it in tmp2.
After that, the time between positive edges is assigned to ctra register, then the value of tmp2 is added to ctra, not sure how this works but it has to be assigning the pin number to ctra. Then mov adds a literal 1 to frqa, I believe when ctra detects a rising edge.
The same is done for ctrb with falling edges in the next section.
Then the literal value of 1 is copied to mask, followed by mask being shifted left by the number of bits in tmp2 (the pin number), then a bitwise and operation is performed to dira on the ! of mask, possibly alternating back and forth between input and output?
After that a literal 4 is added to tmp1, then tmp1 is copied to cyclepntr
Then it appears to go into a loop.
wait for pin to not equal high (is this comparing the pin to itself and waits for it to equal something set in the register?)
move a literal 0 to phsa to clear it
wait for to equal high (looks like it compares it to itself again and waits for it to equal something set in the register?)
move a literal 0 to phsb
wait for pin to not equal high (looks like it compares it to itself again and waits for it to equal something set in the register?)
move value of phsa to cycles
wait for pin to equal high (looks like it compares it to itself again and waits for it to equal something set in the register?)
add phsb to cycles containing phsa cycles already
write value of cycles to cyclepntr
goto restart
I don't see how this interacts and transfers the data to the spin portion. Does an assembly cognew automatically return a value when you assign it to a variable, like what is done here in the first spin method with okay and cog? Even if that were true, I still can't figure out how the assembly data is passed into the spin code? My brain hurts now, but I've done my best to try and understand the code.
dat
org 0
frcntr mov tmp1, par ' start of structure
rdlong tmp2, tmp1 ' get pin#
mov ctra, POS_DETECT ' ctra measures high phase
add ctra, tmp2
mov frqa, #1
mov ctrb, NEG_DETECT ' ctrb measures low phase
add ctrb, tmp2
mov frqb, #1
mov mask, #1 ' create pin mask
shl mask, tmp2
andn dira, mask ' input in this cog
add tmp1, #4
mov cyclepntr, tmp1 ' save address of hub storage
restart waitpne mask, mask ' wait for 0 phase
mov phsa, #0 ' clear high phase counter
highphase waitpeq mask, mask ' wait for pin == 1
mov phsb, #0 ' clear low phase counter
lowphase waitpne mask, mask ' wait for pin == 0
mov cycles, phsa ' capture high phase cycles
endcycle waitpeq mask, mask ' let low phase finish
add cycles, phsb ' add low phase cycles
wrlong cycles, cyclepntr ' update hub
jmp #restart
Comments
Thanks for taking the time to look through the code and for the reply, I have been in this situation before and "coding around" it never fixes the problem permanently.
The main cog was originally going to be the debugger cog, since I have jm_freqin using 2 cogs and the simulation cogs sending to both jm_freqin objects/cogs using another 2 cogs. Do you think I should have a separate cog outside of the main cog for debugging? Normally I'm only sending a few lines of pst, as I understand what you are saying and how it slows things down, although I know in the unclean version of the code I posted there was commented out pst code everywhere.
As for number 2, I appreciate you pointing that out. I have not seen an issue from that yet, but I'm sure it would have come up. I will make that change tonight. I will also being putting the 2 calculation parts that are in the main loop, into methods and calling them from the main loop.
Let me know if I should allocate a separate cog entirely from the main cog for debug data and if you have any other "best practice" suggestions.
Thanks again!
For example create an object for that crank thing (sorry, but the following code is untested again - no PropTool available currently):
VAR byte pin, presentTeeth, missingTeeth, cogID long frequencyRate long pulse, sync, timing, localIndex long stack[10] PUB start(p, initFrequency, pt, mt) pin := p presentTeeth := pt missingTeeth := mt if cogID cogstop( cogID-1 ) cogID := cognew( crankPulser, @stack )+1 PUB setFrequency( newFrequ ) if( newFrequ <= 0 ) frequencyRate := 7000 else frequencyRate := newFrequ <# 7000 timing := clkfreq / frequencyRate PUB getFrequency return frequencyRate PRI crankPulser '' Output 1 to 500Hz signal on pin '' -- high-going pulse is fixed to 1ms '' -- pntr is hub address of signal frequency (1 to 500Hz) ctra := (%00100 << 26) | pin ' setup counter for pin frqa := 1 phsa := 0 dira[pin] := 1 ' make output pulse := clkfreq / 10_000 ' ticks in 1ms sync := cnt ' sync with system clock repeat repeat localIndex from 1 to (presentTeeth- missingTeeth) phsa := -pulse ' create 1ms pulse waitcnt(sync += timing) ' hold for current hz setting repeat localIndex from 1 to (missingTeeth) 'phsa := -pulse ' create 1ms pulse waitcnt(sync += timing)The point is that the number of locals has been shrinked dramatically. The compiler now has a chance to complain if the data size + code size extends the available RAM. Using local variables will not complain even if your stack is to small and can produce any kind of strange and very hard to find bugs!
You have this in a separate file now and can easily setup a small program which only tests this part. The test program also gives you a good impression on how to use the object without any code that's dealing with completely different stuff - like in your real project main program. This is very usefull if coming back to the program a few years later ;o)
Moving the variables to the VAR section of the object has one drawback: these variables are not accessible by the program using the object. So you have to add getter and setter functions as needed. But on the other hand the setter function can already do the validation and makes sure that only valid values are written into the variables.
Now it's also very easy to create 2 instances running with different frequencies on different PINs:
OBJ
pulser1: "NewPulserCode.spin"
pulser2: "NewPulserCode.spin"
or
pulserArray[2]: "NewPulserCode.spin"
and let it run:
pulser1.start( 4, 6000, 36, 2 )
pulser2.start( 16, 500, 36, 2 )
and change the frequency
pulser1.setFrequency( 3000 )
Run a new COG for debugging ... my general answer would be YES.
It's only needed if you deal with timings because sending debug information to PST might destroy your timings if you do it in the program itself. It can happen that it does not work because of debugging - it can also happen that it works with debugging, but without it no longer works!
Adding debug statements to several COGs is also problematic!
Having simple blinking LEDs that show the status is sometimes also very helpfull and is much faster than sending strings and converted numbers via serial interface.
I've learned through years of programming that you never assume any variable is initialized, until it is!
I agree and I actually know better than to assume anything.
Bruce
When I do not have some local variables in main, one of my functions does not work. Can you tell me what this means? What does it mean when you put long[] in front of a pointer? pntr is a pointer to an @long variable in my code.
There is some sort of timing issue with my code I believe. If it just tells the code to read the long value, why would it be where you pass the address to the method parameter, and then read the long value of the address that the method parameter stores? Why not just pass the long to begin with?
timer := long[ someadr ]
is the same as
timer := value
The point is that in function calls the parameters are passed by value. Something like means: start a new COG and execute function doSomething by passing 50. You will find 50 in the local variable f. There is no link between frequency and f. When you change frequency in the main, f still has the value 50. By passing the address you actually have a link to frequency in doSomething and you can always read the actual value of frequency.
About your problem of not having a local variable: that's why there is a getter-function in the object. If you need the value of frequencyRate you'd simply call the getFrequency function.
By the way I found a bug in my example: in the start function initFrequency is not stored. Simply calling setFrequency solves the problem.
I'm slowly trying to migrate to your code format.
I have a cog operating the com data now, but I'm not sure if I'll be able to use crankPulser because I was seeing some inconsistencies that I didn't understand.
Here is my current code, it works ok, but I cannot get one of the calculated values if I increase the line below to 10000, at 5000 it does fine.
I'm just not sure why the code below fails to read the low pulse timing gaps that will appear after each set of 34 successive "high/low" pulses, I don't think it's a speed issue but certainly appears that way??? Any thoughts as to what my mistake is here?
localCrankPeriod := fcrank.period if(localCrankPeriod) <> 0 and l_pstReadCrankWheel == 1 calculateCrankRpm(localCrankPeriod)Now that I have separate threads, above a certain speed I cannot reliably read and write to the same long. How to fix this, I have no idea
Sometimes it's better to start from scratch again. Maybe you want to continue with my version. It uses the crank object I already posted before. Please have a closer look at the settings, as I used a LED-pin of my quickstart-board as output, so I can see what happens.
I also reduced the baudrate of PST ... don't know why, but it's my preferred setting ;o)
Such kind of parameters should rather be defined in the CON section at the very beginning, so they can easily be changed without changing something inside of the code.
Most important change was the setup of the counter. Your setup used PLLDIV of Vco / 128 which means that the counter runs much slower than the clock, but calculations are based on clkfreq.
What I do very often in terminal based projects is to wait until a key is pressed before really starting the program.
Maybe I'll take a night off, I've been trying to get this working for 2 weeks now and I may need some clarity by taking a step back. I see what you mean about the counter now, that was due to my ignorance, I was modifying what someone else wrote and need higher than a 500hZ resolution. I'll try this and report back my results.
Thanks again
PUB elapsed_time_msec(p_TimeVar) | RightNow, ClockTicks
is to complicated!
If you do
currentTime - startTime
your calculation would be right even if you had a wraparound. No need for this:
if RightNow < p_TimeVar ClockTicks := ||($FFFFFFFF - RightNow + p_TimeVar) else ClockTicks := ||(RightNow - p_TimeVar)just say
ClockTicks := RightNow - p_TimeVar
I did not use counters too often so far!
In retrospective:
I used turbosupras code with the counter connected to a quickstart LED. What I saw was that the LED in fact was blinking, but veeery slow. That's why I thought the counter would maybe run with the wrong frequency. And I changed it without a re-read of the manual.
At the same time I spotted the problem with the frequency setting and changed that as well. Looks like the real fix was the frequency setting.
@turbosupra:
Did you run the code? Is that what you want crank to do?
If you want to go higher than 500Hz, you can't live with an pulse width of 1ms. Simply change this as well and you can go higher.
Simply remove the calculation of pulse from crankPulser and put it into setFrequency like that:
pulse := timing >> 1
Additionally you can define a MAX_FREQUENCY constant and use that in setFrequency as well.
My method that uses the jm_freqin object is even able to calculate rpm from it
repeat localIndex from 1 to (missingTeeth) phsa := 0 ' create 1ms pulse waitcnt(sync += timing)I also noticed that my code would consider a "second" (as defined below) every 81600000 cycles instead of 80000000 which was also strange. I initially define timeStampCrank once the first time the method is called and then I try and enter the part of the code underneath of this, when cnt-l_timeStampCrank is greater than 80000000 clk cycles. I cannot for the life of me figure out why the timing would be so far off at 81.6 million cycles? Am I approaching this incorrectly?
if (l_timeStampCrank == 0) l_timeStampCrank := (cnt) if((cnt-l_timeStampCrank) > clkfreq)sync := cnt
....
waitcnt( sync += timing )
goes on with execution exactly after timing clockticks, except you missed the cnt-value which adds ~53seconds to the runtime. So, in your loop you maybe have to add some cycles for the condition-check of the repeat .. from, but this should be some hundred clocks.
Well ... with the cnt - l_timestampCrank - method you don't wait for an exact time. You want to continue in your program and start something if the time has exceeded. How much it can exceed of course depends on the other code running in that loop. If you have some serial output this might easily extend 1/10th of a second. Makes sense to post the whole code here to get an idea which code delays the loop.
The reason I want to measure time, is because I want to calculate how many times the following code is passed through each second, so # of times/sec.
repeat localIndex from 1 to (missingTeeth) phsa := 0 ' create 1ms pulse waitcnt(sync += timing)I am not doing any pst writing in that cog, (it is called in the main cog) as I learned my lesson on that oneI've attached an archive of the code, thank you for being willing to look at it.
clkfreq / (presentTeeth * timing ) gives you the answer. (Assuming you can live with integers)
Ok, no PST calls in between, but you still do a lot in between two calls of calculateCrankRpm. So, let's say you call calculateCrankRpm and the code reaches the if statement when cnt - l_timeStampCrank is equal to 79_999_999. Your code has to go another round:
1. return from calculateCrankRpm
2. call fcrank.freq - doing calculations
3. call fcrank.period
4. call calculateCrankRpm again which first runs through a lot of calculations
Pretty sure this can sum up to 1600000 additional clock cycles
What's the purpose of calculateCrankRpm? Looks like it's doing some averaging. Maybe you want to have a look into this thread http://forums.parallax.com/showthread.php?137868-Moving-average-question-from-a-Beau-Schwabe-reply
It looks like you could simply add a function getRPM into Crank.spin, which simply returns
clkfreq / (presentTeeth * timing ) * 60
I don't know enough about your problem domain, so I can't easily estimate what the purpose of the code is. Maybe you can explain the next step and we can develop your application step by step.
calculateCrankRpm allows me to take a rolling average of how many clk cycles it took to go from positive edge to positive edge, for the last 5 teeth. It also assumes that if the clk cycle count is bigger than the previous average multiplied by 1.1, that it is the missing teeth gap (which may also be a long solid tooth instead of a long gap, I have to verify this by disassembling something but it serves the exact same purpose either way), and it does not include that value in its rolling average. That thread was a good read.
If I added clkfreq / (presentTeeth * timing ) * 60, I could keep track of presentTeeth the way I keep track of l_Cnt, but I'm fuzzy on keeping track of the time I've done since last calculation (timing). The reason being, is that I should get l_Cnt to go to ~167 at 10000rpm, because this means the crank is rotating 167 times a second, and you get +1 to l_Cnt for each single full rotation. I'm not seeing anywhere near that, even when I go 81.6 million cycles, or about 1.02 seconds.
To help illustrate, here is an idea of what I my hall effect sensor trigger wheels look like, the "sensing" part is done by a hall effect sensor. My goal is to keep track of how fast I'm going from the positive edge of one tooth to the positive edge of another tooth and calculate rpm from it. I also want to be able to reference which tooth I am at in the rotation. Does that make sense? This sensor is the main component of a car/truck in telling the cars computer when to fire a fuel injector to a certain cylinder and when to fire a spark plug for a certain cylinder.
Is there a standard way to time how long a method takes, or is my l_timeStampSubtracted sufficient?
First of all thanks for the nice picture. But that's exactly what confuses me. With the code you write .. do you want to simulate these trigger wheels for external usage or do you want to read/measure the sensor signals produced by these wheels and the current crank-code is only for checking the measure-code?
I don't really remember where the code (cnt - lastMeasuredCnt) > timespan came from - was it in your original post. I remember that I corrected your first implementation ... however .... this kind of measurement is good for tasks that you want to perform somewhen around that given timespan. Or more precicely in the range of exactly this timespan or more. This kind of code is not good if you need exact results.
l_Cnt += 1 if((cnt-l_timeStampCrank) > clkfreq) l_timeStampCrankPrevious := l_timeStampCrank l_timeStampCrank := cntAllowing a deviation in the if statement (in time) means in the end that the numbers might also vary from one to the next try.If you really need this measurement-stuff, I'd suggest to add the moving average as outlined by Beau in the thread I mentioned into the PASM-part of Johns frequency measurement object. There you should have plenty of time for summing up some values.
Only cosmetical changes showing you how I'd do the output.
Doing these changes I also got some new questions:
What does l_toothGapRpm mean?
What does l_toothRpm mean?
What is ((clkfreq/fcrank.period)*60)/36 ? Am I right with labeling it as RPM?
And finally why should the RPMs be different at all?
If the wheel turned one round each of it's teeth also turned one round and the gap turned one round?
I know that I am crossing two very different worlds with the mechanics and technical terms of automotive, and the electronics and technical terms of circuitry. I try to keep the automotive things out of the discussion to keep things less confusing, but if you have any questions about that I can explain that in as much detail if you would like.
As for the different methods. I have found that it is much easier troubleshooting and writing code from inside of my house, then outside in the car ... possibly even on the couch or in bed, so I decided to try and use the prop to simulate the sensors as best as possible, before I sat out in the car for hours in my driveway with the laptop, extension cord and wasting a lot of gasoline
l_toothGapRpm is supposed to be the rpm calculated from the bridge or gap (the part I've circled in the picture below) . I believe the automotive manufacturers do this so that they can tell when an engine rotates past the tooth that they deem 0 or top dead center, but this is only speculation on my part. I do know however that it is there to differentiate when the 360 degree rotation starts.
l_toothRpm is supposed to be the rpm calculated from a single positive edge of one of those triggers, to the next positive edge of the following trigger. I have error checking in the code to throw out large variances such as the bridge/gap circled because the value would be way way off.
((clkfreq/fcrank.period)*60)/36 is rpm. It takes the amount of clk cycles between a rising edge and the previous rising edge before it and it then divides the clkfreq by that number as the denominator to calculate how many times a second that "period" would occur based on that single snapshot in time. Then it calculates the number of seconds in a minute, since Rpm is per minute and finally it divides by the number of times that "period" occurs before a single revolution of the trigger wheel has occurred. That sounded confusing when I wrote it, so I can explain better if it reads that way also.
I can't use straight frequency though, because of the delimiter that the automotive company has built into the hall effect trigger wheel, so I have to have logic that addresses that, which is why I have to take and average and insure that the new value is not outside of that average by a certain amount.
Ah, I did not know you could do that with the print screen, that is AWESOMEEEE! That's the coolest thing I have learned all day, does it by chance somehow save any overhead with bits? I like the constants too, that makes sense.
I'm not sure wheather I understand this question correctly! You mean: Does it save bandwidth? ?
Currently it needs more bandwidth, but as you only update the screen once per second that's currently not an issue. The point is that this way the positioning information has to be send first and that old values need to be overwritten completely. For example if you output 83444 first and then the number decreases to 11 you have to overwrite 444 with spaces or you'd see 11444.
But some extra code could be added, which only updates the values that really changed. Depending on the stability of the values you might save bandwidth.
Ok ... so measuring is the essence of your program. So I'll concentrate on that tomorrow. Which max. RPM do we talk about?
I don't believe I'd ever eclipse 10000rpm, so that would be a good ceiling.
Is this device you build for display-only or is it meant to control other hardware related to the input?
With the statement you gave earlier that you want to know which tooth is actually passing the sensor I'd expect you want to control something based on this information.
I think that Johns frequency measurement is a good starting-point. But I also think that you have to extend exactly this PASM driver to do all the number juggling.
At 10000RPM we have a tooth frequency of 10000/60*36 = 6000 teeth/second, right?. I think that it will be hard to do all the calculations you do in calculateCrankRpm can't in SPIN in 1/6000s. Maybe you could start several COGs sharing this task, but Johns PASM-driver has a lot of spare time (in PASM you have >3000 instructions which can be executed in this time) and it's the place where the input-numbers come from, so why not process them there?
Are you ready for PASM? ;o)
I will say that I am ready if you are willing to teach/guide, I'll do my best and hope to not frustrate you while trying to learn.
I scoped the sensor a while back, and found the pictures. The first is what the OEM manufacturer says about the signal, the next picture is of the sensor at 1000rpm (600 teeth a second), the third picture is of the sensor at 5000rpm (3000 teeth a second). I was planning on tapping into this signal (the factory computer uses this signal) with a resistor bridge and using it for controlling other things, you are correct about that. I built an application in c sharp that allows me to receive the data and display it in real time, as well as log it to an excel spreadsheet line by line.
' -------------------------------------------------------------------------------------------------- POS_DETECT long %01000 << 26 NEG_DETECT long %01100 << 26 tmp1 res 1 tmp2 res 1 mask res 1 ' mask for frequency input pin cyclepntr res 1 ' hub address of cycle count cycles res 1 ' cycles in input period fit 492Then method frcntr is started with its own cog, I don't understand why the pin is passed as a stack address long though. Or is cognew different in PASM because it automatically has a 492 long stack, so it doesn't need the stack defined?
Then the pin number is copied to tmp1, and after that the program will read the value of tmp1 and store it in tmp2.
After that, the time between positive edges is assigned to ctra register, then the value of tmp2 is added to ctra, not sure how this works but it has to be assigning the pin number to ctra. Then mov adds a literal 1 to frqa, I believe when ctra detects a rising edge.
The same is done for ctrb with falling edges in the next section.
Then the literal value of 1 is copied to mask, followed by mask being shifted left by the number of bits in tmp2 (the pin number), then a bitwise and operation is performed to dira on the ! of mask, possibly alternating back and forth between input and output?
After that a literal 4 is added to tmp1, then tmp1 is copied to cyclepntr
Then it appears to go into a loop.
wait for pin to not equal high (is this comparing the pin to itself and waits for it to equal something set in the register?) move a literal 0 to phsa to clear it wait for to equal high (looks like it compares it to itself again and waits for it to equal something set in the register?) move a literal 0 to phsb wait for pin to not equal high (looks like it compares it to itself again and waits for it to equal something set in the register?) move value of phsa to cycles wait for pin to equal high (looks like it compares it to itself again and waits for it to equal something set in the register?) add phsb to cycles containing phsa cycles already write value of cycles to cyclepntr goto restart
I don't see how this interacts and transfers the data to the spin portion. Does an assembly cognew automatically return a value when you assign it to a variable, like what is done here in the first spin method with okay and cog? Even if that were true, I still can't figure out how the assembly data is passed into the spin code? My brain hurts now, but I've done my best to try and understand the code.
dat org 0 frcntr mov tmp1, par ' start of structure rdlong tmp2, tmp1 ' get pin# mov ctra, POS_DETECT ' ctra measures high phase add ctra, tmp2 mov frqa, #1 mov ctrb, NEG_DETECT ' ctrb measures low phase add ctrb, tmp2 mov frqb, #1 mov mask, #1 ' create pin mask shl mask, tmp2 andn dira, mask ' input in this cog add tmp1, #4 mov cyclepntr, tmp1 ' save address of hub storage restart waitpne mask, mask ' wait for 0 phase mov phsa, #0 ' clear high phase counter highphase waitpeq mask, mask ' wait for pin == 1 mov phsb, #0 ' clear low phase counter lowphase waitpne mask, mask ' wait for pin == 0 mov cycles, phsa ' capture high phase cycles endcycle waitpeq mask, mask ' let low phase finish add cycles, phsb ' add low phase cycles wrlong cycles, cyclepntr ' update hub jmp #restart