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

Fix SetLedRaw overflow bug #415

Draft
wants to merge 7 commits into
base: master
Choose a base branch
from
Draft

Fix SetLedRaw overflow bug #415

wants to merge 7 commits into from

Conversation

beserge
Copy link
Contributor

@beserge beserge commented Oct 8, 2021

off = rawbrightness + on
on = startcycle = ledindex << 2.
With lots of drivers ledindex can get quite large, so eventually off = (rawbrightness + on ) > 4095.

There's an if which sets the fullbrightness flag which I changed to
rawbrightness + on > 4095 rather than rawbrightness > 4095

fixes #329

Off value = rawbrightness + on.
On value = startcycle = ledidx << 2.
With lots of drivers ledidx can get quite high
So eventually off = rawbrightness + on > 4095 and the led gets dimmer.
Fixed this by just changing the fullbrightness if to include the on value.
@github-actions
Copy link

github-actions bot commented Oct 8, 2021

Unit Test Results

    1 files  ±  0    16 suites  +3   0s ⏱️ ±0s
145 tests +20  145 ✔️ +20  0 💤 ±0  0 ±0 

Results for commit effaff7. ± Comparison against base commit 2333ad2.

♻️ This comment has been updated with latest results.

src/dev/leddriver.h Outdated Show resolved Hide resolved
@beserge
Copy link
Contributor Author

beserge commented Oct 12, 2021

OK I've got the fix in as described earlier. For now I'm keeping track of updates on a per buffer basis rather than per led, which allows us to keep utilizing the DMA to its full extent.

There is a slightly noticeable blink when you push the led over the edge into the next frame, but I believe it's a function of the previously mentioned hardware aspect rather than the software.

@stephenhensley
Copy link
Collaborator

Nice! So this seems to fix that issue.

However, I am seeing something a bit funky, that is definitely more noticeable/distracting.

I'm using the Daisy Field - Keyboard Test example to verify this btw (in case that makes comparison easier).

So the new issue:

LEDs that are fully bright, flicker upon any changes being written to other LEDs. (e.g. using the field knobs to set LEDs 1,3,5 to fully bright, and then rotating any of the other knobs. You'll see only those three knobs flicker while rotating the knob).


Since it seems the issue is writing the full-on bit, mid-cycle causes the LED to temporarily not have the maximum brightness, A solution that could work, but seems like a bit more internal work than I'd like to imagine is necessary. Is to manage the full-bright LEDs separate from the LEDs that have <100% pulsewidth.

This would make the DMA message longer since we wouldn't be able to use auto-increment anymore, but we'd essentially build a list of address/values, only writing to the ones that had changed.

Alternatively, a much less technically correct, albeit simpler solution would be just limit the maximum brightness to 4095, and not use the 13th bit special 100% feature.

For LEDs this wouldn't really be an issue, but if/when this driver is used for servos, or other specific PWM use cases a true 0-100% may be required.

The fifth transfer fills up the queue and we get stuck on
line 147 of i2c.cpp
@stephenhensley stephenhensley marked this pull request as draft November 29, 2021 20:27
Still a slight blink to be fixed as we jump over the fullon threshold
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.

PCA9685 LED Driver maximum brightness dips
3 participants