[Backlight] Make hardware PWM respect BACKLIGHT_ON_STATE

[fauxpark]

Description

The hardware PWM backlight assumes an NMOS/NPN, and doesn't care about the BACKLIGHT_ON_STATE define. With PMOS/PNP, all that needs to be done is invert the PWM signal by setting the COMxx0 bit, though when disabling the PWM on level 0, we also need to pull the line high, or it will just turn the LED back on.

I had to change the default value of the define to 1, as the vast majority of boards (all current boards with hardware PWM) have NMOS backlight circuits. There will still be some with software PWM that relied on the fact that it defaulted to 0... if necessary I can hunt them down and fix em here.

Thanks to amoz on Discord, for pointing out the CIE lightness curve function can be reused here 😁

Types of Changes

• Core
• Bugfix
• New feature
• Enhancement/optimization
• Keyboard (addition or update)
• Keymap/layout/userspace (addition or update)
• Documentation

Issues Fixed or Closed by This PR

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation accordingly.
• I have read the CONTRIBUTING document.
• I have added tests to cover my changes.
• I have tested the changes and verified that they work and don't break anything (as well as I can manage).

[zvecr]

While impact is low, the default behaviour changing warrants a discussion around if this is a breaking change.

A short term solution could be to have 2 different defaults for now to maintain backwards compatibility, get this PR in, then run the defaults being aligned through the breaking change process.

I have added the label for visibility while the discussion is ongoing.

[fauxpark]

My guess is there are very few boards out there with PNP backlighting, and the ones currently in the repo can easily be fixed by explicitly defining BACKLIGHT_ON_STATE 0, as I mentioned.

I'm not sure if the "don't touch user keymaps" rule extends to keyboards that aren't in the repo, but if it does then that would definitely warrant the breaking change process.

[zvecr]

From previous conversations, the breaking change process is not just for keymaps (though they are the most user facing part of the firmware so would be its primary use). Pulling a quote from https://hackmd.io/EO2XN14kRkaWe7D2wEW6IA

Give users confidence that their update won’t break their setup

Given the rolling release, it makes it difficult to broadcast the change in behaviour.

Anyway, I figured i would flag it so its impact can be discussed.

[noroadsleft]

I'm not sure if the "don't touch user keymaps" rule extends to keyboards that aren't in the repo

I would hope not – there should be zero expectation that QMK changes don't break your keyboard if you don't submit your keyboard to qmk:master IMO. Expecting us to maintain compatibility/functionality of boards we don't know about is an unreasonable expectation IMO.

[zvecr]

@noroadsleft I would agree with you when a branching strategy and/or release process is being followed. However, take something like semver:

Given a version number MAJOR.MINOR.PATCH, increment the:

MAJOR version when you make incompatible API changes,
MINOR version when you add functionality in a backwards compatible manner, and
PATCH version when you make backwards compatible bug fixes.

QMK uses patch for each commit on master, minor for each big change, and forever zero for major. semver isnt a silver bullet and entirely might not apply to the way QMK wants to deliver software, but maintaining backwards compatibility is something that is a realistic expectation in various areas of software delivery.

[noroadsleft]

@zvecr And this is why my opinion on matters is not to be trusted. 😄

[zvecr]

@noroadsleft For completeness, I totally side with you on Expecting us to maintain compatibility/functionality of boards we don't know about is an unreasonable expectation IMO. But its the current release process that makes me go "but how do i know stuff is now broken at runtime?".

[yanfali]

This is starting to smell like a rev issue. Can we just have a nmos/pmos rev folder and put something in the read me about if your keyboard doesn't work try the other rev. It's harmless from a hardware standpoint, I believe as all you would get is a non working backlight if you run the wrong code on the wrong hardware. Vendors are going to rev their hardware, we should signal that in some way to qmk users.

So as explained to me by Zed, Soft PWM has an existing behavior, hardware PWM has the opposite.

This sounds like we're changing the default behavior. I don't think we should do that unless it goes into future. Better to have two behaviors, principle of least astonishment should apply here. I agree with Zed at a future date we go through future branch and align the two APIs

The other option is this goes straight into future. It's about 3 weeks out

[fougner]

Why not just skip the default value change (literally a oneliner) for now and get this fix in? Then we can merge the default value in a few weeks.

Also, general opinion from me when it comes to backwards breaking changes: I'd argue even out-of-tree keyboards should be expected to work if no breaking changes are communicated. This is afterall a firmware platform and people might have local projects that they can't or won't contribute upstreams. It all depends on how "nice" you want to be to the end user.

It's really nice that the quaterly breaking-change window exists. That enables a sort of middleground for not holding on to bad defaults or deprecated APIs but also breaking stuff in an expected and well-communicated way. The only thing that maybe could've been improved is to follow the semantic versioning strictly. I.e. every quarter we do a major release instead do symbolize the breaking changes.

[fauxpark]

Doesn't matter what the default is, it will break either hardware or software PWM. I chose to change it as I estimated there are far fewer boards with the latter.

We could add a separate define for hardware PWM and unify them as part of the breaking change process, but I think I would rather just let this PR roll over into future as is. AFAIK there are currently no boards in the repo that use a PMOS on a hardware PWM pin, because up until now it hasn't been supported.

[noroadsleft]

Thanks!