[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-389130405
 
 
   Solved on https://github.com/apache/mynewt-core/pull/1016


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-388184617
 
 
   Ok, you are right. I will change it. (I was sure the GPIO HAL was uint8_t)


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-388184617
 
 
   Ok, you are right. I will change it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-388098615
 
 
   > But what you want here is not an IO pin, definitely not a GPIO pin, you 
want a PWM pin. I'm not sure the comparison to GPIO is applicable here, this is 
new ground. 
   
   This statement's validity is highly hardware dependent, however I still 
doubt you'll have more than 255 'PWM pins'. By the way, I was not comparing 
anything, nrf52 PWM and soft PWM use GPIO pins.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-388091863
 
 
   @mlampert, so far we are doing soft PWM and hw PWM using GPIO pins, because 
these are the only pins we got an abstraction which represents them (besides 
the fact that soft PWM necessairily uses GPIO and most, if not all, pins on 
nRF52xx are GPIO). However you are still asking me to have some out of the 
pattern type here, with _no benefit_ whatsoever. Please implement this new pin 
abstraction, ping me and I'll be happy to then refactor this code.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-388091863
 
 
   @mlampert, so far we are doing soft PWM and hw PWM using GPIO pins, because 
these are the only pins we got an abstraction which represents them (besides 
the fact that soft PWM necessairily uses GPIO and most, if not all, pins on 
nRF52xx are GPIO). However you are still asking me to have some out of the 
pattern type here, with _no benefit_ whatsoever. Please implement these new pin 
abstraction, ping me and I'll be happy to then refactor this code.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-388091863
 
 
   @mlampert, so far we are doing soft PWM and hw PWM using GPIO pins, because 
these are the only pins we got an abstraction which represents them (besides 
the fact that soft PWM uses necessairily GPIO and most, if not all, pins on 
nRF52xx are GPIO). However you are still asking me to have some out of the 
pattern type here, with _no benefit_ whatsoever. Please implement these new pin 
abstraction, ping me and I'll be happy to then refactor this code.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-387910285
 
 
   Note that on nordic devboards (namely nrf52840pdk) there are also multiple 
ports containing multiple pins, we are still dealing with this by using some 
addressing scheme like the one you mentioned(actualy this is the standard way 
tha manufacturer handles this on its SDK), also GPIO pins are usually either 
input or output configurable.
   
   >A while back we had a discussion on the mailing list about introducing a 
pin type. And adding more uint8_t interfaces increases the inertia to move into 
that direction.
   
   I remember that, it looks a lot like a problem mostly(if not only) happening 
with stm chips. I think if you really want to move on to new type for the GPIO 
pins you probably should first implement it and push it (which will probably 
make you refactor a bunch of code in the process). It doesn't make much sense 
for me now to implement a new pin type out of the blue for a use case I am not 
even fully aware of. 
   I don't think changing the pin type for uint32_t on the PWM API while having 
GPIO HAL API pins as uint8_t is very consistent. If you want to move in that 
direction that is a whole different task there, it won't even help you having a 
bunch of drivers with(ugly dirty) casts which later on you'll have to refactor 
again. 
   Anyway I am not opposing to anything here, I just don't see a real advantage 
on having this type changed only on the PWM API, mostly because you'll probably 
change the GPIO pin type in the future(since that's your objective) which will 
bring us to an inevitable refactor no matter what. I don't see how it increases 
inertia, I just feel that since software is always being rewritten we should 
fight for it to be consistently well written, not like an unfinished WIP.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-387705724
 
 
   >Any chance I can persuade you to make pwm_chan_cfg::pin a uint32_t?
   
   Ok, give me a use case where you'd need it. It is uint8_t because gpio hal
   uses this data type, which makes sense to me since I doubt you'll have more
   than 255 gpio pins on an MCU, besides the consistency between APIs we want 
to keep.
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-387705724
 
 
   >Any chance I can persuade you to make pwm_chan_cfg::pin a uint32_t?
   
   Ok, give me a use case where you'd need it. It is uint8_t because gpio hal
   uses this data type, which makes sense to me since I doubt you'll have more
   than 255 gpio pins on an MCU, besides the coherence between APIs.
   
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-387705724
 
 
   Ok, give me a use case where you'd need it. It is uint8_t because gpio hal
   uses this data type, which makes sense to me since I doubt you'll have more
   than 255 gpio pins on an MCU, besides the coherence between APIs.
   
   On 9 May 2018 06:03, "mlampert"  wrote:
   
   Thanks for the clarifications.
   
   Any chance I can persuade you to make pwm_chan_cfg::pin a uint32_t?
   
   —
   You are receiving this because you were assigned.
   
   Reply to this email directly, view it on GitHub
   ,
   or mute the thread
   

   .
   


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-387586215
 
 
   > The cycle and sequence have moved from the channel to the device. This 
simplifies things quite a bit and makes a lot of sense for the cycle. I am 
wondering though if it would make sense to keep the sequence on the channel. I 
don't have a real world use case either way, but I could imagine that I want to 
keep one channel going (forever) while running sequences on other channels of 
the same device. This is more an ask for "inspiration" ;)
   
   I believe this is the most natural way to implement this feature, typically 
pwm devices are clock dividers, which may trigger these interrupts. A device 
has one or more channels, so these channels are all affected by such 
interrupts. A real world case is for example RGB LED's with the nRF52 PWM 
driver on which we may associate a single(3 of its channels) device to each 
LED. If you want a channel going for ever you should use another PWM device, 
other than the one you are using with the n_cycles configured, this is how I 
got RGB LED's doing different things at the seme time.
   
   > pwm_configure_device (is this required? if not used, can it be skipped?)
   
   It is not required as you may see on the pwm_test app's code. We should 
always have a default configuration for the driver on which these features 
aren't in use.
   
   > when client call "pwm_disable", should the pwm complete the current cycle 
or immediately terminate the pwm output? Or is that left to the implementation?
   
   I think it makes sense for it to finish the cycle, although this might be a 
bit too hardware specific. We can force it to finish the cycle on soft PWM but 
on an hardware based solution it will be up to the driver's implementation.
   
   > calling pwm_configure_device on a device that has already been configured 
previously maintains the existing channel configurations?
   
   It should.
   
   > is pwm_get_resolution_bits still needed now that we have pwm_get_top_value 
?
   
   I'm not sure but it isn't hurting anyone, is it?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-387499899
 
 
   Hi Markus, the API and both drivers' refactors are ready for PR, I am 
changing this on most BSP's right now.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-381991519
 
 
   Ok, I agree with the most that have been said here but we need to take a 
look at the nature of the problem and how PWM is implemented. Typically there's 
a HW device which is active or inactive, it uses different channels which might 
have a duty cycle. I think that what we're failing at here is having something 
called enable_duty_cycle instead of set_duty_cycle. This is because we are in 
fact enabling and disabling a device(as long as we're enabling at least a 
single channel or disabling all of them). So IMO, we should have both device 
configure/enable/disable functions and channel configure/unconfigure/set_duty. 
Note that we are setting the duty cycle to 0 whenever disabling a channel, 
which doesn't make that much sense. I would like to see a flow for a basic set 
up like: pwm_set_frequency, pwm_configure_channel, pwm_set_duty, pwm_enable. 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-381148189
 
 
   Ok, I see 2 issues here, probably we want :
   1 - To make pwm_disable to just stop the channel playback and not 
unconfigure it. Sincelosing the device will make every channel unconfigured.
   2 - pwm_is_enabled because we now can make a channel run for a number of 
cycles and may want to know if it has already been configured. (it will remain 
configured after playing for n cycles)
   3 - A way to make a channel unassigned to a pin. (I'll make a PWM_NO_PIN 
macro so the user may do this via configure_channel) 
   
   @mlampert do you have any input on this?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-381148189
 
 
   Ok, I see 2 issues here, probably we want :
   1 - To make pwm_disable to just stop the channel playback and not 
unconfigure it. Sincelosing the device will make every channel unconfigured.
   2 - pwm_is_enabled because we now can make a channel run for a number of 
cycles and may want to know if it has already been configured.
   3 - A way to make a channel unassigned to a pin. (I'll make a PWM_NO_PIN 
macro so the user may do this via configure_channel) 
   
   @mlampert do you have any input on this?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-381148189
 
 
   Ok, I see 2 issues here, probably we want :
   1 - To make pwm_disable to just stop the channel playback and not 
unconfigure it.
   2 - Closing the device will make every channel unconfigured.
   3 - pwm_is_enabled because we now can make a channel run for a number of 
cycles and may want to know if it has already been configured.
   4 - A way to make a channel unassigned to a pin. (I'll make a PWM_NO_PIN 
macro so the user may do this via configure_channel) 
   
   @mlampert do you have any input on this?


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-381148189
 
 
   Ok, I see 2 issues here, probably we want :
   1 - pwm_disable to just make the channel stop and not unconfigure it.
   2 - Closing the device will make every channel unconfigured.
   3 - pwm_is_enabled because we now can make a channel run for a number of 
cycles and may want to know if it has already been configured.
   4 - A way to make a channel unassigned to a pin. (I'll make a PWM_NO_PIN 
macro so the user may do this via configure_channel) 


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-376981578
 
 
   > An API for testing if the pwm is currently enabled would be useful.
   
   I'm PRing this soon along with a README for pwm_test.
   
   > The same goes for configured state, as pwm_disable unconfigure the channel
   
   @sdalu What you mean by this? PWM implementations usually get some default 
values when you don't configure them. If you don't assign a pin to a channel it 
doesn't make much sense to do anything with it. I don't see a case when someone 
needs to query whether a channel has been configured.
   
   > Perhaps a diagram showing what function can be called in what order would 
be a nice addition, as well as explicitly saying in the documentation when it 
its legit to call the function
   
   A README file for apps/pwm_test is in the making but I think it wouldn't be 
hard to agree that you need to open a device, then configure a channel, then 
set a duty cycle on it (i.e. enable). So, I'll make sure the PWM API 
documentation is updated with the requirements for calling each function (i.e. 
calling pwm_enable_duty_cycle requiring the channel to be already configured) 
and that this somehow is mentioned on the README.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services

[GitHub] mlaz commented on issue #955: pwm enabled?

[GitBox]

mlaz commented on issue #955: pwm enabled?
URL: https://github.com/apache/mynewt-core/issues/955#issuecomment-375739181
 
 
   This seems reasonable to me. On it.


This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org


With regards,
Apache Git Services