Change keyboard layout or other actions from host

aliher1911 commented 3 years ago

I'm using same keyboard on mac/win/linux and I was after a way to pick default layout depending on what os was booted. I added a serial port to usb descriptor and a handler that responds to layer change commands received.

If there's an interest to have it as part of tmk i can beautify the code and create pull request.

It is reasonably extensible as handler parses commands and just invokes one of actions internally so backlight control and the like could be easily added to it. Mind that I can only add that to lufa driver and test on teensy board.

The choice of serial over custom data protocol or hid console was to be able to send commands from scripts without using any usb libraries.

tmk commented 3 years ago

Yes, I'm interested in mainly how to integrate LUFA CDC ACM.

Can you share the code as is first? So I can check and understand how it is implemented. And I can merge your code myself perhaps?

I don't like to use your time more to make pullreq if possible.

aliher1911 commented 3 years ago

This is the branch on my fork: https://github.com/aliher1911/tmk_keyboard/tree/control-interface I just rebased on master so you are only interested in last 3 commits there. To enable it you need to set SERIAL_ENABLE = yes in keyboard makefile (https://github.com/aliher1911/tmk_keyboard/blob/split-keyboard/keyboard/split/Makefile).

I'm open to changing it so that i don't have to maintain my own version.

tmk commented 3 years ago

Thank you It looks great! I'll look into it closely tomorrow. I'd like to try directing debug outputs(sendchar()) to CDC serial.

BTW, I have plan to change license of codes under tmk_core/ to more permissive one (like MIT, modified BSD or apache2) than current GPL. It would be appreciated if you can grant permissive license on your work.

aliher1911 commented 3 years ago

I'm totally fine with the license change. What do you need for that technically?

I was digging into the CDC module in library and some calls seem to be blocking if output buffer is full. I disabled auto flush in the CDC task and flush buffer after every send in the hope that it would at least make another keyboard scan before sending next block. But overall if amount of data is high it would still block in other method afaik. Those limitations could be removed by basically ditching CDC module methods and reimplementing them in lufa.c. The other option is probably having a big enough ring buffer to keep all the send data. At the moment buffer will also drop writes silently if buffer is full, i thought that it would be a reasonable tradeoff for what i was doing, because control commands and responses are really tiny. But I would try to change sendchar() and see if matrix debug will go through without significantly slowing things up.

tmk commented 3 years ago

I have plan to work something with Nordic Semi nRF5 chip and we are required to link/merge their proprietary Softdevice module to our firmware to use BLE functions. I'm not completely sure but their license doens't seem to be compatible with GNU GPL. Due to this I need 'permisive' license probably. https://developer.nordicsemi.com/nRF5_SDK/nRF5_SDK_v12.x.x/doc/12.3.0/nRF5_Nordic_license.txt I'm thinking MIT license now, I like its simpleness and short license text. https://choosealicense.com/licenses/mit/

I was digging into the CDC module in library and some calls seem to be blocking if output buffer is full. I disabled auto flush in the CDC task and flush buffer after every send in the hope that it would at least make another keyboard scan before sending next block. But overall if amount of data is high it would still block in other method afaik.

This may be related? 'CDC_Device_Flush()' seems to wait when endpoint bank is full.

    if (BankFull)
    {
        if ((ErrorCode = Endpoint_WaitUntilReady()) != ENDPOINT_READYWAIT_NoError)
          return ErrorCode;

        Endpoint_ClearIN();
    }

https://github.com/abcminiuser/lufa/blob/LUFA-170418/LUFA/Drivers/USB/Class/Device/CDCClassDevice.c#L228-L234

Endpoint_WaitUntilReady() can block for 100ms with default config in the worst case. https://github.com/abcminiuser/lufa/blob/LUFA-170418/LUFA/Drivers/USB/Core/AVR8/Endpoint_AVR8.c#L164-L195

Those limitations could be removed by basically ditching CDC module methods and reimplementing them in lufa.c. The other option is probably having a big enough ring buffer to keep all the send data. At the moment buffer will also drop writes silently if buffer is full, i thought that it would be a reasonable tradeoff for what i was doing, because control commands and responses are really tiny. But I would try to change sendchar() and see if matrix debug will go through without significantly slowing things up.

Yes, dropping chars is acceptable when overloaded, memory resource and buffer size is limited in microcontroller. Current HID debug console output also drops chars.

And I agree on integrating CDC feature into lufa.c or place related files under lufa/ directory.

