Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement variable PWM frequency #756

Merged
merged 1 commit into from Jan 14, 2016

Conversation

@monkbroc
Copy link
Member

commented Dec 4, 2015

  • Supports 1 Hz to 65535 Hz
  • User facing API: analogWrite(pin, value, pwm_frequency)
  • Calculate an appropriate prescaler for the range of frequencies
  • Allow coherent updates of duty cycle and frequency on the fly by using the timer update disable feature
  • Add tests for PWM with frequency

Limitations

  • All PWM pins on the same timer must use the same frequency.
  • Coherent update of frequency on multiple channels on the same timer is not guaranteed. Duty cycle only updates are OK.

Other changes

  • Remove extern "C" from the analogWrite, digitalWrite to allow overloaded versions
  • Share pwm_hal for Core, Photon and Electron

Bugfixes

I tested this change on a Photon, Core and Electron for all the PWM
capable pins. I verified a range of frequencies and duty cycles from 1 Hz up to 60 kHz with a multimeter. It was always spot on.

Closes #213

@andyw-lala

This comment has been minimized.

Copy link
Contributor

commented Dec 4, 2015

Awesome work.

I hope the cheque is in the post.

@bkobkobko

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2015

This is awesome and will be well loved in the community!

I do have a question: doesn't removing the extern "C" have backward compatibility implications? I bet @m-mcgowan knows for sure! I guess since you are adding an argument the case can be made that it is inherently not backward compatible, but in the two arg case, it would nice if you were not forced to upgrade.

@monkbroc

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2015

Thanks for the kind words Andy and Brian.

I don't think there are any compatibility issues for the extern C. If you recompile and you used the 2 argument version nothing will change and your code will still work.

The only change is that the function name analogWrite will be mangled with C++ rules.

pin_t pin = D0; // pin under test
// when
pinMode(pin, OUTPUT);

analogWrite(pin, 25); // 10% Duty Cycle at 500Hz = 200us HIGH, 1800us LOW.

This comment has been minimized.

Copy link
@technobly

technobly Dec 5, 2015

Member

I believe this was originally in the for() loop to help vet issues with re-initializing the PWM. Having it in the for() loop should not cause a problem, and at least one of these tests should keep it in the for() loop :)

This comment has been minimized.

Copy link
@monkbroc

monkbroc Dec 5, 2015

Author Member

OK. BTW, I gave you push access to my fork if you want to make other tweaks.

@technobly

This comment has been minimized.

Copy link
Member

commented Dec 5, 2015

@monkbroc really great to finally see this implemented! Thank you for the extensive testing!! I don't even mind that you replaced my name with yours in the header 😁

@bkobkobko

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2015

Hi @monkbroc Julien

I am pretty familiar with name swizzling, so that was not what I was getting at in terms of backward compatibility. @m-mcgowan wanted to have as a goal that you could load user code against system parts 1 & 2 even with mismatched versions as long as you were not using something new in the HAL layer. This is a really nice goal since it prevents the case where a user uses the web IDE to load new code to their device but it does not run and they enter safe mode instead.

It is a goal that cannot be reached in all cases since the HAL layer gets extended from time to time, but it is very laudable goal. What I want to know is does this change in compiler linkage and name swizzling prevent new code from running on older system parts?

@monkbroc

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2015

I don't even mind that you replaced my name with yours in the header

To be fair @technobly, Satish had "omitted" your name in the Photon pwm_hal.c. 😉 I simply added my name after his.

I understand what you mean now @bkobkobko. I still think it's ok.

The main way to maintain compatibility between system and user parts is through the application binary interface (ABI) in the dynalib. The original HAL_PWM_Write still takes 2 arguments. I added a new HAL_PWM_Write_With_Frequency. So a recompiled user part using analogWrite with 2 arguments will still call HAL_PWM_Write whether it's using the old or new system parts. The name mangling of analogWrite will not impact this.

