quantum: led: split out led_update_ports() for customization of led behaviour

[t-8ch]

Allow implementors of led_update_user() to modify the current led_state
but still use the default functionality of led_update_kb().
This can be used to reuse one of the standard LEDs for a different
usecase without having to reimplement led_update_kb().

Fixes #13272

This is still missing updates to the docs and keymaps.
Will do those after a first round of feedback.

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

Issues Fixed or Closed by This PR

• [Feature Request] Provide a way to modify the led_state consumed by led_update_kb from led_update_user #13272

Checklist

• My code follows the code style of this project: C, Python
• I have read the PR Checklist document and have made the appropriate changes.
• 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).

[fauxpark]

LED state should never be modified; it will get out of sync with the host LED state.

[t-8ch]

@fauxpark Only the copy inside led_update_kb() right before triggering the physical LEDs is modified. So this should not desync any actual state.

An alternative would be to move the physical LED logic in led_update_kb() to its own freestanding function that users can call from their own led_update_user().

[drashna]

So this should not desync any actual state.

The simple act of changing it will desync it. The KB/user level led state will no longer match the data that has been given to the keyboard. it will no longer be accurate.

[t-8ch]

With "state" you mean the physical LED?
My goal is to use the scroll_lock LED as a layer indicator instead.
The custom layout does not have a scroll-lock key anyways.

[drashna]

But this does effect every board that uses the function. And it could definitely cause issues for other boards.

Also, this will break any board that calls the led_update_user function.

[t-8ch]

Fair enough.
What would you think about splitting the pin logic from led_update_kb() into it's own function led_update_io() (or something).
Then I can fiddle with my led_state as I want and can call that function from my led_update_user() and the prototype stays the same for everybody.

[drashna]

I was thinking about suggesting something like that. At least, at the kb level.

[t-8ch]

Ok, I'll update the PR then

[t-8ch]

@drashna @fauxpark This should be ready for another round of reviews.

[t-8ch]

@drashna @fauxpark This is now implemented in the discussed way to not require any changes to existing user code. Would you do another review?

[github-actions]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[joukewitteveen]

I can't find what the PR was like initially, but in its current state it looks like an improvement to me. Certainly something deserving another chance.

[t-8ch]

/unstale

[drashna]

Needs a format pass, at the very least

[t-8ch]

@drashna I tried to keep the changes minimal. The format issues are in the existing code.

[t-8ch]

@drashna For some reason the same formatting is accepted with the old function but not the new one.
Reformatting the nested ifdef looks weird, so I removed one level if #ifdef as it is not needed.

[github-actions]

Thank you for your contribution!
This pull request has been automatically marked as stale because it has not had activity in the last 45 days. It will be closed in 30 days if no further activity occurs. Please feel free to give a status update now, or re-open when it's ready.
For maintainers: Please label with bug, awaiting review, breaking_change, in progress, or on hold to prevent the issue from being re-flagged.

[joukewitteveen]

I wonder if this can be improved so that the moonlander code gets a little simpler. The moonlander has 6 leds and largely foregoes led_update_kb in favor of some bespoke logic. I can't figure out exactly how the current MR would streamline this, but I have a feeling it should be able to. Possibly, it needs to make led_update_ports __attribute__((weak)).

[t-8ch]

@joukewitteveen
Not sure about moonlander but making led_update_ports() weak would allow us to replace custom implementations of led_update_kb() that only delegate to other led mechanisms.

For example in keyboards/nullbitsco/tidbit/tidbit.c

// Use Bit-C LED to show NUM LOCK status
bool led_update_kb(led_t led_state) {
    bool res = led_update_user(led_state);
    if (res) {
        set_bitc_LED(led_state.num_lock ? LED_DIM : LED_OFF);
    }
    return res;
}

would become

// Use Bit-C LED to show NUM LOCK status
void led_update_ports(led_t led_state) {
  set_bitc_LED(led_state.num_lock ? LED_DIM : LED_OFF);
}

Looking at custom implementations of led_update_kb() there seem to be quite a few that fit this pattern.

[tzarc]

Reading through this PR and its implementation, I must be missing something.

Can I just confirm the intent -- this PR is asking for the ability to manipulate the led_state, in order to avoid some manual function calls?

Why can't your example of:

// Use Bit-C LED to show NUM LOCK status
void led_update_ports(led_t led_state) {
  set_bitc_LED(led_state.num_lock ? LED_DIM : LED_OFF);
}

...just become:

// Use Bit-C LED to show NUM LOCK status
bool led_update_user(led_t led_state) {
  set_bitc_LED(led_state.num_lock ? LED_DIM : LED_OFF);
  return false;
}

...without the extra function?

What problem are we actually trying to solve here?

[joukewitteveen]

I see two reasons.

1. You may want to change the LEDs not only when the traditional LED state changes. For instance, you may want to change the LEDs on a layer change.
2. A keyboard may want to implement a different way to physically change the LEDs without changing the state change logic.

[t-8ch]

Thanks!
Exactly hat @joukewitteveen explained. For me I want to repurpose the capslock key as layer indicator, as I have disabled capslock anyways. And it's nicer to be able to simply express: "Use the capslock LED for layer indication, keep the rest the same" than having to copy-paste all of the low-level logic.