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

PCA9685 LED Driver maximum brightness dips #329

Open
stephenhensley opened this issue Apr 28, 2021 · 8 comments · May be fixed by #415
Open

PCA9685 LED Driver maximum brightness dips #329

stephenhensley opened this issue Apr 28, 2021 · 8 comments · May be fixed by #415
Labels
bug Something isn't working

Comments

@stephenhensley
Copy link
Collaborator

I noticed this a while ago, and I guess totally forgot to make an issue. Just want to be clear this is not new or related to the minor change in #327

It seems like when an LED is set to its maximum value it actually dips in brightness a little bit. I first noticed this on the Daisy Field, but I have since noticed it on a few custom boards as well.

I haven't really had a chance to investigate it yet, but it can be fairly noticeable in certain settings. So I want it to be documented.

I suspect it might be the result of having the on/off times being super close to each other. I don't recall experiencing this issue with old pre-daisy PCA9685 code that I have, but it's been a while since I've looked at the differences between what we have now and some of my 2-3 year old C drivers.

@TheSlowGrowth have you noticed this behavior on your end as well? I know you've used this led driver quite a bit as well.

@stephenhensley stephenhensley added the bug Something isn't working label Apr 28, 2021
@ch-nry
Copy link

ch-nry commented Apr 29, 2021

I can confirm that.
I suspect the problem is that the "full brightness" bit is used only on next PWM cycle. Th content of on and off are still used during the curent PWM cycle.

There are multiple solution to solve this. Since I also don't like the curve on the low intensity, I just made an other curve that never go to 4095, but stop at 4094...

A better way is probably to do something like this (but I did not tried it)
const auto on = draw_buffer_[d].leds[ch].on & (0x0FFF); // remove full brightness bit
if(rawBrightness >= 0x0FFF)
draw_buffer_[d].leds[ch].off = (on + 0x0FFF) & (0x0FFF); // set to almost full for this pwm cycle
draw_buffer_[d].leds[ch].on = 0x1000 | on; // set "full on" bit for next pwm cycle
else
draw_buffer_[d].leds[ch].off = (on + rawBrightness) & (0x0FFF);
draw_buffer_[d].leds[ch].on = on; // clear "full on" bit

@ch-nry
Copy link

ch-nry commented Apr 29, 2021

btw, it should be if(rawBrightness > 0x0FFF), since it's it should work correctly with rawBrightness = 0x0FFF...

@TheSlowGrowth
Copy link
Contributor

Hmm, I had something like this initially but I thought i had fixed it... I use my led drivers for "peak-meter"-style led indicators and never noticed something weird with them. But I guess those are flickering all the time and such an error would easily go unnoticed.

@ch-nry Not sure if I understand you correctly. 0x0FFF == 4095 - so 0x1000 is not even possible, correct? So if(rawBrightness > 0x0FFF) would never evaluate to true.
Also (on + 0x0FFF) & (0x0FFF) seems wrong to me? I'll have to study the datasheet again.

@ch-nry
Copy link

ch-nry commented Apr 29, 2021

@TheSlowGrowth : I don't understand the problem : if "rawBrightness" is >= 0x1000, then the full on bit will be set, and the led will be on forever.
rawBrightness is a 16 bit number, so the value can go up to 0xFFFF.

if "rawBrightness" == 0x0FFF , then draw_buffer_[d].leds[ch].off = (on + rawBrightness) & (0x0FFF);
i.e : off = on-1,
i.e : led is 4095/4096 time ON and 1/4096 OFF.

Without the full on bit, the led can't be switch on permanently, since if on == off, the led is off all the time.

All in all, the control of the led is from 0 to 4096, I.E from 0x0000 to 0x1000, and not from 0x0000 to 0x0FFF.
You can still use if(rawBrightness >= 0x0FFF), to keep the control from 0 to 0x0FFF, but the 0x0FFF control will be inaccessible: it's not really important.

well, I'll have to test to see if I'm right, but since I've made more modifications to this file, it's not on my todo list.

@stephenhensley
Copy link
Collaborator Author

