[RSDK-10192] [RSDK-10199] Update PWM logic for ESP-IDF 5 #440
Conversation
micro-rdk/src/esp32/pwm.rs
Outdated
| impl LedcTimerWrapper<'_> { | ||
| fn new(id: u8, frequency_hz: u32) -> Result<Self, Esp32PwmError> { | ||
| let timer_config = TimerConfig::default().frequency(frequency_hz.Hz()); | ||
| let timer = OnceCell::new(); | ||
| let _ = timer.set(create_timer_driver(id, &timer_config)?); | ||
| let _ = timer.set(LedcTimerOption::new(timer, &timer_config)?); |
There was a problem hiding this comment.
you change the signature of the function it doesn't take a oncecell anymore
There was a problem hiding this comment.
I don't think I did? Which function do you mean?
There was a problem hiding this comment.
LedcTimerOption::new takes a u8 and not a OnceCell
There was a problem hiding this comment.
Sorry I'm still confused, create_timer_driver didn't take a OnceCell either, what function are we talking about again?
There was a problem hiding this comment.
LedcTimerOption::new(timer: u8, conf: &TimerConfig) takes an u8 but you pass timer as first arg. I think you meant id :P
|
|
||
| impl LedcTimerOption<'_> { | ||
| pub fn new(timer: u8, conf: &TimerConfig) -> Result<Self, Esp32PwmError> { | ||
| match timer { |
There was a problem hiding this comment.
need to cover the default case
There was a problem hiding this comment.
ah, you're right, I missed that. My bad
micro-rdk/src/esp32/pwm.rs
Outdated
|
|
||
| pub fn timer(&self) -> ledc_timer_t { | ||
| match self { | ||
| Timer0(_) => TIMER0::timer(), |
There was a problem hiding this comment.
where are these symbols coming from
There was a problem hiding this comment.
imported above from esp_idf_svc::hal::ledc
There was a problem hiding this comment.
I don't see Timer0 is it defined elsewhere?
There was a problem hiding this comment.
oh i see then the syntax should be Self::Timer0
There was a problem hiding this comment.
yep, this PR's a real mess huh?
| @@ -109,8 +109,7 @@ impl<'a> PwmDriver<'a> { | |||
|
|
|||
| pub fn get_timer_frequency(&self) -> u32 { | |||
| let timer: ledc_timer_t = (self.timer_number as u8).into(); | |||
| // TODO(RSDK-10199): SpeedMode is now a trait in ESP-IDF 5 | |||
| unsafe { ledc_get_freq(SpeedMode::LowSpeed.into(), timer) } | |||
| unsafe { ledc_get_freq(LowSpeed::SPEED_MODE, timer) } | |||
There was a problem hiding this comment.
i don't think LowSpeed is an enum anymore
There was a problem hiding this comment.
It's not, it's a struct implementing the SpeedMode trait, but the trait contains a variable called SPEED_MODE
There was a problem hiding this comment.
can trait variables be private? I didn't think they could
EDIT: I've confirmed that public/private visibility does not exist for things inside traits (although you can make the trait itself private by not exporting it)
@npmenard, thoughts on what kind of direction you'd prefer? |
| @@ -166,6 +165,50 @@ impl From<PwmChannel> for usize { | |||
| } | |||
| } | |||
|
|
|||
| pub enum LedcTimerOption<'a> { | |||
| Timer0(LedcTimerDriver<'a, TIMER0>), | |||
There was a problem hiding this comment.
There is a couple of place where LedcTimerDriver is returned or used would need to add the new generic param
@gvaradarajan will test on hardware when more of the platform modernization tickets are satisfied.