Skip to content

Add pulseio.FrequencyIn #1144

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 29 commits into from
Closed

Conversation

sommersoft
Copy link

@sommersoft sommersoft commented Aug 25, 2018

Additions

  • pulseio.FrequencyIn
    • SAMD21 and SAMD51 are working. Accuracy is within ± 1kHz for me so far. Testing throughout has been with the use of a PWMOut program. EDIT: Accuracy is more like ±1%...
    • nRF and ESP8266 are currently stubbed out. Will start working nRF soon.

Changes

  • Moved shared_timer_handler() out of samd-peripherals and back into CircuitPython.
    • New location is .../atmel-samd/timer_handler.c.
    • Updated samd-peripherals submodule.
    • Updated all uses of shared_timer_handler() in common-hal modules.

Issues

Size. pIRkey will fail Travis for the French translation. But, even beyond that, FrequencyIn eats up ~1.5K of the flash, which is what I consider a large problem on non-Express builds. During a discussion with @dhalbert on Discord, decreasing the -finline-limit was mentioned with a caveat that I have no interest in defeating. I'm kind of hoping this review will point out some obvious waste I'm conducting in my code... If that doesn't bare fruit though, I figure excluding FrequencyIn from non-Express builds will be the best option.

@sommersoft sommersoft requested a review from tannewt August 25, 2018 18:41
@sommersoft sommersoft requested a review from dhalbert August 25, 2018 18:41
@jepler
Copy link

jepler commented Aug 25, 2018

This is relevant to my interests. I have a metro m4 express and a GPS referenced oscillator, is there some useful testing I can do?

@sommersoft
Copy link
Author

sommersoft commented Aug 25, 2018

@jepler,

Test? Yes, please! That would be much appreciated.

Usage is pretty easy:

import board
import pulseio

freq_in = pulseio.FrequencyIn(board.PIN)

# returns the reading in hertz
freq_in.value

In current state, it will not detect over ~512kHz. It should pause detection above that. I'll take the DMA leap eventually...

@jepler
Copy link

jepler commented Aug 26, 2018

I gave this a whirl with an Adafruit Metro M4 Express together with the this GPS referenced RF generator.. In one terminal I connect to the M4, and in the other I connect to the RF generator and send frequency commands.

With the frequency set to 1kHz,

>>> sum(f.value for i in range(1000)) / 1000
958.077

With the frequency set to 2kHz,

>>> values = [f.value for i in range(1000)]
>>> min(values)
1903
>>> max(values)
1923
>>> sum(values) / len(values)
1915.54

The two frequency averages are in a ratio of 1.99936:1, which is close to the true 2:1 to within 1 part in 1000, which is fine. However, things are systematically about 5% (5000ppm) low, probably owing to the low quality of the frequency reference on the M4 Express. There's also a pretty high sample to sample variance of around 1%; I don't know whether this is expected or unexpected.

When I reprogram the frequency source to 200kHz, it still works. But at 300kHz -- even without actually querying the frequency -- the board hangs.

[changes frequency to 200kHz]
>>> sum(f.value for i in range(1000)) / 1000
192371.0
[changes frequency to 300kHz]
>>> [circuitpython hangs]
[changes frequency to 1Hz]
[circuitpython remains hung]

For values below 1kHz, the results don't seem very good either. For example,

[changes frequency to 800Hz]
>>> [f.value for i in range(10)]
[763, 763, 762, 765, 766, 765, 764, 765, 765, 765]
[changes frequency to 700Hz]
>>> [f.value for i in range(10)]
[16838, 16850, 16931, 16943, 16931, 16919, 16858, 16919, 16931, 16931]
[changes frequency to 1Hz]
>>> [f.value for i in range(10)]
[19985, 19956, 19956, 19956, 19942, 19956, 19970, 19999, 19999, 20013]
>>> [f.value for i in range(10)]
[183550, 183550, 183681, 183550, 183681, 183681, 183681, 183550, 183286, 183550]

(those last two values, differing by a factor of 9x, were both counted when the input signal is a 1Hz square wave!)

@sommersoft
Copy link
Author

sommersoft commented Aug 26, 2018

When I reprogram the frequency source to 200kHz, it still works. But at 300kHz -- even without actually querying the frequency -- the board hangs.

☹️ Escape hatch fail. And possibly a little something else, too. I might need to re-visit emulating how @tannewt changed PulseIn to keep from endless looping.

For values below 1kHz, the results don't seem very good either.

The TC overflow flag has been shaky throughout my debugging; never could get it to work out sub-1kHz reliably.

However, things are systematically about 5% (5000ppm) low, probably owing to the low quality of the frequency reference on the M4 Express. There's also a pretty high sample to sample variance of around 1%; I don't know whether this is expected or unexpected.

