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
