libretiny-eu / libretiny

PlatformIO development platform for IoT modules
http://docs.libretiny.eu/
MIT License
382 stars 55 forks source link

PWM implementation appears broken #200

Closed lmcd closed 7 months ago

lmcd commented 8 months ago

Colours are often getting randomly screwed up over PWM.

I've been trying to debug this for a while now, and thought ESPHome was transforming colour values somehow before delivering them over PWM to an RGB bulb I had, but after logging inside libretiny_pwm.cpp, I can see that all the duty cycle values are correct, yet somehow the bulb is often the wrong colour or in the wrong state.

Sometimes I can get the bulb in a state where max duty sent to red channel only and others zero will show a bright red light as expected. And other times, the same values sent over PWM will result in the light being off.

Is there some kind of state that's not being managed in the chipset? I know these chips have on-board PWM - is this not being managed correctly?

lmcd commented 8 months ago

I can see this happening with the red channel often. Everything will be behaving as normal scrubbing through colours, then the red channel will just sort of go offline... then after a while of throwing more RGB values at the bulb, the channel will wake up again.

Maybe the PWM pin is getting stuck in a disabled state somehow?

I have tried ESPHome on 2 different bulbs of the same brand, and seeing identical results. No issue with stock Tuya firmware or openbeken

Cossid commented 8 months ago

Are you able to provide a concrete reproducible case?

lmcd commented 8 months ago

Can confirm commenting out this seems to resolve the problem:

    if (pinEnabled(pin, PIN_PWM)) {
        // disable PWM
        pinRemoveMode(pin, PIN_PWM);
    }
    // force level as LOW
    pinMode(pinNumber, OUTPUT);
    digitalWrite(pinNumber, LOW);

So, some pins are getting stuck in an off state somehow

lmcd commented 8 months ago

Are you able to provide a concrete reproducible case?

I'm not sure exactly how to provide this, but basically I'm cycling through HSV values (saturation and brightness as 1.0) - so a hue-rotate basically.

This should be nice and smooth, but when it gets to certain parts in the cycle, it will glitch and a channel will be disabled.

lmcd commented 8 months ago

I think I've narrowed down the issue to pinEnabled.

If I remove both pinEnabled checks in /cores/beken-72xx/arduino/src/wiring_analog.c I get the expected results.

lmcd commented 8 months ago

Ok with this change I was still seeing a few edge case results where setting 255, 0, 0 (for example) would cause all channels to disable on the first cycle, then setting 255, 0, 0 again would cause the correct red result.

I have now chosen to never disable PWM, and to just set a duty cycle value below the LED's viewable threshold to emulate an 'off' state. This works with no issues or glitches at all:

if (dutyCycle) {
    // enable PWM and set its value
    pwm.cfg.bits.en     = PWM_ENABLE;
    pwm.cfg.bits.int_en = PWM_INT_DIS;
    pwm.cfg.bits.mode   = PWM_PWM_MODE;
    pwm.cfg.bits.clk    = PWM_CLK_26M;
    pwm.end_value       = frequency;
    pwm.p_Int_Handler   = NULL;
    __wrap_bk_printf_disable();
    sddev_control(PWM_DEV_NAME, CMD_PWM_INIT_PARAM, &pwm);
    sddev_control(PWM_DEV_NAME, CMD_PWM_INIT_LEVL_SET_HIGH, &pwm.channel);
    sddev_control(PWM_DEV_NAME, CMD_PWM_UNIT_ENABLE, &pwm.channel);
    __wrap_bk_printf_enable();
    // pass global PWM object pointer
    data->pwm = &pwm;
    pinEnable(pin, PIN_PWM);

    // update duty cycle
    sddev_control(PWM_DEV_NAME, CMD_PWM_SET_DUTY_CYCLE, &pwm);
} else {
        // go round again, but this time write a value low enough that the channel will appear as 'off'
    analogWrite(pinNumber, 1);
}

This is obviously just a hack and not ideal - but fully resolves my PWM issues for now until a proper resolution is identified. I don't know these boards well enough to quickly diagnose the root of this issue.

agreenfield1 commented 8 months ago

I have several wifi rgbww lights of different types and have also observed strangeness when adjusting color. Basically getting unexpected changes to the pwm on the wrong pin (turning off as mentioned). This even occurred when I configured the light as 5 different monochromatic lights.

Specific example: rgbww light configured as five monochromatic lights. Set all five to 50% brightness. Adjusting the brightness of the red light sometimes causes the green light to turn off.

szupi-ipuzs commented 7 months ago

@lmcd where did you apply this change? Could you create a pull request?

I experience the same with one of my devices. I have 2 very similarly looking ambient lights:

I haven't opened any of them, flashed both via cloudcutter (different profiles). WOOX turns out to be CB3S (so BK7231N), the other is an unknown BK7231T (I think I will open it one day to check exactly). Both have a very similar setup, only pwm pin numbers differ a bit. The point is I only experience the problem with WOOX (=CB3S), not with the other one. Which would suggest the problem is specific to BK7231N...

I can try and check what happens if I switch to the generic board (generic-bk7231n-qfn32-tuya), as I currently have board: cb3s in yaml.

szupi-ipuzs commented 7 months ago

I can try and check what happens if I switch to the generic board (generic-bk7231n-qfn32-tuya), as I currently have board: cb3s in yaml.

Nope, didn't help.

lmcd commented 7 months ago

@lmcd where did you apply this change? Could you create a pull request?

I experience the same with one of my devices. I have 2 very similarly looking ambient lights:

  • LSC MOOD LIGHT (970743)
  • WOOX R5145 (2951462)

I haven't opened any of them, flashed both via cloudcutter (different profiles). WOOX turns out to be CB3S (so BK7231N), the other is an unknown BK7231T (I think I will open it one day to check exactly). Both have a very similar setup, only pwm pin numbers differ a bit. The point is I only experience the problem with WOOX (=CB3S), not with the other one. Which would suggest the problem is specific to BK7231N...

I can try and check what happens if I switch to the generic board (generic-bk7231n-qfn32-tuya), as I currently have board: cb3s in yaml.

It's in /libretiny/cores/beken-72xx/arduino/src/wiring_analog.c

I've been using this change for a few weeks now and has completely resolved all issues. Im not gonna do a PR though as it's a hack. I'm hoping someone that knows these boards better than I do can determine what the best fix is.

szupi-ipuzs commented 7 months ago

@lmcd thanks, I see what you did there, although I don't yet fully grasp the meaning :) I took a look at the analogWrite() function and I am wondering if this can be called from more than one thread at the same time? If so, then there is a race condition on the

static pwm_param_t pwm;

being set in this very method.

@kuba2k2 , could you comment on that? Is it possible there's a race condition there? Such a race condition could possibly create the effects we observe...

kuba2k2 commented 7 months ago

It shouldn't be called from more than one thread. ESPHome is single-threaded (AFAIK, at least the main loop probably is). If you want, you can try adding a mutex there to see if it resolves the problem. I am not able to reproduce this issue yet (no PWM bulbs with ESPHome).

lmcd commented 7 months ago

This is not related to threads. The bugs for me are predictable and reproducible. There's something going on with the on/off state for PWM mode getting stuck or not being configured correctly.

My patch just forces PWM to always be on and never switch off, and to emulate 'off', I'm just setting a very low PWM value.

szupi-ipuzs commented 7 months ago

Yes, this occured to me too. Still I would like to try the change with mutex, it costs nothing but time, and as an extra I will learn how to setup my own development environment for libretiny 😄 (any tips for that?).

I was also wondering about this happening on only one of my (almost identical) devices... @lmcd , @agreenfield1 could you check which board/chip is used in the devices you have problems with? Maybe we can narrow it down to some specific setup...

lmcd commented 7 months ago

@szupi-ipuzs: see this: https://github.com/libretiny-eu/libretiny/issues/201

I'm using CB2L Tuya chip, which has bk7231n: https://developer.tuya.com/en/docs/iot/cb2l-module-datasheet?id=Kai2eku1m3pyl

kuba2k2 commented 7 months ago

any tips for that

Install Visual Studio Code with PlatformIO add-on. Clone libretiny repository into ~/.platformio/platforms/libretiny (remove it if it's already there). Create a new PlatformIO project with LibreTiny platform (in VSCode). Add both the LibreTiny directory and the PIO project to a single VSCode workspace, open the workspace. Let it refresh indexes. Copy (or symlink) the .vscode/c_cpp_properties.json file from the PIO project to LibreTiny directory. Make sure to backup your code frequently - PIO might remove/reinstall it if you use version: dev/recommended in ESPHome. To be safe, set version: latest in your YAML. If you don't have ESPHome yet, git clone it and install with pip install -e .. Then run it as usual: esphome compile xxx.yaml.

agreenfield1 commented 7 months ago

The board I've observed the issue with is the CBU module (bk7231n again)

szupi-ipuzs commented 7 months ago

I have finally setup a compilable environment of libretiny with esphome (@lmcd' way), and I'm tinkering around the pwm-related commands. And I have noticed something strange. This could be a false lead, but it'd better to investigate:

The pwm_param_t structure in pwm_pub.h that is downloaded by esphome is different than the one defined eg. here (which I'm guessing is the correct sdk for bk7231n?). Namely, the version used by esphome does not have the init_level0 and init_level1 fields.

Regardless what these fields are for, this could mean one of 2 things: 1) The whole SDK for bk7231n that libretiny+esphome uses is of different version than what I have found - then this difference might be ok. 2) An incorrect structure and/or header is chosen - and this might be because a wrong flavor of sdk was chosen? Although this would cause a whole kind of weird issues...

@kuba2k2 care to explain?

kuba2k2 commented 7 months ago

This is the SDK used by LibreTiny: https://github.com/libretiny-eu/framework-beken-bdk/blob/platformio/beken378/driver/include/pwm_pub.h As you can see, there's no init_level. This SDK is universal (BK7231T, BK7231N, BK7252, etc), while OpenBeken uses Tuya's SDK for BK7231N only.

szupi-ipuzs commented 7 months ago

Oh, I did not realize there's more than one flavor of sdk... Is it then possible PWM is simply broken in the sdk itself? Anyway, I will keep digging and keep asking stupid questions :)

kuba2k2 commented 7 months ago

Probably not, I don't think it's broken. One could try copying over the implementation from Tuya SDK, but it's unlikely to fix anything. The problem must either be in LibreTiny Arduino core, or the way ESPHome uses it.

lmcd commented 7 months ago

Probably not, I don't think it's broken. One could try copying over the implementation from Tuya SDK, but it's unlikely to fix anything. The problem must either be in LibreTiny Arduino core, or the way ESPHome uses it.

I've debugged everything ESPHome is doing, and it's not the source of this issue. This is sometihing to do with libretiny.

Maybe we can look at what OpenBK7231N is doing. Their PWM implementation seems to have a lot more stuff going on. So maybe there's more pins/bits/configuration you need to set to have PWM work reliably.

szupi-ipuzs commented 7 months ago

I think i've found it, see #215