Ported code from SetupApi to CfgMgr32 #362

2021-12-23T06:29:11-06:00

DJm00n commented

2021-12-23 06:29:11 -06:00

(Migrated from github.com)

Ported code from SetupApi to CfgMgr32. See: https://docs.microsoft.com/windows-hardware/drivers/install/porting-from-setupapi-to-cfgmgr32
Use Unicode Windows API internally where possible. Windows operates natively in UTF-16 (WCHAR) so do we too.

- Ported code from SetupApi to CfgMgr32. See: https://docs.microsoft.com/windows-hardware/drivers/install/porting-from-setupapi-to-cfgmgr32 - Use Unicode Windows API internally where possible. Windows operates natively in UTF-16 (WCHAR) so do we too.

Youw (Migrated from github.com) reviewed

2021-12-24 00:55:31 -06:00

Youw (Migrated from github.com) left a comment

Once you've removed dependency on SetupAPI - we should remove link dependency on it too: from CMakeLists, autotools and vsproj files

windows/hid.c

					
				@ -627,1 +619,4 @@

					CONFIGRET cr;

					wchar_t* device_interface_list = NULL;

					DWORD len;

Sinse we are refactoring - lets get rid of this comment

Youw commented

2021-12-24 00:56:34 -06:00

(Migrated from github.com)

This is nice refactoring, thanks.

❤️ 1

Youw (Migrated from github.com) reviewed

2021-12-29 09:52:47 -06:00

windows/hid.c

None of it is relevant to the topic of the PR.
Please revert all the white-space changes and shuffling around of the order of declarations.

It makes unnecessary noise in the git history, which I often need to consult.

None of it is relevant to the topic of the PR. Please revert all the white-space changes and shuffling around of the order of declarations. It makes _unnecessary noise in the git history_, which I often need to consult.

Youw (Migrated from github.com) reviewed

2021-12-29 10:08:35 -06:00

Youw (Migrated from github.com) left a comment

Remove all of the link dependencies on cfgmgr32 and hid from all of build scripts.

Remove all of the link dependencies on `cfgmgr32` and `hid` from all of build scripts.

windows/CMakeLists.txt

					
				@ -4,7 +4,6 @@ add_library(hidapi_winapi

				)

And with this change you will break the build for everywone who don't have WDK installed (which is a large amount of users).

To have a support of DDK builds, it has to be done properly, e.g. a CMake option, and link against hid.lib only when the user explicitly needs to.

And with this change you will break the build for everywone who don't have WDK installed (which is a large amount of users). To have a support of DDK builds, it has to be done properly, e.g. a CMake option, and link against `hid.lib` only when the user explicitly needs to.

windows/hid.c

					
				@ -21,11 +21,17 @@

				********************************************************/

whese are removed, not moved - is that mean we don't use any of stdlib.h/string.h anymore?

whese are removed, not moved - is that mean we don't use any of `stdlib.h`/`string.h` anymore?

Youw (Migrated from github.com) reviewed

2021-12-29 10:10:06 -06:00

windows/CMakeLists.txt

					
				@ -4,7 +4,6 @@ add_library(hidapi_winapi

				)

Just to be clear: DDK support is out of scope of this PR.
And it would require a lot more than I've mentioned in the previous comment.

Just to be clear: DDK support is out of scope of this PR. And it would require a lot more than I've mentioned in the previous comment.

DJm00n (Migrated from github.com) reviewed

2021-12-29 11:24:48 -06:00

windows/hid.c

alright

DJm00n (Migrated from github.com) reviewed

2021-12-29 11:25:41 -06:00

windows/hid.c

					
				@ -21,11 +21,17 @@

				********************************************************/

it seems. or these are included by some other headers. not sure how to check.

DJm00n (Migrated from github.com) reviewed

2021-12-29 11:52:26 -06:00

windows/CMakeLists.txt

					
				@ -4,7 +4,6 @@ add_library(hidapi_winapi

				)

Alright. I'll do another PR for that.

Youw (Migrated from github.com) reviewed

2021-12-29 15:53:45 -06:00

windows/hid.c

					
				@ -21,11 +21,17 @@

				********************************************************/

or these are included by some other headers

That is a time bomb. I've had a failed builds after a compiler/library upgrade, because some of the headers where implicitly included where they shouldn't.

> or these are included by some other headers That is a time bomb. I've had a failed builds after a compiler/library upgrade, because some of the headers where implicitly included where they shouldn't.

Youw (Migrated from github.com) reviewed

2022-01-08 04:33:13 -06:00

Youw (Migrated from github.com) left a comment

except for the potential leak - looks good

windows/hid.c

					
				@ -588,136 +589,91 @@ static struct hid_device_info *hid_get_device_info(const char *path, HANDLE hand

					}

device_interface_list leak here

`device_interface_list` leak here

windows/hid.c

check for non-NULL return

check for non-`NULL` return

Youw (Migrated from github.com) approved these changes

2022-01-09 06:33:04 -06:00

Sunbreak commented

2022-03-29 01:18:27 -05:00

(Migrated from github.com)

A little curious why porting SetupApi to CfgMgr32?

UWP will continue to be supported with "bug, reliability, and security fixes", but Windows App SDK is the "single path forward"

https://github.com/flutter/flutter/issues/14967#issuecomment-1033337989

A little curious why porting SetupApi to CfgMgr32? > UWP will continue to be supported with "bug, reliability, and security fixes", but Windows App SDK is the "single path forward" https://github.com/flutter/flutter/issues/14967#issuecomment-1033337989

Sign in to join this conversation.

No reviewers

Youw