Skip to content

Misc cleanup for src/joystick/hidapi/SDL_report_descriptor.c and new test program#15959

Open
ao2 wants to merge 7 commits into
libsdl-org:mainfrom
ao2:wip/ao2/task957-prepare
Open

Misc cleanup for src/joystick/hidapi/SDL_report_descriptor.c and new test program#15959
ao2 wants to merge 7 commits into
libsdl-org:mainfrom
ao2:wip/ao2/task957-prepare

Conversation

@ao2

@ao2 ao2 commented Jul 8, 2026

Copy link
Copy Markdown

Hi,

here are some cleanups and improvements for src/joystick/hidapi/SDL_report_descriptor.c in preparation for future changes.

cc @smcv

@ao2

ao2 commented Jul 8, 2026

Copy link
Copy Markdown
Author

I took the SDL_DYNAMIC_API hack in 992d47d from test/testevdev.c, but apparently this breaks build with msbuild (https://github.com/ao2/SDL/actions/runs/28957779597/job/85921577553#step:44:401) with the following error:

 (ClCompile target) -> 
         D:\a\SDL\SDL\src\joystick\hidapi\SDL_report_descriptor.c(31,9): error C2220: the following warning is treated as an error [D:\a\SDL\SDL\VisualC\SDL\SDL.vcxproj]
         D:\a\SDL\SDL\src\joystick\hidapi\SDL_report_descriptor.c(47,1): error C1020: unexpected #endif [D:\a\SDL\SDL\VisualC\SDL\SDL.vcxproj]

And I have no idea how to solve it.

@slouken slouken added this to the 3.x milestone Jul 8, 2026
@ao2 ao2 force-pushed the wip/ao2/task957-prepare branch 2 times, most recently from 037f2f5 to a84e078 Compare July 9, 2026 11:25
@ao2

ao2 commented Jul 9, 2026

Copy link
Copy Markdown
Author

The new version fixes, or rather avoids, the problems with msbuild.

This should be ready for review.

@ao2 ao2 force-pushed the wip/ao2/task957-prepare branch from a84e078 to 937b67b Compare July 9, 2026 11:43
slouken
slouken reviewed Jul 9, 2026
View reviewed changes
Comment thread src/joystick/hidapi/SDL_report_descriptor.c Outdated
slouken
slouken reviewed Jul 9, 2026
View reviewed changes
Comment thread src/joystick/hidapi/SDL_report_descriptor.c Outdated
slouken
slouken reviewed Jul 9, 2026
View reviewed changes
Comment thread CMakeLists.txt Outdated
dep_option(SDL_TESTS_LINK_SHARED "link tests to shared SDL library" "${SDL_SHARED}" "SDL_SHARED;SDL_STATIC" "${SDL_SHARED}")
set(SDL_TESTS_TIMEOUT_MULTIPLIER "1" CACHE STRING "Timeout multiplier to account for really slow machines")

dep_option(SDL_REPORT_DESCRIPTOR_EXE "Build SDL_report_descriptor as an executable" OFF "SDL_TESTS" OFF)

@slouken slouken Jul 9, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm wondering if this should be "testdescriptor" and just be part of the normally built tests?

@ao2 ao2 Jul 10, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

That would be fine by me, and I addressed this in the latest version.

Just noticing that the test is meant to accept a command line argument and will exit with a non-zero value if no arguments are passed.

But I guess we are good if this is not added by default to the automated test suite?

@slouken slouken Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Just noticing that the test is meant to accept a command line argument and will exit with a non-zero value if no arguments are passed.

But I guess we are good if this is not added by default to the automated test suite?

Yeah, that's fine.

ao2 added 2 commits July 10, 2026 13:23
@ao2
testevdev: Always use decimal for HID report descriptors size
6c4f79a 
The size of the HID report descriptors in the SDL_COMPILE_TIME_ASSERT
checks is specified sometimes in decimal and sometimes in octal.

Always used decimal for consistency.

Signed-off-by: Antonio Ospite <antonio.ospite@collabora.com>
@ao2
SDL_report_descriptor: Add missing include guards in header file
8f8136e 
Signed-off-by: Antonio Ospite <antonio.ospite@collabora.com>
@ao2 ao2 force-pushed the wip/ao2/task957-prepare branch from 937b67b to 6c99c26 Compare July 10, 2026 11:23
ao2 added 5 commits July 10, 2026 13:51
@ao2
SDL_report_descriptor: Fix some ignored return values
e0f82fc 
Fix some ignored return values resulting in incomplete error checking.

For instance:

  - ParseMainItem() was always returning true because the return value of
    AddInputFields() was not handled.

  - ParseLocalItem() was always returning true because the return value of
    AddUsage() was not handled.

Signed-off-by: Antonio Ospite <antonio.ospite@collabora.com>
@ao2
test: Add new test program for SDL_report_descriptor code
5abe560 
Add support for testing the SDL_report_descriptor code by adding a new
executable to make it easier to experiment with HID report descriptor
parsing.

The main() function which originally was in
src/joystick/hidapi/SDL_report_descriptor.c is moved to a separate file
named test/testdescriptor.c in order to simplify dealing with multiple
build systems.

For instance if the main() was in
src/joystick/hidapi/SDL_report_descriptor.c the code would have to
account for the cases when the file is built as part of the library or
when it's built as part of a standalone executable, in which case the
dynamic API mechanism needs to be disabled.

This would pose a problem especially with MSVC which uses pre-compiled
headers (PCH) which do not sit well with the preprocessor logic that
would be needed, like the hacks added in test/testevdev.c

Using a completely separate source file for the standalone executable
avoids the issue, especially considering that the new file is not built
with MSVC at all.

The new test program can be built with:

  cmake -DCMAKE_BUILD_TYPE=Debug -DSDL_TESTS=ON -S . -B build
  make -C testdescriptor

Signed-off-by: Antonio Ospite <antonio.ospite@collabora.com>
@ao2
testdescriptor: Fix compiler warning
3aa395b 
Fix compiler warning:

-----------------------------------------------------------------------
.../test/testdescriptor.c: In function ‘main’:
.../test/testdescriptor.c:56:17: warning: unused variable ‘file’ [-Wunused-variable]
   56 |     const char *file = argv[1];
      |                 ^~~~

-----------------------------------------------------------------------

Use the `file` variable when appropriate since it is already declared
and assigned.

Signed-off-by: Antonio Ospite <antonio.ospite@collabora.com>
@ao2
testdescriptor: Clean up descriptor context before exiting
e61f2db 
For correctness clean up the descriptor parsing context before exiting,
even though this is not a problem in practice for a standalone
executable.

Signed-off-by: Antonio Ospite <antonio.ospite@collabora.com>
@ao2
testdescriptor: Exercise public API for parsing report descriptor
3befd1d 
This gives some more coverage when running the SDL_report_descriptor
test program.

Signed-off-by: Antonio Ospite <antonio.ospite@collabora.com>
@ao2 ao2 force-pushed the wip/ao2/task957-prepare branch from 6c99c26 to 3befd1d Compare July 10, 2026 11:54
@ao2 ao2 changed the title Misc cleanup and improvements for src/joystick/hidapi/SDL_report_descriptor.c Misc cleanup for src/joystick/hidapi/SDL_report_descriptor.c and new test program Jul 10, 2026
@ao2

ao2 commented Jul 10, 2026 •
edited
Loading

Copy link
Copy Markdown
Author

Building testdescriptor on Windows x64 with CMake results in the following error:

D:\a\SDL\SDL\test\testdescriptor.c(69): error C2220: the following warning is treated as an error
D:\a\SDL\SDL\test\testdescriptor.c(69): warning C4267: 'function': conversion from 'size_t' to 'int', possible loss of data

See https://github.com/ao2/SDL/actions/runs/29090880925/job/86399106802#step:33:741

This is because the code uses SDL_LoadFile to load the descriptor, which uses a size_t variable to store the data size, while SDL_ParseReportDescriptor accepts the descriptor size as an int variable.

Maybe SDL_ParseReportDescriptor() should have been declared with a size_t parameter in the first place, can this be changed or is it public API?

@slouken

slouken commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Maybe SDL_ParseReportDescriptor() should have been declared with a size_t parameter in the first place, can this be changed or is it public API?

It's not a public API, feel free to change it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@slouken slouken slouken left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.x

Development

Successfully merging this pull request may close these issues.

2 participants

@ao2 @slouken