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

Refactored setting prescale, add setPrescale #65

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

Bolukan
Copy link
Contributor

@Bolukan Bolukan commented Oct 29, 2019

Scope
Refactored the code for setting prescale which was in the code twice and moved it to a void setPrescale(uint8_t prescale, bool extclk=false) function.

Known limitations
A private setPrescale function could be preferred.

I refrained from changing the definition of existing functions, but considered

  • The function begin(prescale) enables the external oscillator if prescale > 0. To my opinion this function should use a boolean. Also could be considered to move it to a separate void setExternalOscillator(void) function.
  • The used oscillator frequency could be stored in a variable. So the frequency can be set once and used throughout the code, for example if an external oscillator different from 25MHz is used. Also people could choose to use 25MHz, 26MHz or what they seem reasonable for the real frequency of their device.
  • I combined setting prescale and extclk as it needs the same steps.

Tests and examples
In pwmtest.ino an example of setPrescale is added.

@photodude
Copy link
Contributor

photodude commented Oct 29, 2019

@Bolukan could you back out the change to writeMicroseconds() from your PR? I'm submitting a PR that simplifies the calculations for the writeMicroseconds() pulse and I'm using the frequency constant. Thank you in advance.

See #66 for the writeMicroseconds() calculation improvement

@photodude
Copy link
Contributor

Thank you @Bolukan.

@ladyada
Copy link
Member

ladyada commented Nov 3, 2019

i believe this was hand-added in last push - please check?

@Bolukan
Copy link
Contributor Author

Bolukan commented Nov 3, 2019

I removed a change (25.000.000 to a constant) back on his request in the last commit.

@ladyada
Copy link
Member

ladyada commented Nov 3, 2019

ok i did a bit of work on it in last push, please check and adjust PR as necessary - thank you :)

@Bolukan
Copy link
Contributor Author

Bolukan commented Nov 3, 2019

I dont know if I used the right route ('Comply to current master branche', 'Merge pull request #1 from adafruit/master' and 'use setPrescale function') and it took some time to get compliant to clang-format. But work is done ...

examples/pwmtest/pwmtest.ino Outdated Show resolved Hide resolved
@ladyada
Copy link
Member

ladyada commented Nov 3, 2019

nice work!

@photodude
Copy link
Contributor

@Bolukan As you are adding a new function, would you also update the keywords.txt file with that function?
Here is an explanation of keywords for Arduino IDE’s syntax highlighter
https://spencer.bliven.us/index.php/2012/01/18/arduino-ide-keywords/

Thank you in advance.

@Bolukan
Copy link
Contributor Author

Bolukan commented Nov 4, 2019

Done commenting out line (in example) and update keywords with new functions in current version

@photodude
Copy link
Contributor

@ladyada @Bolukan looks like everything is good to go for considering this. Is there something left to be reviewed?

@ladyada
Copy link
Member

ladyada commented Jan 18, 2020

it needs to be hardware-reviewed since its a code not documentaiton change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants