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

Tim2: Trick to setting fine resolutions? #138

Closed
David-OConnor opened this issue Sep 9, 2020 · 8 comments
Closed

Tim2: Trick to setting fine resolutions? #138

David-OConnor opened this issue Sep 9, 2020 · 8 comments

Comments

@David-OConnor
Copy link
Contributor

David-OConnor commented Sep 9, 2020

Hi. I noticed after testing with an oscillosope, I'm having trouble setting fine resolutions using TIM2. Code:

    let tim2_channels = tim2(
        dp.TIM2,
        160_000,  // resolution of duty cycle
        94.hz(), // frequency of period
        &clocks, // To get the timer's clock speed
    );

    let mut pwm0 = tim2_channels.0.output_to_pa0(pwm0_pin);
    pwm0.set_duty(pwm0.get_max_duty() / 2); // 50% duty cycle
    pwm0.enable();

The scope reports a frequency of 100Hz. If set anywhere between 100 and 60, it outputs 100Hz. If set to 50, it outputs 50Hz. Above 100, and I don't see any output. Are there any gotchas here? Thank you.

@teskje
Copy link
Collaborator

teskje commented Sep 9, 2020

Looks like some kind of rounding issue. The relevant code is here in the pwm module.

It's pretty clear that the calculation of the timer values depends on the configured clocks, so may be worth playing around with those. Maybe increasing PCLK1 also increases the accuracy of the output frequency?

I'm afraid I won't be of much help here. The STM timers are extremely complex and I can't claim to understand them.

@David-OConnor
Copy link
Contributor Author

Ooh good point! I have been experimenting with the clocks, although main the .sysclk(). I wasn't able to get more precision that way, but haven't tried PCLK. Will try that and report back.

I need to dig into the ref man on timers and clocks in general on this. I've never used a scope before or tried configuring them.

@David-OConnor
Copy link
Contributor Author

David-OConnor commented Sep 14, 2020

After more experimenting, it seems several factors go into the frequency output. The clock config as you pointed out (Which I don't understand enough to comment on), and the resolution of duty cycle arg, ie 160_000 above, have interplay with the 94.hz() in ways I don't understand.

resolution of duty cycle appears to be the contents of the pac::tim2::arr register. I'm not sure what the frequency argument is; eg it's not the registers I've tried reading, and I can't figure out what it's doing from the macro source code.

From the source code:

The resolution should be chosen to offer sufficient steps against
your target peripheral. For example, a servo that can turn from
0 degrees (2% duty cycle) to 180 degrees (4% duty cycle) might choose
a resolution of 9000. This allows the servo to be set in increments
of exactly one degree.

It doesn't make mention of this affecting the frequency. It looks like the freq argument is affecting the psc register, for prescaling.

I think the answer may be to start clean from the registers and once working/understood, patch the timer and/or pwm modules, unless I'm missing something. It seems like the examples in examples/pwm.rs work by coincidence. Ie if you naively change to a diff "frequency" or "resolution", it won't do what you expect. Those should be what you tweak from a high-level perspective, but I don't think they're mapping to the registers correctly.

@IamfromSpace
Copy link
Contributor

@David-OConnor Hi David, I originally wrote much of this module, and for the most part this speaks to the challenge in finding a natural interface here. Though the examples themselves may be "too optimized," in that if you fiddle with them, yeah, they don't do what you expect :(

What's happening (more or less) is that there are 8M clock ticks for TIM2 (w/ typical config) to work with. We need our resolution and frequency to divide cleanly, and if we miss, then frequency is rounded to be as close as possible (this may seem silly, but I'll explain). So in the example file, the 50hz gives us a lot to play with and a nice round number, and we can use the entire remainder of 160,000 as the resolution (8M / 50).

The reason we don't want to throw an error if something doesn't divide nicely is immediately apparent in your example. We want 94hz, but this leaves us a very awkward remainder of 85106.38287...., so it's not possible to get exactly 94hz, we'll need to settle. And the more depth and less awkward our resolution, the more error we'll introduce. So a reasonable resolution here might be 1000, and in this case we'll end up with a frequency of 94.1176hz. Hopefully close enough.

ignoring that a resolution of 1 is completely useless, it still can't give us exactly 94hz, we still end up with 94.000423. So exactly 94hz is simply not achievable.

If we're willing to sacrifice having a perfectly round number, we can use 851 as our resolution, we'll get 94.0071hz. If that's not good enough then we're almost totally out of options.

Now, this is hopefully helpful, but I do think that this is pretty problematic--you certainly won't be the only person with this question. Unfortunately, I'm not sure there's any obvious answer. We're trying to abstract away something that's just kind of painful to hide. This is essentially why at the register level you only get dividers that are offset by 1. It's super ugly and not intuitive to work with, but you never get "wrong" answers--it just makes the buyer-beware behavior really apparent.

My immediate thought is that the current interface just tries to have its cake and eat it too. If the user wants to think in terms of frequency (and I imagine most do), then they shouldn't also get to provide a resolution. In the 94hz case, your resolution would have been automatically set to 85106. Then maybe a power user interface forces you to work with low level dividers.

Hopefully this is helpful in explaining what's going on, and then curious to get thoughts on how to either clear this up or alternatives.

@David-OConnor
Copy link
Contributor Author

David-OConnor commented Sep 14, 2020

