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.

Sign up for GitHub

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

Jump to bottom

Correct math on fade time #55

Merged
merged 1 commit into from
Jul 29, 2024
Merged

Correct math on fade time #55

merged 1 commit into from
Jul 29, 2024

Conversation

grandinquisitor
Copy link
Contributor

  • per the datasheet, the pause time is specified as a multiple of 3.5, not 26
  • specified in docstring the valid ranges of fade & pause times accordingly

@grandinquisitor
Correct math on fade time
eaddff7 
* per the datasheet, the pause time is specified as a multiple of 3.5,
  not 26
* specified in docstring the valid ranges of fade & pause times
  accordingly
FoamyGuy
FoamyGuy reviewed Jul 20, 2024
View reviewed changes
adafruit_is31fl3731/__init__.py
:param fade-out: int positive number; 0->100
:param pause: int breath register 2 pause value
:param fade_in: fade time in ms, range = 26 to 3328
:param fade-out: fade time in ms, range = 26 to 3328
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there was an intention originally to make the API use "cleaner" numbers 0-100 rather than the literal ms values of 26-3328, though in the way it's implemented now your fix to the docs does appear to reflect the correct values that the implementation is expecting.

Honestly though I think it would be better to change the actual functionality to just accept the values 0-7 and the docstrings can list out how many ms each of them correspond to.

By making the API accept 26 to 3328 it makes it seem to the developer using it that they are free to choose any value within that range and that the duration would be set to the time they chose. But in reality it seems the actual fade duration is always going to be one of these values if I am understanding the datasheet properly:

26
52
104
208
416
832
1664
3328

I think it would be more clear to the developer using the API if we only allow 8 possible different values if that is technically how many different possible values are supported by the chip.

FoamyGuy
FoamyGuy approved these changes Jul 29, 2024
View reviewed changes
Copy link
Contributor

@FoamyGuy FoamyGuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me.

I tested it successfully on a 15 x 7 Charlie Wing and Feather RP2040, with a modified version of the simpletest to use fade() with a delay, and autoplay to loop.

@FoamyGuy FoamyGuy merged commit 48427e0 into adafruit:main Jul 29, 2024
1 check passed
adafruit-adabot added a commit to adafruit/Adafruit_CircuitPython_Bundle that referenced this pull request Jul 30, 2024
@adafruit-adabot
Automated update by Adabot (adafruit/adabot@d723e94)
f631081 
Updating https://github.com/adafruit/Adafruit_CircuitPython_BME280 to 2.6.25 from 2.6.24:
  > Merge pull request adafruit/Adafruit_CircuitPython_BME280#69 from rwmanos/patch-1

Updating https://github.com/adafruit/Adafruit_CircuitPython_BME680 to 3.7.8 from 3.7.6:
  > Merge pull request adafruit/Adafruit_CircuitPython_BME680#73 from FoamyGuy/precommit_copyright
  > Merge pull request adafruit/Adafruit_CircuitPython_BME680#70 from justmobilize/ruff-updates

Updating https://github.com/adafruit/Adafruit_CircuitPython_DisplayIO_SH1106 to 1.4.0 from 1.3.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_DisplayIO_SH1106#19 from EAGrahamJr/issue-16

Updating https://github.com/adafruit/Adafruit_CircuitPython_EMC2101 to 1.2.6 from 1.2.5:
  > Merge pull request adafruit/Adafruit_CircuitPython_EMC2101#34 from rpavlik/fix-name

Updating https://github.com/adafruit/Adafruit_CircuitPython_IS31FL3731 to 3.4.1 from 3.4.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_IS31FL3731#55 from grandinquisitor/main

Updating https://github.com/adafruit/Adafruit_CircuitPython_PyPortal to 6.3.4 from 6.3.3:
  > Merge pull request adafruit/Adafruit_CircuitPython_PyPortal#134 from justmobilize/ruff-updates

Updating https://github.com/adafruit/Adafruit_CircuitPython_NTP to 3.3.0 from 3.2.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_NTP#39 from mMerlin/main

Updating https://github.com/adafruit/Adafruit_CircuitPython_PortalBase to 2.1.1 from 2.1.0:
  > Merge pull request adafruit/Adafruit_CircuitPython_PortalBase#102 from justmobilize/ruff-updates

Updating https://github.com/adafruit/Adafruit_CircuitPython_Requests to 4.1.6 from 4.1.4:
  > Merge pull request adafruit/Adafruit_CircuitPython_Requests#202 from FoamyGuy/precommit_copyright
  > Merge pull request adafruit/Adafruit_CircuitPython_Requests#173 from justmobilize/ruff-updates

Updating https://github.com/adafruit/Adafruit_CircuitPython_Bundle/circuitpython_library_list.md to NA from NA:
  > Updated download stats for the libraries
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FoamyGuy FoamyGuy FoamyGuy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@grandinquisitor @FoamyGuy