boards/linum-stm32h753bi: Support NXDoom - #19579
Conversation
github-actions
Bot
added
Arch: arm
|
| uint8_t lastkey[6]; /* Keys down in the previous report */ | ||
| #endif | ||
| #ifdef CONFIG_HIDKBD_KBDUPPER | ||
| struct keyboard_lowerhalf_s lower; /* Keyboard upper-half interface */ |
There was a problem hiding this comment.
why not always use keyboard_lowerhalf_s to simplify the code base?
There was a problem hiding this comment.
Just to be sure I follow: do you mean dropping CONFIG_HIDKBD_KBDUPPER and always registering through the keyboard upper half, so there is a single code path here?
If that is it, I am fine with it, and there is already a case for it in the tree: the USB branch in examples/lvglterm (171 lines of kbd_decode()) exists only because this driver speaks a byte stream, on top of an upper half path that already worked for a matrix keyboard.
My only concern is what it does to the existing users. Making it unconditional changes what read() on /dev/kbd[n] returns, and 9 in-tree configurations still use examples/hidkbd, which decodes the byte stream:
ci20:jumbo qemu-intel64:jumbo
nucleo-h743zi2:jumbo sama5d3-xplained:bluetooth
nucleo-h753zi:jumbo stm32butterfly2:nsh
olimex-lpc1766stk:hidkbd stm32butterfly2:nshusbhost
olimex-stm32-p407:hidkbd
They would still build, but read struct keyboard_event_s into a char buffer, so the breakage would be silent rather than a CI failure.
Is that acceptable to you, or would you rather I move those configurations to examples/keyboard in nuttx-apps first and change this driver afterwards?
There was a problem hiding this comment.
Just to be sure I follow: do you mean dropping CONFIG_HIDKBD_KBDUPPER and always registering through the keyboard upper half, so there is a single code path here?
Yes.
If that is it, I am fine with it, and there is already a case for it in the tree: the USB branch in examples/lvglterm (171 lines of kbd_decode()) exists only because this driver speaks a byte stream, on top of an upper half path that already worked for a matrix keyboard.
My only concern is what it does to the existing users. Making it unconditional changes what read() on /dev/kbd[n] returns, and 9 in-tree configurations still use examples/hidkbd, which decodes the byte stream:
we can add a new option to support the byte stream in upper layer for compatibility.
ci20:jumbo qemu-intel64:jumbo nucleo-h743zi2:jumbo sama5d3-xplained:bluetooth nucleo-h753zi:jumbo stm32butterfly2:nsh olimex-lpc1766stk:hidkbd stm32butterfly2:nshusbhost olimex-stm32-p407:hidkbdThey would still build, but read struct keyboard_event_s into a char buffer, so the breakage would be silent rather than a CI failure.
Is that acceptable to you, or would you rather I move those configurations to examples/keyboard in nuttx-apps first and change this driver afterwards?
it's better to update the upstream code to use the standard method.
There was a problem hiding this comment.
I think it is important to discuss with @linguini1 and @ppisa here, since they also are working to fix the INPUT encoding issue on NuttX. Since like each person are working in a direction without clear coordination, so a fix from one person it an issue for other.
There was a problem hiding this comment.
Agreed, better to coordinate.
My idea was to create a system/kbd in a separate PR, merging what examples/hidkbd and examples/keyboard do today, so examples/lvglterm and NXDoom use the same thing instead of each one handling the keyboard its own way.
Until that is agreed, I will put this PR on hold, so I do not add a third variant while the encoding work is in progress.
@linguini1 @ppisa, does that fit what you have in mind?
There was a problem hiding this comment.
Don't let me stop you! I have not invested any proper time in the keyboard stuff yet since I've been focusing on GSoC milestones. I will jump in later in the future and get up to date with your changes then.
There was a problem hiding this comment.
I am not sure what is the best option. Some system level KBD which aggregates inputs from all others can be useful. The mapping which lower level keyboards streams should aggregated into system level one should be somehow configurable which adds complexity. Linux input event is quite powerful, I have implemented remapping of numeric keyboard (with some Etneter, cursors and F-keys) as T-9 which output has been seen by application as the regular alphanumeric keyboard long time ago on Linux on our iMX1 based terminal. But it is necessary to consider complexity, because NuttX has man value on smaller system, where thinks has to be kept simple and memory efficient.
Another problem is, if the keyboards should deliver keys pressures as ASCII codes or if all should be scancodes specifying location on physical keyboard only. The second approach allows national keyboards variants but requires system of keymaps which are somehow chosen according to locales and geometry reported by keyboard. So may it be using ASCII standardizes this at least a little and you see something understandable at the start and simple keyboards can live with this and list of special keys. More complex applications can use modifies and national ones needs to do full remapping...
really, I am not sure what is the best approach for NuttX so I do not want to hold the effort. I think that fixed even format is better for now than that complex ESC based stream. But even this is not strong pinion.
enum kbd_keycode_e had no way to name a modifier key, so a driver that can tell that Ctrl, Shift, Alt or GUI was pressed had nothing to report it as, and an application had no way to bind an action to one or to know that one is being held. Append the eight modifiers to the end of the enumeration, which leaves the value of every existing keycode unchanged. Signed-off-by: Jorge Guzman <jorge.gzm@gmail.com>
The HID keyboard driver registers /dev/kbd[n] as a byte stream and only ever reports key presses: there is no way for it to say that a key was released. That is enough to type with, but not for an application that has to know how long a key is held down, such as a game, where a movement key would never stop being pressed. Add CONFIG_HIDKBD_KBDUPPER, which registers the device through the keyboard upper-half driver instead, so that reads return struct keyboard_event_s. Presses and releases are derived by comparing each HID report against the previous one, which is the only way to tell them apart given that the specification gives no significance to the order of the keycodes in a report. Modifiers are reported as keys of their own, so an application can see that Ctrl is down and which key it is held with. The two interfaces are mutually exclusive because they share the device name, so the byte stream is not built in this mode. Signed-off-by: Jorge Guzman <jorge.gzm@gmail.com>
chromakey is a uint32_t, so printing it with %d warns. The line is only compiled when CONFIG_STM32_FB_CMAP is enabled, which is why it has gone unnoticed. Signed-off-by: Jorge Guzman <jorge.gzm@gmail.com>
7c199ca to
594e58f
Compare
Runs NXDoom on the board's LCD, played with a USB HID keyboard and reading the game data from the microSD card, so it exercises the LTDC framebuffer, the OTG FS host and the SDMMC peripheral at once. Three settings are needed that are not obvious: CONFIG_FAT_FORCE_INDIRECT, because the FAT layer otherwise reads whole sectors straight into the caller's buffer and the SDMMC IDMA cannot reach the caller's buffer when it lives in external SDRAM. The failure appears part way through startup, once the internal RAM has filled and allocations start coming from SDRAM. CONFIG_HIDKBD_NOGETREPORT, because the keyboard answers GET_REPORT on the control pipe with an empty report and only delivers key data on its interrupt endpoint. Without it the keyboard enumerates, reports no error, and no key is ever seen. CONFIG_STM32_LTDC_L1_L8 with the frame buffer colour map, because DOOM is natively palettised: letting the display convert the palette while it scans out removes the conversion from the blit and halves the amount of data written per frame. Signed-off-by: Jorge Guzman <jorge.gzm@gmail.com>
The options that let a board trade memory for drawing speed were only described in their Kconfig help, where someone bringing the game up on a new board is unlikely to find them. Describe what each one does and when it is worth enabling. Signed-off-by: Jorge Guzman <jorge.gzm@gmail.com>
594e58f to
0d102f1
Compare
| KEYCODE_RALT, /* Right Alt */ | ||
| KEYCODE_LGUI, /* Left GUI (Windows/Command) */ | ||
| KEYCODE_RGUI /* Right GUI (Windows/Command) */ | ||
| }; |
There was a problem hiding this comment.
ACK, this would help Microwindows as well, note to @Acfboy
Then when modifiers are delivered then they can be applied even in Microwindows event keyboard alternative handling.
Translation to modifiers keys should be added to https://github.com/apache/nuttx/blob/master/drivers/input/virtio_key_decode.c which solves this for VirtIO KBD and Goldfish one.
|
I am happy that USB HID keyboard allows mode with event style delivery. I think it is better than that complex decode of the stream with ESC characters. I would even agree if this even mode is default for USB HID KBD. |
6 participants
Summary
Adds a configuration that runs NXDoom on the board's LCD, played with a USB HID
keyboard and reading the game data from the microSD card, so it exercises the
LTDC frame buffer, the OTG FS host and the SDMMC peripheral at once.
Three settings are needed that are not obvious, and each one is documented in
the board page with the symptom it cures, because each cost a while to find:
CONFIG_FAT_FORCE_INDIRECT: the FAT layer otherwise reads whole sectorsstraight into the caller's buffer, and the SDMMC IDMA cannot reach that
buffer when it lives in external SDRAM. The failure appears part way through
startup, once internal RAM has filled and allocations start coming from
SDRAM, and looks like a truncated file:
w_read_lump: only read 0 of 5192 on lump 1070.CONFIG_HIDKBD_NOGETREPORT(withCONFIG_USBHOST_ASYNCH) : the keyboardaccepts GET_REPORT on the control pipe but always answers with an empty
report, and only delivers key data on its interrupt endpoint. The symptom is
a keyboard that enumerates as
/dev/kbda, reports no error at all, and neverproduces a key.
CONFIG_STM32_LTDC_L1_L8with the frame buffer colour map, DOOM is nativelypalettised, so letting the display convert the palette while it scans out
removes the conversion from the blit and halves the amount of data written
per frame.
The zmodem commands are enabled as well, so the IWAD can be copied over the
serial console when no card reader is at hand. Note that the board's existing
zmodemconfiguration uses CDCACM, which cannot be used here because the OTGFS port is in host mode for the keyboard; the transfer goes over USART1
instead.
Also included:
arch/arm/src/stm32h7: fix LTDC format specifier for the CLUT index,chromakeyis auint32_tand was printed with%d. The line is onlycompiled when
CONFIG_STM32_FB_CMAPis enabled, which is why it had goneunnoticed; this configuration is the first to enable it.
Documentation/applications/games/nxdoom: document the display options, theoptions added by PR B were only described in their Kconfig help, where
someone bringing the game up on a new board is unlikely to find them.
Impact
nxdoomsection; the NXDoomapplication page gains a section on the display options.
lvgl,lvglterm,lvglterm_kbda) are untouched and keep RGB565; each has its own defconfig.Testing
Host: Linux x86_64,
arm-none-eabi-gcc.Board: LINUM-STM32H753BI with the 1024x600 LCD, a USB HID keyboard, and a
microSD card holding
doom1.wad.Built warning-free;
nxstylereports 0 warnings on the changed source file.The board's other LTDC configurations were rebuilt to confirm they are
unaffected.
Game running on the board:
The game plays from the USB keyboard and runs stably; it was left running for
minutes at a time without an assertion. Available memory before starting,
showing the 6 MiB contiguous SDRAM region the game's zone comes from:
This PR is being tested with PR apache/nuttx-apps#3681