Thanks Will, Sterling was a fan of that style API as eel (one that takes freq and duty cycle). Sterlings solution to the error case was to add a frequency tolerance so the code could do its best to accommodate and know when its not close enough.
My thought was this. The hal goal is to allow abstraction of the HW and provide a common library that apps and libs can use. With that in mind, it seems the lowest level abstraction of the HW is best. Folks can always go add libraries on top of this and we want to encourage that. The lowest level API for PWM would probably set the repeat and capture registers (period and on). I added an API for set_duty for things like LED etc., where its really just about simplicity. I¹m not opposed to adding another API for set freq_duty that takes the freq (in Hz?) and the duty cycle) or replacing the set_duty API and creating a FREQ_DONT_CARE macro. It still seems to me like its harder to do error handling in an app (expecting an error in every call to set_freq_duty) rather than learn capabilities (query resolution and clock_freq). Probably a important piece of this is whether the HAL should automatically change the clock source frequency and PWM clock dividers to try to achieve the requested frequency as accurately as possible. I decided to keep it simple for now. Paul On 3/30/16, 6:13 PM, "will sanfilippo" <[email protected]> wrote: >I am not a huge fan of the API using clock ticks as opposed to time. I >wonder if the API should just be frequency and duty cycle. If the >underlying HW cant support it it can return an error. I have not thought >this through completely so I am sure there is some reason this is not >good :-) > >I guess one possible issue is the issue where the HW cant support the >exact frequency. For example, I ask for 1kHZ but can only get 1.1 or .9. > >Anyway, I dont have an alternate proposal so maybe I shouldnt comment :) > >Will > >> On Mar 30, 2016, at 9:56 AM, [email protected] wrote: >> >> >> >> A quick discussion thread on the PWM API. I currently proposed a PWM >>API with five simple methods in my PWM git pull request. >> >> hal_pwm_set_period(uin32_t usec); >> hal_pwm_set_on_duration(uint32_t usec); >> hal_pwm_on(); >> hal_pwm_off(); >> hal_pwm_create(); >> >> The on/off/create APIs are fine, and I don't intend to change them. >> >> But the setting APIs assume a lot about the underlying PWM controller >>setup. Mainly that it has lots of resolution (clock rate) and lots of >>precision (bits of PWM register). The goal of this simple API was to >>abstract the HW enough to make it simple to use, but I think I've got >>overboard and made it unusable. So I want to propose a new HAL API like >>this. >> >> /* returns the count frequency for the underlying PWM. This is set by >>the BSP and HW limitations */ >> uint32_t hal_pwm_get_clock_freq_hz(void) >> >> /* returns the register size of the counter regiser for the underlying >>PWM. This is set the by BSP and HW limitations */ >> uint32_t hal_pwm_get_resolution_bits(void); >> >> /* sets the period of the PWM waveform in clock (clock frequency >>returned above). Can't exceed the resolution above (2^N) or error */ >> Int hal_pwm_set_period(uin32_t clocks); >> >> /* sets the on duration of the PWM waveform in clocks (clock frequency >>returned above). Can't exceed the resolution above or error */ >> Int hal_pwm_set_on_duration(uint32_t clock); >> >> /* sets the duty cycle of the PWM. 0=always low, 255 = always high. >> * Sets the period to the smallest possible to achieve this exact duty >>cycle. Overwrites any >> * changes made by hal_pwm_set_period and hal_pwm_set_on_duration. This >>is designed to be the simple API that folks >> Would use if they just want a duty cycle for controlling LED brightness >>etc */ >> Int hal_pwm_set_duty_cycle(uint8_t frac); >> >> Comments? This API is a bit more complicated but lets the application >>know a lot more about the underlying functionality of the PWM provided >>by the BSP. >> >> Paul >> >> >