Thank you very much! That makes a lot of sense, and as you point out, it's not immediately clear what the best approach is. I had assumed higher resolution number meant more precise, but it's the opposite; I never tried lowering it. I can confirm that using values as you suggest, the measured freq converges on 94 (etc). I was wrong about it being a complex interaction between multiple registers, as once you lower the resolution value (ARR register?) the freq converges on the set one.

One way forward is to document this explicitly in the source code and/or docs, without changing behavior.

Is there a cost to setting a low resolution value? Eg do you think it's appropriate to have the API expose frequency, and it auto picks an appropriate resolution to get close?

Why does a resolution of 85,106 introduce more errors than 1,000?

@IamfromSpace
Copy link
Contributor

IamfromSpace commented Sep 14, 2020

I had assumed higher resolution number meant more precise, but it's the opposite

Ah, I hadn't considered that resolution would be ambiguous! That makes sense. And agreed, the immediate solution is to clarify this behavior in the docs.

Is there a cost to setting a low resolution value?

The main downside of a low resolution is that you may not have the steps you need for your given application. For a servo that ranges from 5-10% for 0-180 degrees, you need at least 3600 resolution to get a single degree accuracy. And if you have that resolution you can rotate exactly one degree by simply adding 1 to the duty cycle, which is nice. Something nasty like 3757 (totally arbitrary btw) can't fall exactly on each degree. Most applications probably don't have to consider this level of detail, but in theory, some will.

Eg do you think it's appropriate to have the API expose frequency, and it auto picks an appropriate resolution to get close?

Yeah, I'm starting to lean this way, resolution is automatically chosen based on the frequency (in at least the high level interface). The idea here would be to set the prescaler to maximize the resolution up to the bit depth of the individual timer. A lower level interface could allow overwriting this value. Perhaps resolution should just be an Option value.

Why does a resolution of 85,106 introduce more errors than 1,000?

85106 would be better than 1000 in that it'll be closer to 94hz and provide higher fidelity. However, the mental model for manipulating the duty cycle won't be as obvious.

@David-OConnor
Copy link
Contributor Author

David-OConnor commented Sep 15, 2020

Related: Does anyone know why the following isn't working? Here's an existing method on timers from timers.rs:

                /// Stops the timer
                pub fn stop(&mut self) {
                    self.tim.cr1.modify(|_, w| w.cen().disabled());
                }

I'm adding this, which is p much the same, but a diff field:

                /// Set timer alignment to Edge, or one of 3 center modes.
                /// STM32F303 ref man, section 21.4.1:
                /// Bits 6:5 CMS: Center-aligned mode selection
                /// 00: Edge-aligned mode. The counter counts up or down depending on the direction bit
                /// (DIR).
                /// 01: Center-aligned mode 1. The counter counts up and down alternatively. Output compare
                /// interrupt flags of channels configured in output (CCxS=00 in TIMx_CCMRx register) are set
                /// only when the counter is counting down.
                /// 10: Center-aligned mode 2. The counter counts up and down alternatively. Output compare
                /// interrupt flags of channels configured in output (CCxS=00 in TIMx_CCMRx register) are set
                /// only when the counter is counting up.
                /// 11: Center-aligned mode 3. The counter counts up and down alternatively. Output compare
                /// interrupt flags of channels configured in output (CCxS=00 in TIMx_CCMRx register) are set
                /// both when the counter is counting up or down.
                pub fn set_alignment(&mut self, alignment: Alignment) {
                    let word = match alignment {
                        Alignment::Edge => 0b00,
                        Alignment::Center1 => 0b01,
                        Alignment::Center2 => 0b10,
                        Alignment::Center3 => 0b11,
                    };
                    self.tim.cr1.modify(|_, w| w.cms().bits(word));
                }

Error:

 ^^^ method not found in `&mut stm32f3::W<u32, stm32f3::Reg<u32, stm32f3::stm32f303::
tim6::_CR1>>`

Maybe this is something about only some timers having certain fields? How should we handle this?

If this is the case, do we need to redo the module, perhaps with a sep struct for each timer? (Or for each timer class, eg
we can lump 2, 3, 4 together)

Or standalone fns that take enums as args:

timer::set_alignment(Tim2, Alignment::Center1);

edit: The pwm module goes straight for the raw pointers:

            fn enable(&mut self) {
                unsafe {
                    (*$TIMx::ptr())
                        .ccer
                        .modify(|_, w| w.$ccx_enable().set_bit());
                }
            }

Maybe because there's an ownership issue of timer struct vs pwn struct?

Edit again: I'm not sure how to approach this. I tried splitting up the timer.rs macros by category and implementing the missing features, but they get in an ownership fight with pwm.rs consuming the dp.TIMx peripheral. This is presumably why pwm.rs uses raw pointers.

What do you think is the way out of this? Mutable borrow? I know the HAL ecosystem is based on owning, but I don't think it makes sense in this case. (See also: busses like SPI) Roll the pwm.rs methods into the Timer struct?

Edit - another idea: Do all the config in timer.rs, and pass timer::Timer into the pwm struct, instead of passing dp.TIMx. I think you could dodge the raw pointers entirely this way, and add the features you want.

#141

@David-OConnor
Copy link
Contributor Author

Closed in favor of PR.

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

No branches or pull requests

3 participants