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 > > > > >
