Skip to content
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

(Breaking) Add more res/freq configurations to PWM #196

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

IamfromSpace
Copy link
Contributor

This is still a bit in draft form, but I was wanting to get more feedback sooner. Also, this relates very closely to other timer discussions that I've seen going about recently, but haven't closely followed, so I wanted to makes sure that this aligned with what other folks were seeing for things like this going forward.

Issues of intuitive configuration have come up in both #138 and now #195. A single path to configuring the registers is probably not sufficient, as it seems there will always be setups that don't quite line up right. This adds a trait to get the final register values based on the current timer clock. A power user might just send in these values directly (the trait impl of just the identity function), while there may be a variety of other possible structs that allow the values to be calculated differently. Ideally, picking a config style that matches the intent more frequently leads to correct results.

@David-OConnor
Copy link
Contributor

David-OConnor commented Jan 17, 2021

Thoughts on returning as part of the result, a percent (or portion) off from requested?

edit: This is subjective, but I'm not sure this abstraction goes to the core of the issue. It's usually possible to pick a PSC and ARR that give close to the desired freq or period; we need an algorithm that finds a good combo. One approach would be to find as close of a PSC and ARR as we can, then return it, and the the portion off from ideal. Also, raise an error if the timer period is too long. (Ie requires maxing out PSC and ARR)

What's the intent behind the user manually entering ARR? (But not PSC)

@IamfromSpace
Copy link
Contributor Author

IamfromSpace commented Jan 17, 2021

The idea here is that we expose multiple strategies to a user, ideally they gravitate to the one that has the fewest leaks for their use case

So another one to add might be MaximizeResolution where a frequency tolerance is suppied.

Another (I hadn’t thought of this before your comments in #195) could be MaximizeFreqAccuracy, where ARR only gets what’s left over (or nothing). That works great as a general timer approach.

In theory, this is a good place in the docs where we can both describe typical strategies, their use-cases, and pitfalls.

It may make sense to return a result, so certain strategies can signal when/why the fail, but my expectation is that most consumers (the timers) wouldn’t really be able to do much besides panic at that point anyway.

@IamfromSpace
Copy link
Contributor Author

Just added an interesting approach that seems to check a lot of boxes. The PrioritizeResolution strategy tries to find the largest resolution that doesn't overly sacrifice frequency accuracy. With such a high resolution as well, it's likely that the duty cycle can be controlled finely enough that individual steps don't accumulate their own error.

Here's an example calculation of the error bounds, from in the comments (should likely be in the docs as that gets ironed out):

// Example:
//   - Fi = 72MHz
//   - Fo = 50Hz
//   - Max Resolution = 2^16 (standard timer)
//
// Then:
//   - Worst Case Error ~= 0.00146%
//   - Worst Case Frequency ~= 50.000728Hz
//   - Actual Frequency ~= 0.000833%
//   - Actual Frequency ~= 50.0004167Hz

Also, whenever the division doesn't need to be factored (always the case for u32 high resolution timers), the full value goes into ARR and there's no (extra) loss in accuracy from this approach. That's >1098Hz for standard timers.

Thoughts on this? Hopefully it seems appealing.

@IamfromSpace
Copy link
Contributor Author

Just added a PrioritizeFreq strategy which, @David-OConnor, is similar to your square root approach. However, this looks for smallest factors first, in hopes that we find an imbalanced factoring, so we can arrive at a large resolution too.

This just uses simple wheel factorization because it has O(1) space complexity, with minimal memory usage, and is deterministic. The classic approaches here for smaller numbers are a lookup table (way too large) or probabilistic algorithms. It seemed inappropriate to add randomness here, not knowing the ordering of factors seems like a major downside in getting nice resolutions, and it seems like most inputs in practice will have many small factors (lots of 2s and 5s), so our wheel should work well in many cases.

External package options seemed mostly geared around cryptography, mostly didn't support no_std, or still consumed a lot of memory via allocators. I tried to do my research here, but I'm not a number theory expert, so there may yet be better ways to do this. The hardware constraints seem to severely limit options though.

Lastly, we don't get the optimal resolution, because by always taking the smallest factor, they're might be a combination with a larger factor that's better (ex. factors are 2,2,3 where 4 makes the resolution small enough, but so does 3). To solve that (something like PrioritizeFreqThenMaximizeRes) we encounter the Subset Product Problem which is NP-Complete, so I'm not even sure it's worth recommending if we added it.

@IamfromSpace
Copy link
Contributor Author

From a general standpoint, this is strategy is a good one for timers not in PWM mode as well, so it's worth exploring lifting this up a level as a general timer configuration approach. Notably, this could also answer #190 because a variant of these strategies could accept a ratio. Something like:

OptimizeFreqRatio {
    frequency_numerator: u32,
    frequency_denominator: u32,
}

So you could represent a period of 9.5s with something like:

OptimizeFreqRatio {
    frequency_numerator: 19,
    frequency_denominator: 2,
}

I can take a look at extending the code here to support these as strategies as well.

Does this seem like a good solve?

@David-OConnor
Copy link
Contributor

David-OConnor commented Feb 1, 2021

I'll add your wheel factorization approach to my forks here and on L4 after testing. The sqrt approach I have is easy to understand, doesn't overflow, and is often good enough, but your approach looks better.

A note on performance: Approaches that involve extra calculations to get more precision aren't ideal if you're setting a timer multiple times throughout program run, but are generally fine if used on initialization only. If you're setting a timer regularly, it may be best to per-calculate ARR and PSC and set directly regardless of how you found them.

@IamfromSpace
Copy link
Contributor Author

Great—note this is currently untested. My immediate goal has been to prove things out and make sure I don’t work in a bubble and miss things that others would have identified up front. I’ll prioritize testing this out next and let you know when it’s good to go. In theory, it could come in as a separate PR/module. The good thing is that these kinds of things unit test quite nicely.

That’s good callout on reuse. This technically can support it currently but it’s not immediately obvious. At the end of the day, you need the register values. These strategies all have a trait which allow you to calculated them. However, the register values themselves satisfy the trait implicitly.

So my expectation for usage was to pass in strategies (which especially makes sense for PWM, where you are likely to configure once). However, it’s also possible to pre-calculate manually, but simply applying the trait and holding the resulting register values for later—since they themselves satisfy the trait to configure a timer.

A wrinkle there is that the resulting calculation is only guaranteed to be correct per timer. In the event that you pass in a strategy for a timer that differs in input frequency from the original, you’ll get bad results. Maybe we could add a phantom type for the timer it was calculated for? Or maybe there are other alternatives?

@Sh3Rm4n
Copy link
Member

Sh3Rm4n commented Mar 7, 2021

Thank you for working on this. I haven't looked to deeply into this, so I can't add anything constructive on the design site of things yet. But I really like the aim to provide multiple solutions to the user\1

If a review of the code is needed, just ping me.

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.

3 participants