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

Correct math on fade time #55

Merged
merged 1 commit into from
Jul 29, 2024
Merged

Correct math on fade time #55

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions adafruit_is31fl3731/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,9 +244,9 @@ def fade(
default), the breath feature is used for fading. if fade_in is None, then
fade_in = fade_out. If fade_out is None, then fade_out = fade_in

:param fade_in: int positive number; 0->100
: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.

:param pause: pause time in ms, range = 3.5 to 448
"""
if fade_in is None and fade_out is None:
self._register(_CONFIG_BANK, _BREATH2_REGISTER, 0)
Expand All @@ -261,13 +261,14 @@ def fade(
if fade_out != 0:
fade_out = int(math.log(fade_out / 26, 2))
if pause != 0:
pause = int(math.log(pause / 26, 2))
pause = int(math.log(pause / 3.5, 2))
if not 0 <= fade_in <= 7:
raise ValueError("Fade in out of range")
if not 0 <= fade_out <= 7:
raise ValueError("Fade out out of range")
if not 0 <= pause <= 7:
raise ValueError("Pause out of range")

self._register(_CONFIG_BANK, _BREATH1_REGISTER, fade_out << 4 | fade_in)
self._register(_CONFIG_BANK, _BREATH2_REGISTER, 1 << 4 | pause)

Expand Down
Loading