Divide by 16 boxcar averaging not working properly - 9SO8 assembly

[Circlotron]

Got a little project going where I want to set or clear a bit depending on whether an input trigger pulse is above or below 120Hz. I grab the 1.25MHz counter reading every trigger pulse and take the average of the last 16 readings and compare it to a value. The thing is, if I don't use the averaged value it sets or clears as good as dead on 120Hz, no problem, but if I use the averaged value it sets or clears at 119.09Hz. Sure, I lose the the 4 LSBs when I right shift 4 times, but this is an error approaching 1%, way more than those 4 bits would account for. Pretty sure the lost bits are all zeros anyway. I just can't figure out what's happening. This is the relevant portion of the code. And, yeah, it's hardly optimal, I know, but it should work. Any ideas? assembly suggestions only, not C.

Code: [Select]

********************************************************************************
WAIT_TRIG BRSET TRIG,PORTB,WAIT_TRIG ;inverted trigger from coil
LDHX TCNTH ;get current counter value (1.25MHz)
STA TCNTH ;reset counter
STHX TEMP_CTR_MSB

JSR PUSH_DOWN ;push previous counter readings down one place
LDHX TEMP_CTR_MSB ;latest count
STHX MSB_1 ;put latest on the pile
BCLR TOF,TSC ;just in case
JSR ADD_UP ;add up last 16 timer periods
JSR DIV_16 ;average = MSB_TOT:LSB_TOT
*********************************************
LDHX MSB_TOT ;averaged count
CPHX #10417 ;1800rpm or less, 120Hz, 8.333mS
BHS CLR_BIT ;clear if < 120Hz
BSET 0,PORTA ;set if > 120Hz
JMP WAIT_TRIG

CLR_BIT BCLR 0,PORTA ;
JMP WAIT_TRIG
********************************************************************************
;push last 16 counter readings down one place

PUSH_DOWN LDHX MSB_15
STHX MSB_16

LDHX MSB_14
STHX MSB_15

LDHX MSB_13
STHX MSB_14

LDHX MSB_12
STHX MSB_13

LDHX MSB_11
STHX MSB_12

LDHX MSB_10
STHX MSB_11

LDHX MSB_9
STHX MSB_10

LDHX MSB_8
STHX MSB_9

LDHX MSB_7
STHX MSB_8

LDHX MSB_6
STHX MSB_7

LDHX MSB_5
STHX MSB_6

LDHX MSB_4
STHX MSB_5

LDHX MSB_3
STHX MSB_4

LDHX MSB_2
STHX MSB_3

LDHX MSB_1
STHX MSB_2
RTS
*************************************
;Add up latest 16 counter readings

ADD_UP CLR     LSB_TOT
                CLR     MSB_TOT
                CLR     EXT_TOT

                LDA     LSB_TOT
                ADD     LSB_1
                STA     LSB_TOT
                LDA     MSB_TOT
                ADC     MSB_1
                STA     MSB_TOT
                LDA     EXT_TOT
                ADC     #0
                STA     EXT_TOT

                LDA     LSB_TOT
                ADD     LSB_2
                STA     LSB_TOT
                LDA     MSB_TOT
                ADC     MSB_2
                STA     MSB_TOT
                LDA     EXT_TOT
                ADC     #0
                STA     EXT_TOT

                LDA     LSB_TOT
                ADD     LSB_3
                STA     LSB_TOT
                LDA     MSB_TOT
                ADC     MSB_3
                STA     MSB_TOT
                LDA     EXT_TOT
                ADC     #0
                STA     EXT_TOT

                LDA     LSB_TOT
                ADD     LSB_4
                STA     LSB_TOT
                LDA     MSB_TOT
                ADC     MSB_4
                STA     MSB_TOT
                LDA     EXT_TOT
                ADC     #0
                STA     EXT_TOT

                LDA     LSB_TOT
                ADD     LSB_5
                STA     LSB_TOT
                LDA     MSB_TOT
                ADC     MSB_5
                STA     MSB_TOT
                LDA     EXT_TOT
                ADC     #0
                STA     EXT_TOT

                LDA     LSB_TOT
                ADD     LSB_6
                STA     LSB_TOT
                LDA     MSB_TOT
                ADC     MSB_6
                STA     MSB_TOT
                LDA     EXT_TOT
                ADC     #0
                STA     EXT_TOT

                LDA     LSB_TOT
                ADD     LSB_7
                STA     LSB_TOT
                LDA     MSB_TOT
                ADC     MSB_7
                STA     MSB_TOT
                LDA     EXT_TOT
                ADC     #0
                STA     EXT_TOT

                LDA     LSB_TOT
                ADD     LSB_8
                STA     LSB_TOT
                LDA     MSB_TOT
                ADC     MSB_8
                STA     MSB_TOT
                LDA     EXT_TOT
                ADC     #0
                STA     EXT_TOT

                LDA     LSB_TOT
                ADD     LSB_9
                STA     LSB_TOT
                LDA     MSB_TOT
                ADC     MSB_9
                STA     MSB_TOT
                LDA     EXT_TOT
                ADC     #0
                STA     EXT_TOT

                LDA     LSB_TOT
                ADD     LSB_10
                STA     LSB_TOT
                LDA     MSB_TOT
                ADC     MSB_10
                STA     MSB_TOT
                LDA     EXT_TOT
                ADC     #0
                STA     EXT_TOT

                LDA     LSB_TOT
                ADD     LSB_11
                STA     LSB_TOT
                LDA     MSB_TOT
                ADC     MSB_11
                STA     MSB_TOT
                LDA     EXT_TOT
                ADC     #0
                STA     EXT_TOT

                LDA     LSB_TOT
                ADD     LSB_12
                STA     LSB_TOT
                LDA     MSB_TOT
                ADC     MSB_12
                STA     MSB_TOT
                LDA     EXT_TOT
                ADC     #0
                STA     EXT_TOT

                LDA     LSB_TOT
                ADD     LSB_13
                STA     LSB_TOT
                LDA     MSB_TOT
                ADC     MSB_13
                STA     MSB_TOT
                LDA     EXT_TOT
                ADC     #0
                STA     EXT_TOT

                LDA     LSB_TOT
                ADD     LSB_14
                STA     LSB_TOT
                LDA     MSB_TOT
                ADC     MSB_14
                STA     MSB_TOT
                LDA     EXT_TOT
                ADC     #0
                STA     EXT_TOT

                LDA     LSB_TOT
                ADD     LSB_15
                STA     LSB_TOT
                LDA     MSB_TOT
                ADC     MSB_15
                STA     MSB_TOT
                LDA     EXT_TOT
                ADC     #0
                STA     EXT_TOT

                LDA     LSB_TOT
                ADD     LSB_16
                STA     LSB_TOT
                LDA     MSB_TOT
                ADC     MSB_16
                STA     MSB_TOT
                LDA     EXT_TOT
                ADC     #0
                STA     EXT_TOT