I expected this. The usage of FREQM to check the frequency of DFLL is inside the call to get the value; not the interrupt handler. So, the DFLL has likely changed frequencies enough to make a difference when the value is calculated. When I had the frequency check inside the interrupt handler, I got consistent HardFaults.

Overall, I am not surprised by your results. Hoped for better, but confirmation some of the issues I encountered is good. Thank you @jepler for running these tests!

@sommersoft
Copy link
Author

sommersoft commented Aug 27, 2018

EDIT: This applies to SAMD51 only...

Through additional brainstorming, and discussion over in samd-peripherals, I'm wondering if I should setup the TC using a different clock than DFLL.

I could use DPLL1, set below 100MHz to ensure that TC4-7 don't exceed that limit. Then, all the kludgeyness of using the FREQM to adjust for DFLL drift wouldn't be necessary. In my quick-looks, DPLL1 isn't used, but I still need to roll through the common-hal to verify that.

I lose the "easy button" of using the established timer/clock functions in samd-peripherals, but that's not a biggie in my book.

@tannewt
Copy link
Member

tannewt commented Aug 28, 2018

Thank you for your continued work on this. I think it should only be added on express boards with space available. We don't have room for new things in the non-express builds.

What is the lower bound of what you are trying to measure? What's the high bound? Interrupting on every pulse seems very limiting to the top end.

Why not interrupt at a fixed frequency based on the best calibrated clock and count the number of clock cycles over that duration? The TC event action would then be COUNT instead of PPW. The upper bound will be then based on the TC limits (100mhz) and range.

@sommersoft
Copy link
Author

I think it should only be added on express boards with space available.

👍 I'll work out the least intrusive way I can do this (and hopefully not set a nasty precedent).

What is the lower bound of what you are trying to measure? What's the high bound?

My very lazy answer to this is "whatever is capable". I think sub-1kHz would be a nice lower bound, but would be easily convinced to raise that when balancing usability and overall firmware impacts. A lot of that depends on implementation (ability to capture count wraparound...). High bound...I invoke Scotty: "I'm giving her all she's got, Captain". 😄

Why not interrupt at a fixed frequency based on the best calibrated clock and count the number of clock cycles over that duration? The TC event action would then be COUNT instead of PPW.

I've spent the last day pondering on your meaning of this. I can't bridge "fixed frequency interrupt and counting cycles" with knowing the state of the pin. Here are a few things I've walked through while thinking about this:

  • Using the TC to capture the pin state, and changing to count events (TC.EVACT = COUNT). This still triggers the interrupt in the same manner. Unless you change INTSET = OVF so that the interrupt only happens when the count wraps around. The CC[n] is still updated on each event, but that seems fraught with failed readings on the boundary. Or, am I thinking too cautiously?

  • Making it more along the lines of PulseIn (not a bad thing), but just having the durations counted by rise-to-rise as opposed to each rise-to-fall. This would eliminate the need for the TC completely with only the EIC required and generating interrupts. Could maybe even just add the option to PulseIn to either report both period and pulse or report period info alone.

@tannewt
Copy link
Member

tannewt commented Aug 30, 2018

The first is closer to what I'm thinking.

I should have been more explicit that the approach will use two TCs. One will count cycles of the input frequency and the second will measure the fixed duration (say 1ms). So, the first won't interrupt at all. The second will on overflow to generate the reference period and an interrupt. The second can also be shared if there are multiple frequencies in. In the interrupt handler you read and reset the first TC count and then compute the frequency based on the count and sample period. It'll end up averaging over a number of source cycles but that should help.

@sommersoft
Copy link
Author

Ahh, two TCs makes sense. Tried switching simply the INTENSET to OVF, and keeping PPW during the shows tonight. Didn't think that through; counter resets automagically when a capture is taken so overflow wouldn't trigger the interrupt until there was no signal. 😄

Working the dual TC approach now...then will work the clock source.

@sommersoft
Copy link
Author

Seeing as how this PR is REALLY stale, would it be better to close this and open a new PR for my current iteration? I'm close (hopefully this weekend), but have been thinking about this...

@dhalbert
Copy link
Collaborator

dhalbert commented Mar 1, 2019

Sure, no need to ask permission.

@sommersoft
Copy link
Author

Thanks @dhalbert. Only concern was obfuscating the history. I was planning to reference this PR in the new one at any rate. Will close once the new one is up.

@sommersoft
Copy link
Author

Closing in favor of #1614.

@sommersoft sommersoft closed this Mar 3, 2019
@sommersoft sommersoft deleted the freq_in branch September 26, 2019 02:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants