Skip to content
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

Drivers/input #14790

Merged
merged 4 commits into from
Nov 14, 2024
Merged

Drivers/input #14790

merged 4 commits into from
Nov 14, 2024

Conversation

fangpeina
Copy link
Contributor

Note: Please adhere to Contributing Guidelines.

Summary

Related modifications to drivers/inputs:

  1. Extend the ioctl interface of the force feedback framework to support factory calibration of force feedback devices.
  2. 86225 driver supports simulating RTP playback mode using RAMLOOP mode. For resource-constrained devices,
    simulate RTP playback effects using preset custom RAMLOOP combinations instead of using RTP files to play
    custom vibration effects.
  3. Fix RTP mode work error and compliation warn in aw86225.

Impact

drivers/input/ff_upper, drivers/input/aw86225

Testing

Self testing on actual devices

For resource-constrained devices, simulate RTP
playback effects using preset custom RAMLOOP
combinations instead of using RTP files to play
custom vibration effects.

Signed-off-by: fangpeina <[email protected]>
input/aw86225.c: In function 'aw86225_ram_work_routine':
input/aw86225.c:1976:27: warning: 'rtp_file.data' may be used uninitialized [-Wmaybe-uninitialized]
 1976 |   struct aw86225_firmware rtp_file;
      |                           ^~~~~~~~
In function 'aw86225_ram_loaded',
    inlined from 'aw86225_ram_work_routine' at input/aw86225.c:1982:3:
input/aw86225.c:1445:17: warning: 'rtp_file.size' may be used uninitialized [-Wmaybe-uninitialized]
 1445 |   for (i = 2; i < cont->size; i++)
      |               ~~^~~~~~~~~~~~
input/aw86225.c: In function 'aw86225_ram_work_routine':
input/aw86225.c:1976:27: note: 'rtp_file.size' was declared here
 1976 |   struct aw86225_firmware rtp_file;

Signed-off-by: fangpeina <[email protected]>
@github-actions github-actions bot added Area: Drivers Drivers issues Size: M The size of the change in this PR is medium labels Nov 14, 2024
@nuttxpr
Copy link

nuttxpr commented Nov 14, 2024

[Experimental Bot, please feedback here]

No, this PR summary does not fully meet the NuttX requirements. While it provides a decent overview of the changes, it lacks crucial details required by the template.

Here's a breakdown of missing information:

  • Summary:

    • Missing "Why": The summary describes what changed but not why. Why is factory calibration needed? Why simulate RTP playback? Why was the previous RTP mode broken? These are crucial for reviewers to understand the motivation and value of the changes.
    • Missing "How (exactly)": The summary provides a high-level overview of what changed but not how. What specific ioctl commands were added? How does the RAMLOOP simulation work in detail? What was the specific bug fix for the RTP mode? More technical detail is needed.
    • Missing Issue References: Are there related NuttX issues? These should be linked.
  • Impact:

    • Incomplete Sections: The impact section simply lists affected directories. It needs to address all the points in the template with "YES" or "NO" and explanations where needed. For example:
      • Impact on user: Will users need to change their application code to use the new ioctl commands?
      • Impact on build: Are there any new Kconfig options?
      • Impact on hardware: Are there specific hardware requirements for these changes?
      • Impact on documentation: Does the documentation need updating? Has it been updated?
      • Impact on security: Are there any potential security implications of these changes?
      • Impact on compatibility: Are these changes backward compatible?
    • Missing Detail: Even where impact exists (e.g., drivers/input), the description needs more detail. How are these drivers impacted?
  • Testing:

    • Insufficient Detail: "Self testing on actual devices" is too vague. The template requires specific details about the host and target environments. What OS, architecture, board, and configuration were used?
    • Missing Logs: The template requires "before" and "after" testing logs. These logs are critical for demonstrating that the changes work as intended and don't introduce regressions.

In short, the PR needs to be significantly more detailed to meet the NuttX requirements. It should meticulously fill out all sections of the provided template, providing specific and comprehensive information for each point.

@acassis
Copy link
Contributor

acassis commented Nov 14, 2024

@fangpeina please include a board support to this driver and some Documentation/ about it. I noticed we don't have reference to Haptic: https://nuttx.apache.org/docs/latest/search.html?q=haptic&check_keywords=yes&area=default#

Also I think we don't have any documentation about the Input Subsystem

@xiaoxiang781216 xiaoxiang781216 merged commit 66976c4 into apache:master Nov 14, 2024
53 of 54 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 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.

4 participants