Awesome indeed! On Fri, 09 Mar 2018 10:32:49 -0800 "Sterling Hughes" <sterling.hughes.pub...@gmail.com> wrote:
> Awesome, thanks @mlaz. > > On 8 Mar 2018, at 21:00, Miguel Azevedo wrote: > > > 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