aliher1911 commented 3 years ago

So digging into this further, first implementation had a separation of usb endpoint related operations in lufa and all higher level ops inside serial.c. But if we want to have debug going into CDC endpoint, all the ringbuffer functionality should also go down there. Otherwise sendchar can't really use it. I'm going to sketch how it would look like. If that works, prints would go to CDC and could be monitored from there. Unfortunately startup debug info would still be unavailable if it doesn't fit ringbuffer. I was thinking that maybe it could be pushed synchronously, but we must wait for endpoint to be configured and there's almost nothing inbetween endpoint configure and main loop starting to make it worth. On the other hand, synchronous flushing might make sense for debug info as you may tolerate slower scan times while debugging matrix behaviour. What do you think?

aliher1911 commented 3 years ago

Ok so I changed the way code is organized: https://github.com/aliher1911/tmk_keyboard/commits/control-interface-v2

lufa/cdc.c now contains cdc code with output ringbuffer and exposes putchar so that debug could be redirected if needed. it is enabled by CDC_CONSOLE=yes in make file.
common/remote_control.c contains code that parses commands coming from CDC and does actions as well as confirms with oks. this functionality is enabled with REMOTE_ENABLE=yes.

The endpoint itself is enabled if one of those options is enabled. I tried it and debug is going through just fine (with all the truncation of course if there's not enough data). Default buffer is 32 bytes which is enough for commands, but too small for debug. It could be overridden from makefile.

tmk commented 3 years ago

Thanks for contribution! Just tried v2. Debug prints(xprintf/dprintf) still makes key typing slugish when serial terminal app is not open on host. And severe droping chars is occurs when write many in short period, matrix_print() for example. These are problem to me because I want use CDC output for debug print.

I'm working on CDC console based on your previous control-interface branch. I think issues above were resolved now. https://github.com/tmk/tmk_keyboard/tree/cdc_console

CDC console is enabled with build options below in Makefile.

CONSOLE_ENABLE=yes
CONSOLE_DRIVER=CDC    # HID | CDC

stdin, stdout and functions are available now, hopefully. Not tested closely yet, you can use `getchar()` or similar to get input from user. And to send message on console `xprintf()` or `printf()` can be used. (xprintf is prefered at this point) With these functions you can make your remote control regardless of future console implementation changes. https://www.nongnu.org/avr-libc/user-manual/group__avr__stdio.html On Linux my CDC console seems to work well now but Windows don't recognize the CDC interfaces. I'll test more on Windows. Did you try your v2 on WIndows or Mac?

aliher1911 commented 3 years ago

I was trying to debug. If port wasn't open from host side, it was blocking because endpoint can't flush and CDC_Write methods were blocking. I changed code to check Endpoint_IsInReady() before trying to write to ensure that write won't block.

I think your console code is also prone to blocking like that if CDC_Device_SendByte is called when previous bank is not sent yet. It would wait until ready. What do you want to do with debug output, block further scans until all the data goes through?

I remember in older versions hidconsole also used to drop parts of matrix debugs (I know because I have a large matrix ;), but in recent versions it doesn't happen anymore. I checked the scan frequency and there's definitely a gap in scans after 4.8 ms after keypress what prints matrix and this gap is small when no cdc console is there and larger if cdc is enabled.

tmk commented 3 years ago

As for the blocking problem I found this note on LUFA CDC documenation.

Another major oversight is that there is no mechanism for the host to notify the device that there is a data sink on the host side ready to accept data. This means that the device may try to send data while the host isn't listening, causing lengthy blocking timeouts in the transmission routines. It is thus highly recommended that the virtual serial line DTR (Data Terminal Ready) signal be used where possible to determine if a host application is ready for data.

I confirmed that checking DTR on ControlLineStates before using SendByte() and Flush() is needed and musthave. These checks makes the blocking very rare but there is still very slight chance of block. Setting USB_STREAM_TIMEOUT_MS to 0 would minimizes the blocking time in Endpoint_WaitUntilReady() in the case.

I think debug outputs should not block as users can recognize it, dropping chars would be better than blocking keyboard operation. I want that especially when serial terminal app is not open on host.

With these tricks I have not seen any recogninzable block so far. I tesed with 'cutecom' serial terminal app on Linux. But its situation can vary depeding on app and OS, we need to test more.

I think checking Endpoint_IsINReady() is also useful same way as you refered.

What is your OS and serial terminal?

tmk commented 3 years ago

I just revisited v2 and tested it again.

These settings have in Makefile to enable only CDC console without HID console.

CONSOLE_ENABLE = no     # Console for debug(+400)
CDC_CONSOLE = yes
REMOTE_ENABLE = yes

Applied a patch to enable debug print even with CONSOLE_ENABLE=no. Now debug prints go to CDC console.

diff --git a/tmk_core/common.mk b/tmk_core/common.mk
index de2165b0..242c531e 100644
--- a/tmk_core/common.mk
+++ b/tmk_core/common.mk
@@ -52,10 +52,10 @@ ifeq (yes,$(strip $(CONSOLE_ENABLE)))
 else
     # Remove print functions when console is disabled and
     # no other print method like UART is available
-    ifneq (yes, $(strip $(DEBUG_PRINT_AVAILABLE)))
-       OPT_DEFS += -DNO_PRINT
-       OPT_DEFS += -DNO_DEBUG
-    endif
+#    ifneq (yes, $(strip $(DEBUG_PRINT_AVAILABLE)))
+#      OPT_DEFS += -DNO_PRINT
+#      OPT_DEFS += -DNO_DEBUG
+#    endif
 endif

I confirmed that the keyboard is blocked at startup until serial termainal app is open. After that it is not blocked without block without the app for some reason. Maybe DTR check is needed?

When many characters are sent through cdc_sendchar() in short period(like matrix_print) the first 31 chars are only displayed. You will have to try SendByte() first before place a char into buffer.

I tested this with cutecom on Ubuntu 20.04 for the record.

Let me know if you find any problem on my branch. I'll working on clean up code and API. Just created new issue for CDC Console. https://github.com/tmk/tmk_keyboard/issues/662

aliher1911 commented 3 years ago

This is pretty cool progress! I should be able to easily adapt remote control to new interface. As for OS's used: I tested with linux and minicom successfully, on macos/catalina with minicom as well and it also works fine, but windows 10 is not working. I tried the workaround that was suggested on lufa forum to provide .inf which binds driver to particular interface inside composite (like this one https://github.com/abcminiuser/lufa/blob/master/Demos/Device/ClassDriver/VirtualSerialMouse/LUFA%20VirtualSerialMouse.inf), but it doesn't seem to help.

aliher1911 commented 3 years ago

I tried adding

USB_Descriptor_Interface_Association_t CDC_IAD;

to USB_Descriptor_Configuration_t before CDC interface and then

.CDC_IAD =
    {
       .Header                 = {.Size = sizeof(USB_Descriptor_Interface_Association_t), .Type = DTYPE_InterfaceAssociation},

       .FirstInterfaceIndex    = SERIAL_CCI_INTERFACE,
       .TotalInterfaces        = 2,

       .Class                  = CDC_CSCP_CDCClass,
       .SubClass               = CDC_CSCP_ACMSubclass,
       .Protocol               = CDC_CSCP_ATCommandProtocol,

       .IADStrIndex            = NO_DESCRIPTOR
    },

in the config and it works under windows 10 without the need for any driver/inf file. I tested with PuTTy (which is an ssh client but it also supports com port communication).

So it should be a no issue then!

aliher1911 commented 3 years ago

The question about new console driver approach, will it still be possible to have control over serial which will not be polluted by debug outputs? Otherwise, whatever is used to interact with keyboard will need to discard debug info (like keyboard booting up) when interacting with it. Even better would be to try to direct console to hid while still maintain control functionality for testing purposes for example.

tmk commented 3 years ago

Nice info about IAD! I added IAD on my branch and Windows 10 can recognize it but debug output is blocked severely for some reason. I overlooked something, I'll look into it.

You can't stop debug outputs on console completely, some of prints using dprintf() can be stopped with debug_enable=false but other prints using xprintf() cannot.

At this point CDC console cannot be enabled with HID console at the same time and there is no way to use CDC console for control purpose only.

You are going to use daemon program to switch layers automatically and response from keyboard should be recognized by the program? It was out of my thought to be honest. I was assuming that user interacts in serial terminal app.

Adding header bytes to its response would be a workaround, this is helpful to discriminate between response and debug prints. I guess this works for small chunk of data such as command response? I don't think the response and debug prints are interlaced char by char virtually.

To download big chunk of data like HEX format of keymap from keyboard requires other way, though. Disable debug outputs temporarily would be useful there. Or other communicaton channel than console is just needed.

tmk / tmk_keyboard

Change keyboard layout or other actions from host #660