Refactored setting prescale, add setPrescale

adafruit / Adafruit-PWM-Servo-Driver-Library

Adafruit PWM Servo Driver Library

Other

477 stars 311 forks source link

Refactored setting prescale, add setPrescale #65

Open Bolukan opened 5 years ago

Bolukan commented 5 years ago

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 commented 5 years ago

@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 commented 5 years ago

Thank you @Bolukan.

ladyada commented 5 years ago

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

Bolukan commented 5 years ago

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

ladyada commented 5 years ago

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

Bolukan commented 5 years ago

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 ...

ladyada commented 5 years ago

nice work!

photodude commented 5 years ago

@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 commented 5 years ago

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

photodude commented 4 years ago

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

ladyada commented 4 years ago

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