Can't change PWM frequency after instanciating a Pwm struct

[azerupi]

Hi,

I'm trying to change the frequency of a PWM pin dynamically. This is impossible with the currant API because the method to change the frequency is on the timer, but the timer contains all the pwm channels and when you 'associate' a pwm channel to a pin it is moved out of the timer struct. When you then try to configure the pwm frequency you get a 'borrow of partially moved value' error.

Example:

let mut pwm_timer = Timer::new(peripherals.TIM2, 100.hz(), &mut rcc);
let front_led_pwm_pin = pwm_timer.channel1.assign(gpioa.pa0);

// Some code

pwm_timer.set_frequency(10.hz(), &rcc);

Resulting in the following error:

error[E0382]: borrow of partially moved value: `pwm_timer`
   --> src/main.rs:104:5
    |
102 |     let front_led_pwm_pin = pwm_timer.channel1.assign(gpioa.pa0);
    |                                                ----------------- `pwm_timer.channel1` partially moved due to this method call
103 |
104 |     pwm_timer.set_frequency(10.hz(), &rcc);
    |     ^^^^^^^^^ value borrowed here after partial move
    |
note: this function takes ownership of the receiver `self`, which moves `pwm_timer.channel1`

Workaround

I discussed this on the matrix channel a bit and I currently have two workarounds requiring unsafe:

1. Change the registers directly

   unsafe {
       (*pac::TIM2::ptr()).psc.write(|w| w.bits(psc_val));
       (*pac::TIM2::ptr()).arr.write(|w| w.bits(arr_val));
   }

   This works but you loose the niceties provided by the set_frequency function (e.g. provide a frequency in Hz).

2. Make a raw pointer to the timer object to bypass Rust's ownership

   let pwm_timer_ptr: *mut Timer<TIM2> = &mut pwm_timer;
   // Some code
   unsafe { 
       (*pwm_timer_ptr).set_frequency(frequency.hz(), &context.rcc); 
   }

   This also compiles, however I'm not sure what the implications are. Is this "safe"? I guess it is as long as I don't try to associate the channel with another pin?

HAL crate

Would it be possible to support this use case in the HAL library to avoid the need to use unsafe?

• Maybe this could be achieved by supporting changing the frequency from a Pwm instance? Although it would probably be very strange to have one Pwm instance change the frequency for all the other channels remotely. 🤔
• The other solution would involve some major changes because it would require the timer not to own the channels. I'm not sure what that involves.

What are your thoughts about this?

[hannobraun]

Hey @azerupi, thank you for bringing this up!

2. Make a raw pointer to the timer object to bypass Rust's ownership

   let pwm_timer_ptr: *mut Timer<TIM2> = &mut pwm_timer;
   // Some code
   unsafe { 
       (*pwm_timer_ptr).set_frequency(frequency.hz(), &context.rcc); 
   }

   This also compiles, however I'm not sure what the implications are. Is this "safe"? I guess it is as long as I don't try to associate the channel with another pin?

I think this is sound, as far as Rust's unsafe rules are concerned (although I'm not completely sure). Should be fine as a workaround.

Would it be possible to support this use case in the HAL library to avoid the need to use unsafe?

Definitely! The only hurdles are coming up with an acceptable design, and someone doing the implementation work.

Maybe this could be achieved by supporting changing the frequency from a Pwm instance? Although it would probably be very strange to have one Pwm instance change the frequency for all the other channels remotely.

I share your concern, but I think this would be fine (although far from ideal), as long as this is properly documented. It would be great if someone came up with a better design, but this seems like a low-effort change that would allow you to get rid of your unsafe workaround, and I'm all for that.

The other solution would involve some major changes because it would require the timer not to own the channels. I'm not sure what that involves.

I don't know either. I'm sure an elegant design is hiding in there somewhere, but that will probably take some effort and experimentation to find. I'm happy to provide feedback if anyone comes up with something, but for now, I think think adding a set_frequency method to Pwm (with a big fat warning in the documentation) is better than nothing.

[azerupi]

Thanks for your reply!

for now, I think think adding a set_frequency method to Pwm (with a big fat warning in the documentation) is better than nothing.

In that case I might give it a try and send a pull-request for that when I have some time 🙂

[hannobraun]

Thank you, @azerupi, that would be great!