-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Kernel/HID: Introduce initial USB keyboard support #24184
base: master
Are you sure you want to change the base?
Conversation
event.key = NumKeyCodes[packet_raw.keypress6].key_code; | ||
event.map_entry_index = NumKeyCodes[packet_raw.keypress6].map_entry_index; | ||
if (packet_raw.keypress6 == 0x54) | ||
event.code_point = '/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just from a cursory glance: you repeat the below pattern a lot of times even though it doesn't matter which one of the four if-branches it takes:
if (packet_raw.keypressX == 0x54)
event.code_point = '/';
You should lift these up out of the branches and put it at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed that
if (!packet_raw.keypress5) | ||
m_key5_pressed = false; | ||
if (!packet_raw.keypress6) | ||
m_key6_pressed = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have a lot of code duplication for these 6 keys. Can you reduce this to a single code block that you execute for each of the 6 keys you track?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed that. Now it is an array and the code iterates through it
|
||
namespace Kernel { | ||
|
||
class USBKeyboardDevice final : public KeyboardDevice { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not fond of this. All other HID drivers (USB mouse, PS/2 keyboard and PS/2 mouse) don't do this.
Please find a way to separate USBKeyboardDevice
from the KeyboardDevice
inheritance tree.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed that, Now it inherits from HIDDevice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not what I meant - look at how the USB mouse driver has a USBMouseDevice
which holds a MouseDevice
:
serenity/Kernel/Devices/HID/USB/MouseDevice.h
Lines 34 to 37 in 4d5823a
USBMouseDevice(USB::Device const&, NonnullOwnPtr<USB::InterruptInPipe> pipe); | |
NonnullOwnPtr<USB::InterruptInPipe> m_interrupt_in_pipe; | |
NonnullRefPtr<USB::Device> m_attached_usb_device; | |
I'd want the same to happen here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I don't quite understand.
It seems to me like the USB Mouse class inherits from MouseDevice
and uses itself to process packets
serenity/Kernel/Devices/HID/USB/MouseDevice.h
Lines 22 to 23 in 4d5823a
class USBMouseDevice final : public MouseDevice { | |
friend class DeviceManagement; |
handle_mouse_packet_input_event(packet); |
While the PS/2 drivers need to have either
KeyboardDevice
or MouseDevice
passed onPS2KeyboardDevice(SerialIOController const&, SerialIOController::PortIndex port_index, ScanCodeSet scan_code_set, KeyboardDevice const&); |
PS2MouseDevice(SerialIOController const&, SerialIOController::PortIndex, MouseDevice const&); |
and use it to process input. They inherit from
SerialIODevice
.If you want to have just a sepreate
KeyboardDevice
. I have fixed that. If you want something else, tell me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I wanted is essentially a replicate of the PS2KeyboardDevice
or PS2MouseDevice
class.
But now that I look at this again, we should keep it the way it was for now, so you should revert to inherit from KeyboardDevice
and to not have a separate device, as it doesn't make sense to both inherit and have the same device class as a member.
What we should aim is to have some sort of model like in the PS/2 keyboard and mouse code, but it will take some time to write the proper modifications, so let's not do this for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, it should be fixed
06a577e
to
29c8a06
Compare
USB_REQUEST_RECIPIENT_DEVICE | USB_REQUEST_TYPE_STANDARD | USB_REQUEST_TRANSFER_DIRECTION_HOST_TO_DEVICE, | ||
USB_REQUEST_SET_CONFIGURATION, configuration.configuration_id(), 0, 0, nullptr)); | ||
|
||
auto const& endpoint_descriptor = interface.endpoints()[0]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we guaranteed to have the mouse device at endpoint 0?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you mean a keyboard device but yes.
The driver checks if the device is a keyboard and if it uses Boot Protocol.
Also endpoint 0 is always a control endpoint and we are guaranteed that it is available.
This looks mostly OK now, although I didn't test this. I'd suggest merging this and extensively testing this before making this device a default of some sort. |
Hello! One or more of the commit messages in this PR do not match the SerenityOS code submission policy, please check the |
It was tested with qemu's
usb-kbd
. The code is based on USB Mouse driver