RTS
*************************************
;divide total of last 16 counter readings by 16

DIV_16 LSR     EXT_TOT
                ROR     MSB_TOT
                ROR     LSB_TOT

                LSR     EXT_TOT
                ROR     MSB_TOT
                ROR     LSB_TOT

                LSR     EXT_TOT
                ROR     MSB_TOT
                ROR     LSB_TOT

                LSR     EXT_TOT
                ROR     MSB_TOT
                ROR     LSB_TOT

RTS
********************************************************************************
********************************************************************************
                ORG     $FFFE
                FDB     START
********************************************************************************


[fourtytwo42]

Not being an expert in your particular instruction set (I am more ARM & PIC) something that has gotten me several times in assembly arithmetic is not bounds checking properly and getting random errors as a result. I have even before convinced myself that an overflow is impossible only to discover it happening 

[Circlotron]

The counter rollover value is set to #46875 (37.5mS) so that fits fine into 16 bits. In any case the value we are looking for is #10417 and we are right close to it (but not close enough!) when dithering on the threshold. We have 24 bits available for the 16 16-bit counter values summed so heaps of space there too. In any case, #10417 is 14 bits and the total is an 18 bit value which of course goes back down to 14 bits when right-shifted. It's just weird.

[ozcar]

I don't see any obvious typos or ??

I'd be inclined to test it by loading a constant value (in the vicinity of 10417) instead of the counter, and see what comes out. Or maybe you already did something like that?

Edit:

...and, presumably you have checked that your 1.25MHz really is, well, 1.25MHz?

[Circlotron]

Okay, I’ll try that. The thing runs off a 10MHz oscillator module that measures about 9Hz low so that’s pretty right. The counter rate must presumably be 1.25MHz as expected because the thing works just fine if I don’t use the averaging.

[Circlotron]

Soooo.... after a bit more investigation it appears that the problem is noise on the input trigger signal. If I apply an artificial trigger signal straight from the function generator the detection threshold is 119.989Hz, plenty close enough for me. There is a small amount of latency because I’m using a polling loop. Why it worked okay without the averaging is still a mystery though. That will have to do for now. Maybe I’ll investigate it further later.

[Nominal Animal]

How do you define MSB_n and LSB_n?  Your PUSH_DOWN subroutine assumes MSB_n has smaller address than the corresponding LSB_n (big-endian byte order), but if they are in the wrong order, PUSH_DOWN will mangle the data.

[ozcar]

Looking at it again, I can't figure how it is supposed to work. You spin at WAIT_TRIG until the signal goes low before grabbing the count value. If you get back to WAIT_TRIG before the signal has gone high again, don't you drop straight through and end up inadvertently measuring how long it took you to do your calculations? I must be missing something really obvious, or it would not work at all.

Also, you don't seem to do anything to prevent trying to calculate average of last 16 samples, before you have that many? That would quickly resolve itself, but could cause initial glitch.

[Circlotron]

How do you define MSB_n and LSB_n?  Your PUSH_DOWN subroutine assumes MSB_n has smaller address than the corresponding LSB_n (big-endian byte order), but if they are in the wrong order, PUSH_DOWN will mangle the data.

Yep, that is correct. From the databook regarding STHX:

Stores the contents of H in memory location M and then the contents of X into the next memory location (M + $0001).

[Circlotron]

Looking at it again, I can't figure how it is supposed to work. You spin at WAIT_TRIG until the signal goes low before grabbing the count value. If you get back to WAIT_TRIG before the signal has gone high again, don't you drop straight through and end up inadvertently measuring how long it took you to do your calculations? I must be missing something really obvious, or it would not work at all.

Also, you don't seem to do anything to prevent trying to calculate average of last 16 samples, before you have that many? That would quickly resolve itself, but could cause initial glitch.

You are certainly right, but in this particular case the trigger pulse is only about 10us wide so it is finished before the code is done executing. As for less then 16 samples while starting up, the averaged value will ratchet up from zero which is fine in this particular case. Thanks for your observations.