Yeah, I usually end up having almost all of the LEDs scaled to not go fully bright anyway, so I don't notice it on any of the bigger projects I've worked on, but I noticed it on some of the DaisyExample projects, and with oopsy.

@ch-nry

I suspect the problem is that the "full brightness" bit is used only on next PWM cycle. Th content of on and off are still used during the curent PWM cycle.

This doesn't really explain it because it stays less bright then, say, the next value in the gamma corrected brightness. Once the value is set to 0x0fff the driver sets the bit, and nothing should toggle it off.

Also, I don't think we should change the SetRaw range to handle 0-4096 (to control that bit). I get that the full on bit is at the 0x1000 position, but it's still a 12-bit control. I think it'd be strange to have a non-standard range for something so common.

Honestly, I'm also not really opposed to just adjusting the map to a max of 0x0ffe.. but I am also curious as to why it's doing what it's doing.

@ch-nry
Copy link

ch-nry commented Apr 30, 2021

@stephenhensley

This doesn't really explain it because it stays less bright then, say, the next value in the gamma corrected brightness.

I don't understand what you say.

anyway, I tested and all my solutions works. But if you are ok with the led being at a maximum of 4095/4096, then the function can be simplified to (this is what I use) :

    void SetLed(int ledIndex, float Brightness)
    {
        const uint8_t intBrightness
            = (uint8_t)(clamp(Brightness * 255.0f, 0.0f, 255.0f));
        const uint16_t rawBrightness = gamma_table[intBrightness];

        const auto d  = GetDriverForLed(ledIndex);
        const auto ch = GetDriverChannelForLed(ledIndex);
        const auto on                = draw_buffer_[d].leds[ch].on;
        draw_buffer_[d].leds[ch].off = (on + rawBrightness) & (0x0FFF);
    }

(edited for formating)

@ch-nry
Copy link

ch-nry commented May 1, 2021

since we are discussion about this driver, I know it's not related, but I also made an other modification to this file :
I change the initialisation part to this :

            const uint8_t address = PCA9685_I2C_BASE_ADDRESS | addresses_[d];
            uint8_t       buffer[2];
            buffer[0] = PCA9685_MODE1;
            buffer[1] = 0x00;
            i2c_.TransmitBlocking(address, buffer, 2, 1);
            System::Delay(20);
            buffer[0] = PCA9685_MODE1;
            buffer[1] = 0x00;
            i2c_.TransmitBlocking(address, buffer, 2, 1);
            System::Delay(20);

// ch
            buffer[0] = PCA9685_MODE1;
            // auto increment on
            buffer[1] = 0b00110000; // stand by
            i2c_.TransmitBlocking(address, buffer, 2, 1);

            System::Delay(20);
            buffer[0] = PRE_SCALE_MODE;
            buffer[1] = 0b00000011; // fastest PWM frequency
            i2c_.TransmitBlocking(address, buffer, 2, 1);

            System::Delay(20);
            buffer[0] = PCA9685_MODE1;
            // auto increment on
            buffer[1] = 0b00100000;
            i2c_.TransmitBlocking(address, buffer, 2, 1);
// end of modification

in order to increase PWM frequency to it's maximum. It did not change anything visually, but did reduce flickering when you film your device...

Also, the last part of this file look wrong since buffer 1 is assigned a 9 bit value :

System::Delay(20);
            buffer[0] = PCA9685_MODE2;
            // OE-high = high Impedance
            // Push-Pull outputs
            // outputs change on STOP
            // outputs inverted
            // ch : 0b00010110?
            buffer[1] = 0b000110110; // ch : ??? 9 bit???
            i2c_.TransmitBlocking(address, buffer, 2, 5);

@beserge beserge linked a pull request Oct 8, 2021 that will close this issue
@dromer
Copy link

dromer commented Dec 22, 2023

Hah, I thought it was me but indeed the LEDs on the field do a little "dip" just before you reach the maximum value.

Should've looked for this ticket earlier, as I was contemplating reporting it myself (but also figured it's a bit of a minor issue).

Not exactly sure how the PR from @beserge fixes this, but it would be a nice "low hanging fruit" to finally get resolved in libDaisy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants