[beken-72xx] Fix stopping PWM, use pin-scoped PWM struct

[szupi-ipuzs]

If you look into the beken sdk, it seems that one of the actions taken on CMD_PWM_DEINIT_PARAM is actually setting the pin as gpio input. I believe this was unexpected in LT and was screwing the state after. So... maybe it's better not to call it at all, and instead use CMD_PWM_UNIT_DISABLE? Although I don't know what side effects this might have. This seems to fix #200 , at least on my Woox ambient lamp.

Also, CMD_PWM_INIT_LEVL_SET_LOW seems to be called in example sdk code, when duty cycle is being set to 0. I guess there's no hurt doing this too :)

Also, some sddev_control commands expect channel number as parameter, but they expect a 32bit value, while in LT you used 8bit. To be on the safe side, maybe it's better to fix this.

[szupi-ipuzs]

Just FYI, I've tested this fix on my other (LSC) ambient lamp (which has bk7231t) and have not noticed any issues, so I guess it's safe to be used. However, I reckon it would be good if other users test it on their devices before merging.

I realize this change contains actually 3 different kind of fixes. If you prefer, I can split them into 3 separate PRs and test each of them separately.

[kuba2k2]

Sorry it took me so long, but I've looked at the PR now and noticed one thing.

What your fix does (apart from the 32-bit values), is essentially "pausing" the PWM peripheral instead of fully "disabling" it. From what I understand, this will now not allow switching a pin used as PWM back to GPIO.

Since the whole problem is NOT caused by ESPHome disabling PWM (by calling pinRemoveMode() or similar), a 'proper' fix would be to put your code (CMD_PWM_INIT_LEVL_SET_LOW/CMD_PWM_UNIT_DISABLE) in the else clause when dutyCycle is 0. So, instead of calling pinMode and digitalWrite (one of which will then call pinRemoveMode and disable the PWM completely), the fix would simply "pause" the PWM output.

Also, yes, sddev does indeed read and store a 32-bit value, but it gets automatically trimmed down to 8-bit when calling the actual implementation: bk_err_t pwm_unit_enable(UINT8 ucChannel)

[szupi-ipuzs]

it gets automatically trimmed down to 8-bit

I wouldn't call it trimming, there's first a 32bit fetch from memory:

ucChannel = (*(UINT32 *)param);

In normal scenarios, there would be a compiler warning about "narrowing convertion". Depending on the endiannes and structure field alignment this might be a problem or not. Regardless, I would fix this as it is potentially dangerous.

What your fix does (apart from the 32-bit values), is essentially "pausing" the PWM peripheral instead of fully "disabling" it. From what I understand, this will now not allow switching a pin used as PWM back to GPIO.

You might be right, but I don't really understand, why this matters? This output is configured as PWM in yaml, so why would esphome want to configure it differently?

a 'proper' fix would be to put your code (CMD_PWM_INIT_LEVL_SET_LOW/CMD_PWM_UNIT_DISABLE) in the else clause when dutyCycle is 0

Will try that and let you know.

[kuba2k2]

You might be right, but I don't really understand, why this matters? This output is configured as PWM in yaml, so why would esphome want to configure it differently?

The thing is, LibreTiny is a PlatformIO development platform, which is supposed to match the Arduino APIs. There are developers using it without ESPHome - writing their own code in C/C++. We don't want to introduce any potential behavior changes. If someone uses a PWM pin and then decides to use it as a simple GPIO again, that scenario has to be covered.

I wouldn't call it trimming, there's first a 32bit fetch from memory:

Yes, the value is fetched as a 32-bit integer, which means that next fields from the struct might be included. But, as I previously pointed out, this doesn't matter since calling pwm_unit_enable() automatically casts the value to uint8_t (discarding everything past 8 bits) and calls the function with an 8-bit value, always.

[szupi-ipuzs]

calls the function with an 8-bit value, always.

Yes, but which 8bits exactly out of these 32? Are you sure the ones you intend? This code snippet will show you the problem - if you run it on little endian system, you'll get the correct value, but on big-endian - a garbage. The same applies here. I've just checked that bk7231 is little-endian, but how about realteks? IMHO, leaving it like it is seems risky.

There are developers using it without ESPHome - writing their own code in C/C++

Ok, now I understand. Then we can't break the expected behavior.

[Cossid]

calls the function with an 8-bit value, always.

Yes, but which 8bits exactly out of these 32? Are you sure the ones you intend? This code snippet will show you the problem - if you run it on little endian system, you'll get the correct value, but on big-endian - a garbage. The same applies here. I've just checked that bk7231 is little-endian, but how about realteks? IMHO, leaving it like it is seems risky.

You're modifying the Beken-exclusive adaptation. Realtek is in a different sub-folder. Endian assumptions inside the platform folder are safe.

[szupi-ipuzs]

Ok, I understand. Sorry for investigating so hard, it's just my code smell is very sensitive about such bugs :)

[kuba2k2]

Actually, is there a big-endian compiler that can execute code on godbolt? I looked through the list but didn't find any, sadly.

Both Realtek and Beken are ARM chips, and most ARM chips are little-endian. Sure enough, 32-bit word operations are faster than extracting parts of the word. I too do very much prefer clean code, hence the clang-format checks, comments and line-spacing. To make your fix perfect I'd just rename the variable to simply channel to keep it concise and clear.

[szupi-ipuzs]

Actually, is there a big-endian compiler that can execute code on godbolt? I looked through the list but didn't find any, sadly.

I've been looking for it too and was surprised there isn't any on godbolt. You can set the compiler to ARM and pass it "-mbig-endian" flag. But not execute. So, yeah, I haven't actually tested my code snippet on bigendian, but I'm pretty sure I know how it would work :)

[szupi-ipuzs]

Apparently I was right from the start, when I said this single memory block could be a problem:

static pwm_param_t pwm;

There might not be multiple threads accessing this, but still there are multiple instances of PWM pins using the exact same memory location for tracking their state. Apparently this only made a difference when PWM was being stopped.

Once I moved pwm_param_t inside the PinData_s (so it is now unique for a pin), the problem is gone!

Btw. there will probably be the same problem with adc...

[Cossid]

Btw. there will probably be the same problem with adc...

While I'm not advocating for it not to be fixed, there is only 1 ADC GPIO on Beken modules. P23 is ADC3, and used exclusively. Technically P2-P5 are also ADC alternates, but no module we have seen so far have them available.

[kuba2k2]

Wow, the whole sddev thing sucks... Seriously, why couldn't PWM just use a simple function to set duty cycle/frequency to a channel..?

Anyway, I see the problem now. pinRemoveMode sets the DISABLE bit, and that is not changed when updating the duty cycle later in analogWrite. So it's possible that this bit makes sddev go crazy and disable PWM along with changing the duty cycle...

One day we're going to have a proper, register-based reimplementation of GPIO peripherals... hopefully.

[pataha]

Still happens with this PR on 7231N I have rgbww bulb installed 1.4.1 and this PR In CCT mode

• Sometimes it goes maximum power in cold/warm channel and then can not do brightness adjustment
• Sometimes it flash and goes off then can not adjust the brightness

In rgb mode it still happens as above

[kuba2k2]

How (and where) did you install this PR?

[pataha]

bk72xx:
  board: generic-bk7231n-qfn32-tuya
  framework:
    version: dev

this is how I did

[kuba2k2]

Well, how do you know it used this PR then? You might have an old dev version laying around.

Add the debug: component and post its output. The LibreTiny version will contain a commit hash.

[pataha]

Yes. I checked the version hash it's latest from master branch

[kuba2k2]

So the results are the same with and without the PR?

@szupi-ipuzs any ideas what might be wrong here?

[pataha]

@kuba2k2 yes, the same both versions the way I tested is

• Toggle between maximum 2 cold/warm channels (100% warm, 0% cold vice versa)
• Lower brightness to 50% (<100%), toggle between 2 channels Both result the same, sometimes goes max sometimes off

it does not happen if changing value >0%, warm 100%, cold 50%

[kuba2k2]

Is your bulb RGBCW or RGBCT? Can you post your YAML config?

[szupi-ipuzs]

@kuba2k2 , no idea at the moment, but I can confirm I observe similar behavior with the scenario that @pataha provided. Will look into it.

I propose that we create a separate github issue for this, will be easier to track.

[pataha]

@kuba2k2 it's RGBCW

here is the config, cut off other part for simplicity tried with rgbww and cwww platform, same result

output:
  - platform: libretiny_pwm
    id: output_red
    pin: P8
  - platform: libretiny_pwm
    id: output_green
    pin: P7
  - platform: libretiny_pwm
    id: output_blue
    pin: P6
  - platform: libretiny_pwm
    id: output_cold
    pin: P26
  - platform: libretiny_pwm
    id: output_warm
    pin: P24

light:
  - platform: rgbww
    id: light_rgbww
    name: Light
    color_interlock: true
    cold_white_color_temperature: 6500 K
    warm_white_color_temperature: 2700 K
    red: output_red
    green: output_green
    blue: output_blue
    cold_white: output_cold
    warm_white: output_warm
    default_transition_length: 3s

  # - platform: cwww
  #   name: Light
  #   cold_white: output_cold
  #   warm_white: output_warm
  #   cold_white_color_temperature: 6500K
  #   warm_white_color_temperature: 2700K
  #   constant_brightness: true

[szupi-ipuzs]

Well, it seems using one PWM output somehow affects the other. Since the pwm structure is now pin-scoped there seem to be no possibility to mix them accidentally (will double check this). I've seen in the sdk that the N-chips have something called PWM groups. This smells to me like a kind of coupling that could manifest this way as well. Maybe we have accidentally enabled (or not disabled) the groups feature?

[szupi-ipuzs]

I have created a simple setup in which I can set raw values to each pwm channel:

number:
  - platform: template
    id: cold_pwm_number
    restore_value: yes
    initial_value: 50
    name: PWM cold
    icon: "mdi:chip"
    entity_category: config
    min_value: 0
    max_value: 100
    step: 10
    optimistic: true
    on_value:
      then:
        lambda: !lambda |-
          id(output_cold).set_level(x/100);

  - platform: template
    id: warm_pwm_number
    restore_value: yes
    initial_value: 50
    name: PWM warm
    icon: "mdi:chip"
    entity_category: config
    min_value: 0
    max_value: 100
    step: 10
    optimistic: true
    on_value:
      then:
        lambda: !lambda |-
          id(output_warm).set_level(x/100);

If both channels are nonzero, everything works fine. If I set one channel to 0 (so: disable), then changing the other has no effect. Until I set the first one to non-zero again. It goes both ways.

My cold & warm channels are on different pins (P7 & P6) than @pataha's, but they are also adjacent, this might play a role here.

[szupi-ipuzs]

@pataha , I don't know if you followed, but there's a pending fix for this issue, see #222