Skip to content

Conversation

@HongChao6
Copy link
Contributor

Note: Please adhere to Contributing Guidelines.

Summary

Support for the mouse ioctl interface

Impact

mouse driver

Testing

qemu

Signed-off-by: liuhongchao <liuhongchao@xiaomi.com>
Signed-off-by: liuhongchao <liuhongchao@xiaomi.com>
Signed-off-by: liuhongchao <liuhongchao@xiaomi.com>
@github-actions github-actions bot added Area: Drivers Drivers issues Area: File System File System issues Size: M The size of the change in this PR is medium labels Dec 16, 2024
@nuttxpr
Copy link

nuttxpr commented Dec 16, 2024

[Experimental Bot, please feedback here]

No, this PR description does not meet the NuttX requirements. It's far too brief and lacks critical information. Here's why and what's missing:

Summary:

  • Insufficient detail: "Support for the mouse ioctl interface" is vague. What specific ioctl calls are added? What functionality do they provide? What problem does this solve (e.g., was there no way to control the mouse before)?
  • Missing context: Which part of the codebase is affected? Is this a new driver? Modification to an existing one? A new system call?
  • No explanation of how it works: How is the ioctl interface implemented? What are the internal changes?
  • Missing issue references: Are there any related issues this PR addresses?

Impact:

  • Incomplete answers: Saying "mouse driver" doesn't explain the impact. All the "NO / YES" questions need proper answers with explanations if "YES." For example:
    • Impact on user: Will users need to change their applications to use the new ioctl calls?
    • Impact on hardware: Does this only affect certain mouse hardware or architectures?
    • Impact on documentation: Does this require documentation updates? Has documentation been added?

Testing:

  • Inadequate testing description: "qemu" is not enough. What specific qemu configuration was used? What tests were run? What were the expected results?
  • Missing logs: The "Testing logs before change" and "Testing logs after change" sections are empty. These are crucial for demonstrating that the change works and doesn't introduce regressions. Include relevant log output showing the mouse functionality before and after the change.
  • Missing build host details: What operating system, compiler, and versions were used for the build?

Example of a better Summary:

This PR adds support for the MOUSE_SET_SAMPLERATE and MOUSE_GET_STATUS ioctl calls to the existing mouse driver in drivers/input/mouse.c. These ioctls allow user applications to control the mouse sampling rate and retrieve the current mouse status (buttons, position). This addresses the lack of a standardized interface for interacting with the mouse device, as described in [NuttX Issue #123].

Example of a better Impact Section:

  • Is new feature added? YES - Adds ioctl interface for mouse control.
  • Impact on user? YES - Users can now use the new ioctls to control the mouse sampling rate and get status. Applications will need to be updated to use these features.
  • Impact on build? NO
  • Impact on hardware? NO - Works with existing supported mouse hardware.
  • Impact on documentation? YES - Documentation has been added to the doc/drivers/input.txt file describing the new ioctl calls and their usage.
  • Impact on security? NO
  • Impact on compatibility? NO

Example of a better Testing Section:

  • Build Host: Linux (Ubuntu 20.04), x86_64, GCC 9.4.0
  • Target: qemu-system-arm, sim:nsh config

Testing logs before change:

$ nsh
nsh> mouse_test  # No output, as ioctls are not implemented

Testing logs after change:

$ nsh
nsh> mouse_test
Mouse sampling rate set to 100Hz
Mouse status: Left button pressed, X=100, Y=200

By providing more specific details and filling in the missing information, the PR will be much easier to review and understand, increasing the chances of it being accepted.

@acassis acassis requested review from acassis, lupyuen, pkarashchenko and raiden00pl and removed request for acassis December 17, 2024 14:38
@acassis
Copy link
Contributor

acassis commented Dec 17, 2024

@raiden00pl @PetteriAimonen @lupyuen please take a look.

I think since now the user will be able to see and enable the Mouse, Touchscreen and Keyboard we need to have the devices organized as a new menu where the user select which Mouse, Touchscreen and/or Keyboard he/she want to use

@acassis
Copy link
Contributor

acassis commented Dec 25, 2024

@HongChao6 could you please modify to Kconfig to put all Keyboard drivers under "Enable Keyboard Support", all Mouser drivers under "Enable Mouse Support", and so on. These options need to become a menu, simular to "Sensor Support" and others on Kconfig.

@terry0012
Copy link
Contributor

@raiden00pl @PetteriAimonen @lupyuen please take a look.

I think since now the user will be able to see and enable the Mouse, Touchscreen and Keyboard we need to have the devices organized as a new menu where the user select which Mouse, Touchscreen and/or Keyboard he/she want to use

How about change all input drivers from 'select' to 'depends on' ?
In menu code, how to handle the config DRIVERS_VIRTIO_INPUT, it depends on touch & keyboard & mouse as below:
drivers/virtio/Kconfig:51: depends on INPUT && INPUT_TOUCHSCREEN && INPUT_KEYBOARD && INPUT_MOUSE
should we put DRIVERS_VIRTIO_INPUT in each menu (touch & keyboard & mouse) ?

@terry0012
Copy link
Contributor

remove #ifdef/#else around the public struct, macro and function from .h is also other option to solve this problem,
How do you think? @acassis

@xiaoxiang781216
Copy link
Contributor

xiaoxiang781216 commented Jan 11, 2025

remove #ifdef/#else around the public struct, macro and function from .h is also other option to solve this problem, How do you think? @acassis

let's go this direction, @terry0012 .

@acassis
Copy link
Contributor

acassis commented Jan 11, 2025

remove #ifdef/#else around the public struct, macro and function from .h is also other option to solve this problem, How do you think? @acassis

@raiden00pl @PetteriAimonen @lupyuen please take a look.
I think since now the user will be able to see and enable the Mouse, Touchscreen and Keyboard we need to have the devices organized as a new menu where the user select which Mouse, Touchscreen and/or Keyboard he/she want to use

How about change all input drivers from 'select' to 'depends on' ? In menu code, how to handle the config DRIVERS_VIRTIO_INPUT, it depends on touch & keyboard & mouse as below: drivers/virtio/Kconfig:51: depends on INPUT && INPUT_TOUCHSCREEN && INPUT_KEYBOARD && INPUT_MOUSE should we put DRIVERS_VIRTIO_INPUT in each menu (touch & keyboard & mouse) ?

Yes, this modification will make more sense. A bad thing about "depends on" is because it disappears with the driver from menuconfig. When the drivers are in a fixed subcategory (like Drivers ->Sensors) it is fine, but for INPUT it will be spread around many places, so the user never will know the RPMSG has some KEYBOARD / MOUSE features unless the respective INPUT be enabled first in another place

@terry0012
Copy link
Contributor

terry0012 commented Jan 11, 2025

@HongChao6 Let's revert the changes in Kconfig.
And remove #ifdef/#else around the public struct, macro and function in mouse.h & touchscreen.h & keyboard.h to make sure rpmsgdev works well.
Thanks.

Copy link
Contributor

@acassis acassis left a comment

Choose a reason for hiding this comment

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

Please fix the commit title: nuxxt -> nuttx

Signed-off-by: liuhongchao <liuhongchao@xiaomi.com>
@HongChao6
Copy link
Contributor Author

Please fix the commit title: nuxxt -> nuttx

done.

@xiaoxiang781216 xiaoxiang781216 merged commit 1e2c002 into apache:master Jan 17, 2025
39 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: Drivers Drivers issues Area: File System File System issues Size: M The size of the change in this PR is medium

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants