Hi Markus, and everyone else, My proposal for changes on our PWM API is on this PR (most recent commit): https://github.com/apache/mynewt-core/pull/836
I think it takes care of the oversized unorganized datastructure problem and allows a clean use of basic PWM capabilities (i.e. looping a duty cycle / playing a dyuty cycle for n cycles) as well as for interrupts. Again, I believe all of these features are implementable on most HW based PWM implementations, however this API interface still allows us to use a simple PWM device without any interrupts. I also added the pwm_get_top_value and updated doc, as requested. > Personally I probably would have combined the ISRs into a single callback > function and use an additional enumeration parameter to determine the > "trigger". This would allow future extensions, say if a counter supports more > than one compare value. I see, although this would make us have to use an enumeration for the event type(or "trigger") plus a mechanism to include user data according to it, I don't know what kind of other feature you want to see on PWM but I suppose it isn't that standard and will probably be implementable using pwm_chan_cfg.data. > As for the nrf52 suggestion, what I meant was that the new api is a super-set > of the old api. So once you implement a driver with the new api you can > easily implement a driver with the old api by creating an interface layer > that just sets all extra pointers and values to dummy values. Having to maintain 2 APIs for the same driver doesn't make much sense to me, why not a single scalable API? We can even have some macros to optimize the internal datastructures' size(regarding the interrupt related fields) like PWM_CYCLE_INT and PWM_SEQ_END_INT. Take a look at pwm_test I got on that branch I PRed, it uses this API with both a fully featured driver and a simpler one. Best Regards, Miguel On Thu, Mar 1, 2018 at 6:42 AM, markus <mar...@bibi.ca> wrote: > wow - it's an email day for me .... > > I like the idea of having a single pointer for the additional values, or > changing `data` to a pointer to said structure instead of a `void*` (and move > the void* into that structure). > > Personally I probably would have combined the ISRs into a single callback > function and use an additional enumeration parameter to determine the > "trigger". This would allow future extensions, say if a counter supports more > than one compare value. > > > As for the nrf52 suggestion, what I meant was that the new api is a super-set > of the old api. So once you implement a driver with the new api you can > easily implement a driver with the old api by creating an interface layer > that just sets all extra pointers and values to dummy values. > > Have fun, > Markus > > > PS: about all the many emails - I got the STM32 port to work yesterday so > before going further I wanted to clarify the interface, and then you sent out > this email - and I didn't want to mix topics, hijack your thread ..... So if > anybody asks, I'll blame you - just so you know ;) > > > On Thu, 1 Mar 2018 01:54:53 -0300 > Miguel Azevedo <miguella...@gmail.com> wrote: > >> Hi again Markus, >> >> > I think your enhancements should be their own driver. Plain PWM is >> > such a basic need and feature that I think the interface for it >> > should be as simple as possible. I'm also concerned about the >> > memory footprint increase, which is 3x that of the original >> > interface (and that's just the data structure). >> >> Agreed. Although if you re-use the structure it isn't such a dramatic >> increase in memory footprint. >> Regarding the datastructure size I believe we can keep the previous >> interface, without the callback associated fields and the cycle(I >> don't love this one either) I suggest we have a specific datastructure >> defined on pwm_nrf52.h or even pwm.h(given these are common features >> on PWM devices, any driver would be eligible to implement them on a >> standardized way). This struct could either be a struct containing a >> pwm_chan_cfg which we would 'upcast' and 'downcast' (like we do with >> dev and odev) or simply something we pass through the data field. >> >> > For the nrf52 specifically you could implement the old interface >> > driver on top of a driver with the new proposed interface. >> I don't understand what you mean by using the old interface but I >> think one of the the ways I described will sort out the pwm_chan_cfg >> memory footprint problem. >> >> Thanks for your feedback! :) >> >> Miguel >> >> On Wed, Feb 28, 2018 at 9:24 PM, markus <mar...@bibi.ca> wrote: >> > Hi Miguel, >> > >> > I think your enhancements should be their own driver. Plain PWM is >> > such a basic need and feature that I think the interface for it >> > should be as simple as possible. I'm also concerned about the >> > memory footprint increase, which is 3x that of the original >> > interface (and that's just the data structure). >> > >> > For the nrf52 specifically you could implement the old interface >> > driver on top of a driver with the new proposed interface. >> > >> > My 2 cents, >> > Markus >> > >> > >> > >> > On Wed, 28 Feb 2018 17:51:50 -0300 >> > Miguel Azevedo <miguella...@gmail.com> wrote: >> > >> >> Hi everyone, >> >> >> >> I recently implemented a few new features for the nRF52 PWM driver >> >> and will probably implement them for soft PWM as well. >> >> >> >> The features: >> >> * Interrupts fired every cycle with user handler and parameter >> >> data; >> >> * A mode where the device plays for n cycles; >> >> * Interrupts fired at the end of a sequence of n cycles with user >> >> handler and parameter data. >> >> >> >> These changes are PRed here: >> >> https://github.com/apache/mynewt-core/pull/836 >> >> >> >> There is an example (pwm_test) which uses the also PRed easing >> >> library: https://github.com/apache/mynewt-core/pull/822 >> >> >> >> These features change the pwm_chan_cfg datastructure on pwm.h, >> >> wich is general to all drivers. >> >> I can argue that these features are implementable on virtually any >> >> PWM device since every hardware PWM implementation I've seen so >> >> far uses timers and counters, however there might be (and probably >> >> are) exceptions. >> >> Anyway a driver which doesn't use these new fields on this >> >> datastructure is still implementable(well, we have soft PWM using >> >> the sabe API interface and it works). >> >> >> >> The alternative to this would be to write a datastructure on the >> >> driver's implementation header file (pwm_nrf52.h in this case) with >> >> the new fields. >> >> >> >> What do you think? >> >> >> >> Best, >> >> >> >> Miguel Azevedo >> > >> >> >> > -- -- Miguel Azevedo
