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

feature: LED indicators #999

Closed
wants to merge 2 commits into from
Closed

Conversation

bortoz
Copy link
Contributor

@bortoz bortoz commented Oct 23, 2021

#115

This PR adds support for LED indicators, such as Caps Lock LED and Num Lock LED.

@joelspadin
Copy link
Collaborator

FYI, this seems to have some overlap with #904. It would be nice to make sure we don't end up with competing PWM LED systems.

@bortoz
Copy link
Contributor Author

bortoz commented Oct 30, 2021

FYI, this seems to have some overlap with #904. It would be nice to make sure we don't end up with competing PWM LED systems.

LED indicators do not require PWM LEDs, you can use any LED driver supported by Zephyr. I added the PWM LED driver because the one in Zephyr 2.5 is not working properly but it is only a temporary solution until Zephyr is upgraded.

@ci-bus
Copy link

ci-bus commented Oct 31, 2021

I think it would be better to do two prs, a pr to read the report and another for the implementations

@bortoz
Copy link
Contributor Author

bortoz commented Oct 31, 2021

I think it would be better to do two prs, a pr to read the report and another for the implementations

Is there a particular reason for doing this?

@ci-bus
Copy link

ci-bus commented Nov 4, 2021

I think it would be better to do two prs, a pr to read the report and another for the implementations

Is there a particular reason for doing this?

For two reasons, first so that this is faster and easier to do the code review and second because of what has been working in backlight, the implementation may change, in my design the caplock led is the last led of the underglow, is it possible configure it with your implementation?

@bortoz
Copy link
Contributor Author

bortoz commented Nov 4, 2021

For two reasons, first so that this is faster and easier to do the code review and second because of what has been working in backlight, the implementation may change, in my design the caplock led is the last led of the underglow, is it possible configure it with your implementation?

This PR and backlight PR are independent of each other, you can use only one of them or both together without any problem. As I said earlier this PR does not depend on PWM LEDs, any LED driver can be used.

Regarding your behavior, I don't think it's achievable as the underglow API doesn't allow changing individual LEDs (and modifying those API is beyond the intentions of this PR). However the current implementation of LED indicators allows you to implement a custom behavior, you can find more details in the documentation.

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few initial thoughts. If you don't want to do this as two PRs, please at least split the code additions into two parts:

  • The generic state management and ties into USB and GATT HID systems.
  • The LED display handling of the new state.

That will make this way easier to review.

Thanks!

app/include/zmk/events/led_indicator_changed.h Outdated Show resolved Hide resolved
app/src/hog.c Outdated Show resolved Hide resolved
app/src/host.c Outdated Show resolved Hide resolved
app/src/host.c Outdated Show resolved Hide resolved
@ReFil
Copy link
Contributor

ReFil commented Feb 22, 2022

Been testing this over the past little while, usb behaviour is flawless after i added a subscriber to update when it's connected to handle state changes whislt disconnected, although i'm having significant trouble getting it to work over BLE, tried windows mac and linux and i cannot make it work, have you got any suggestions?

@bortoz
Copy link
Contributor Author

bortoz commented Feb 23, 2022

Been testing this over the past little while, usb behaviour is flawless after i added a subscriber to update when it's connected to handle state changes whislt disconnected, although i'm having significant trouble getting it to work over BLE, tried windows mac and linux and i cannot make it work, have you got any suggestions?

I'm not sure why it doesn't work, but I haven't had a lot of time in the past months to test it properly

@ReFil
Copy link
Contributor

ReFil commented Feb 23, 2022

i've found the problem, it's that the source is not being set properly in your indexing function in led_indicators.c. i'd reccomend replacing the source index setup with what i've done here: ReFil@4733078 which i've been testing across multiple bluetooth profiles as well as usb

@bortoz
Copy link
Contributor Author

bortoz commented Feb 23, 2022

i've found the problem, it's that the source is not being set properly in your indexing function in led_indicators.c. i'd reccomend replacing the source index setup with what i've done here: ReFil@4733078 which i've been testing across multiple bluetooth profiles as well as usb

Thanks! I will look into that

@bortoz bortoz force-pushed the led-indicators branch 5 times, most recently from 1e33ee7 to bd722f5 Compare February 24, 2022 13:56
@bortoz
Copy link
Contributor Author

bortoz commented Feb 24, 2022

@ReFil I made a lot of improvements, can you test the latest version?

@ReFil
Copy link
Contributor

ReFil commented Mar 15, 2022

Spotted another bug, currently consumer reports dont write. the fix entailed changing

.attr = &hog_svc.attrs[10],
the size from 10 to 12

@bortoz
Copy link
Contributor Author

bortoz commented Mar 15, 2022

Spotted another bug, currently consumer reports dont write. the fix entailed changing

Thank you! Fixed

Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions.

app/include/dt-bindings/zmk/led_indicators.h Outdated Show resolved Hide resolved
app/dts/bindings/zmk,led-indicators.yaml Outdated Show resolved Hide resolved
app/src/usb.c Outdated Show resolved Hide resolved
@bortoz bortoz force-pushed the led-indicators branch 9 times, most recently from ffb70a5 to ef49dff Compare April 16, 2023 16:57
@bortoz bortoz force-pushed the led-indicators branch 3 times, most recently from 1065491 to d27698f Compare May 16, 2023 06:49
chrisandreae added a commit to moergo-sc/zmk that referenced this pull request Jun 27, 2023
@bortoz bortoz requested a review from a team as a code owner July 6, 2023 21:43
bryanforbes pushed a commit to bryanforbes/zmk that referenced this pull request Aug 9, 2023
@petejohanson petejohanson self-assigned this Nov 8, 2023
@petejohanson petejohanson added enhancement New feature or request core Core functionality/behavior of ZMK labels Nov 8, 2023
Copy link
Contributor

@petejohanson petejohanson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bortoz Thank you for your patience on this! One or two minor comments on the coding conventions, but otherwise this looks great. I'll get the HID tweaks in a PR right away to get that in, while you work on the couple other comments here.


#pragma once

typedef uint8_t zmk_hid_indicators;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually follow the convention of using a _t suffix for types like these:

Suggested change
typedef uint8_t zmk_hid_indicators;
typedef uint8_t zmk_hid_indicators_t;

}

zmk_hid_indicators zmk_hid_indicators_get_profile(struct zmk_endpoint_instance endpoint) {
int profile = zmk_endpoint_instance_to_index(endpoint);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
int profile = zmk_endpoint_instance_to_index(endpoint);
const int profile = zmk_endpoint_instance_to_index(endpoint);

}

static void raise_led_changed_event(struct k_work *_work) {
zmk_hid_indicators indicators = zmk_hid_indicators_get_current_profile();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
zmk_hid_indicators indicators = zmk_hid_indicators_get_current_profile();
const zmk_hid_indicators indicators = zmk_hid_indicators_get_current_profile();

Comment on lines +57 to +58
uint8_t indicators = report->leds;
zmk_hid_indicators_set_profile(indicators, endpoint);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
uint8_t indicators = report->leds;
zmk_hid_indicators_set_profile(indicators, endpoint);
zmk_hid_indicators_t indicators = (zmk_hid_indicators_t)report->leds;
zmk_hid_indicators_set_profile(indicators, endpoint);

Comment on lines +20 to +22
#define HID_REPORT_ID_KEYBOARD 0x01
#define HID_REPORT_ID_LEDS 0x01
#define HID_REPORT_ID_CONSUMER 0x02
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I missed you had added these here. I had rolled a similar tweak into #1991 with different naming conventions. Let me pull that out into a small refactor PR (where it should be anyways, and we can then build on it for this and that PR as well.

@petejohanson
Copy link
Contributor

@bortoz See #1993 for the small hid.h refactor.

Comment on lines +705 to +707
int err = bt_gatt_write_without_response(peripherals[i].conn,
peripherals[i].update_hid_indicators, &indicators,
sizeof(indicators), true);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See chrisandreae@bc824de from the MoErgo folks for a small safety check that would be good here, if this gets called mid-attribute discovery.

@@ -37,6 +37,8 @@ target_sources(app PRIVATE src/behaviors/behavior_reset.c)
target_sources_ifdef(CONFIG_ZMK_EXT_POWER app PRIVATE src/behaviors/behavior_ext_power.c)
if ((NOT CONFIG_ZMK_SPLIT) OR CONFIG_ZMK_SPLIT_ROLE_CENTRAL)
target_sources(app PRIVATE src/hid.c)
target_sources_ifdef(CONFIG_ZMK_USB app PRIVATE src/usb_hid.c)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is no longer needed after #2006 was merged since CONFIG_ZMK_USB now depends on the if condition

@petejohanson
Copy link
Contributor

@bortoz Thank you so much for the work on this! I've merged #2041 just now that includes all your core work with the minor changes/additions as noted there. I am truly sad this has taken so long, and I appreciate your persistence in sticking with this. I'm hoping we can build out various status/display bits on top of this now that the core has been merged.

❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core functionality/behavior of ZMK enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.