@ScruffR

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2015

Wow, what a great contribution 👍
An eagerly anticipated feature well implemented 😃


To be fair @technobly, Satish had "omitted" your name in the Photon pwm_hal.c. 😉 I simply added my name after his.

The diff seems to suggest different

-/**        
- ******************************************************************************        
- * @file    pwm_hal.c      
- * @authors Satish Nair, Brett Walach      
- * @version V1.0.0     
- * @date    12-Sept-2014       
- * @brief      
- ******************************************************************************
@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2016

Really nice work. 👍 Thanks for adding automated tests, and for factoring it into a shared stm32 file.

The change of extern "C" should not be a compatibility concern. But...we have in the past seen some folks add their own prototypes of wiring functions. We had this happen with millis() - some libraries were declaring extern "C" uint32_t millis(); which then caused a compiler error when we changed linkage to C++.

However, we're lucky this time - a grep of the library sources doesn't reveal that anyone has done this for analogWrite.

@monkbroc

This comment has been minimized.

Copy link
Member Author

commented Jan 11, 2016

Thanks for explaining the rationale @m-mcgowan. So any bare word Wiring function (no namespace or class) that may be used in external libraries that expect C linkage should be wrapped in extern "C". I put extern "C" back in spark_wiring.h

monkbroc added a commit to monkbroc/particle-docs that referenced this pull request Jan 12, 2016
monkbroc added a commit to monkbroc/particle-docs that referenced this pull request Jan 12, 2016

@monkbroc monkbroc force-pushed the monkbroc:feature/pwm-frequency branch from b7af340 to c1c49e1 Jan 12, 2016

@monkbroc

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2016

Scratch that last comment. If we want 2 overloaded signatures for analogWrite, one with and without frequency, then spark_wiring.h can't have extern "C"

@m-mcgowan

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2016

Understood. I'm motioning that we drop extern C for the analogWrite functions. It doesn't look like it breaks compatibility with any libraries having the overload and C++ linkage.

@ScruffR

This comment has been minimized.

Copy link
Contributor

commented Jan 12, 2016

I know this is a silly question, but why two overloads and not one with a default value for frequency?

@monkbroc

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2016

2 reasons: 1) C doesn't allow default arguments so having an extern "C" function with default arguments is weird. 2) the 3 argument version with frequency ignores DAC pins since it makes no sense to use a DAC output with frequency.

@monkbroc

This comment has been minimized.

Copy link
Member Author

commented Jan 12, 2016

I'm motioning that we drop extern C for the analogWrite functions.

Done. I also rebased to fix the merge conflict.

Implement variable PWM frequency
* Supports 1 Hz to 65535 Hz
* User facing API: analogWrite(pin, value, pwm_frequency)
* Calculate an approriate prescaler for the range of frequencies
* Allow updating frequency on the fly
* Add tests for PWM with frequency

Other changes

* Remove extern "C" from analogWrite to allow overloaded versions
* Share pwm_hal for Core, Photon and Electron

Bugfixes

* Fix duty cycle initialization making a second Duty Cycle initialization unecessary

I tested this change on a Photon, Core and Electron for all the PWM capable pins. I verified a range of frequencies and duty cycles from 1 Hz up to 60 kHz with a multimeter. It was always spot on.

@monkbroc monkbroc force-pushed the monkbroc:feature/pwm-frequency branch from da9bd09 to 00b02c8 Jan 12, 2016

@m-mcgowan m-mcgowan added this to the 0.4.9 milestone Jan 14, 2016

m-mcgowan added a commit that referenced this pull request Jan 14, 2016
Merge pull request #756 from monkbroc/feature/pwm-frequency
Implement variable PWM frequency

@m-mcgowan m-mcgowan merged commit 4a431a6 into particle-iot:develop Jan 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@technobly technobly deleted the monkbroc:feature/pwm-frequency branch Oct 27, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.