Feature/usb report leds

[ci-bus]

Feature: Read report leds from usb

Example to test it

1. In board_file.conf add: CONFIG_ZMK_USB_REPORT_LEDS=y
2. In rgb_underglow.c add:

int zmk_rgb_hid_listener(const zmk_event_t *eh) {
    const struct zmk_keycode_state_changed *ev = as_zmk_keycode_state_changed(eh);
    uint8_t receive_report[2] = { 0, 0 };
    if (ev && !ev->state) {
        zmk_usb_hid_receive_report((uint8_t *)receive_report, 2);   
        if (receive_report[0]) {
            if (receive_report[1] == HID_USAGE_LED_CAPS_LOCK) {
                zmk_rgb_underglow_on();
            } else {
                zmk_rgb_underglow_off();
            }
        }
    }
    return 0;
}

ZMK_LISTENER(zmk_rgb_hid_listener, zmk_rgb_hid_listener);
ZMK_SUBSCRIPTION(zmk_rgb_hid_listener, zmk_keycode_state_changed);`

this example code will turn on the LEDs when caps is locked

[joelspadin]

Is there a callback that gets called when there is new data to read? Possibly struct hid_ops::get_report? If so, it would be nice if we could create a ZMK event from that, as that would be more reliable than reading after every key press. (For example, what if you have a keyboard and a separate numpad, and the OS shares num lock state between keyboards? It would be nice if the keyboard could immediately get the new num lock state without waiting for you to press something on it.)

[joelspadin]

This looks copied from zmk_usb_hid_send_report(), but I'm not sure that code makes sense.

If hid_int_ep_read() requires the semaphore to be taken, then I would expect us to check the return value of k_sem_take() to make sure we actually took the semaphore. As is, it looks like this code will try to take the semaphore for 30 ms, then continue regardless of whether it succeeded. Then if hid_int_ep_read() fails, it could give a semaphore it never took.

@petejohanson could you explain how the semaphore code in zmk_usb_hid_send_report() is supposed to work?

[joelspadin]

Maybe it would make more sense to just always support reading the report? Unless there's some reason that the HID report descriptor might need to be kept short, I think making this conditional would just make things more confusing. Any features that depend on this would also need to be conditional, and then anyone who wants to use it in their code has to know to turn it on.

[joelspadin]

Is the ret_bytes parameter to hid_int_ep_read() not necessary? I would expect calling code to need to be able to check how many bytes were actually read, since it might be smaller than len.

[joelspadin]

You should install clang-format (https://releases.llvm.org/download.html) and run your code through it. Otherwise the automated checks will complain that this isn't formatted correctly.

If you're using VS Code and clang-format is installed, Microsoft's C/C++ extension can format files with Shift+Alt+F or automatically on save by setting the editor.formatOnSave setting.

[joelspadin]

Question for someone more familiar with USB/HID than I am: is it okay to attempt to read regardless of the USB status, or do we need a switch like the one in zmk_usb_hid_send_report()?

[joelspadin]

Minor: it would be nice if the declarations in the .h file were in the same order as the definitions in the .c file.

[petejohanson]

@ci-bus Thoughts on the feedback from Nick and Joel?

[ci-bus]

@ci-bus Thoughts on the feedback from Nick and Joel?

Yes, I have to make improvements, I need free time for this

[bortoz]

I made an improved version of this PR which is more user-friendly and works with both USB and BLE: #999

[ci-bus]

I made an improved version of this PR which is more user-friendly and works with both USB and BLE: #999

thanks, I will take a look at this