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

Feat/expand getdeviceinfo #499

Merged
merged 39 commits into from
May 5, 2023
Merged

Conversation

Julusian
Copy link
Contributor

@Julusian Julusian commented Mar 8, 2023

Closes #132

This expands on #474 with some new functionality. I thought it better to do as a separate PR to avoid making #474 even larger.

Only the last couple of commits are relevant here, once #474 is merged the changes involved here will become a lot clearer.

This update hidapi to 0.13.1, and uses the new hid_get_device_info method to add more properties to the getDeviceInfo methods. getDeviceInfo will now return the same content as is provided by a call to HID.devices().

My usecase for this is that in https://github.com/Julusian/node-elgato-stream-deck I need to know what model of streamdeck a given path corresponds to. Until now I have achieved this by making a call to HID.devices(), and finding the entry with a matching path to find the productId and vendorId. This is unsurprisingly not at all efficient as it requires enumerating devices every time a new device is opened.
Instead this will let me open the device, and retrieve the ids directly from it.

@todbot I vaguely remember reading something about concerns or things that need testing with updating hidapi. Do you know of anything that is particularly at risk of being an issue? On linux (hidraw) it was a drop in replacement that appears to work fine without additional changes.

Until this is merged it is possible to test with https://www.npmjs.com/package/@julusian/hid. You can either import it explicitly, or with "node-hid": "npm:@julusian/hid@^3.0.0-0", to alias it to the node-hid name. This includes the changes from #474, #490 and #499.

@todbot
Copy link
Contributor

todbot commented May 5, 2023

I ended up getting compile warnings deep in the new code, so I reverted this PR. Apologies, I shouldn't have merged it without better testing. I'm looking into why gh pr checkout was giving me different results.

The warnings I was seeing on macos and node v16.20.0:

  CC(target) Release/obj.target/hidapi/hidapi/mac/hid.o
  LIBTOOL-STATIC Release/hidapi.a
  CXX(target) Release/obj.target/HID/src/exports.o
  CXX(target) Release/obj.target/HID/src/HID.o
  CXX(target) Release/obj.target/HID/src/HIDAsync.o
In file included from ../src/HIDAsync.cc:27:
In file included from ../src/devices.h:1:
../src/util.h:112:25: warning: 'PromiseAsyncWorker<std::shared_ptr<DeviceContext>>::GetResult' hides overloaded virtual function [-Woverloaded-virtual]
    virtual Napi::Value GetResult(const Napi::Env &env) = 0;
                        ^
../src/HIDAsync.cc:55:28: note: in instantiation of template class 'PromiseAsyncWorker<std::shared_ptr<DeviceContext>>' requested here
class CloseWorker : public PromiseAsyncWorker<std::shared_ptr<DeviceContext>>
                           ^
/Users/tod/projects/node/node-hid/node_modules/node-addon-api/napi.h:2117:37: note: hidden overloaded virtual function 'Napi::AsyncWorker::GetResult' declared here: type mismatch at 1st parameter ('Napi::Env' vs 'const Napi::Env &')
    virtual std::vector<napi_value> GetResult(Napi::Env env);
                                    ^
In file included from ../src/HIDAsync.cc:27:
In file included from ../src/devices.h:1:
../src/util.h:112:25: warning: 'PromiseAsyncWorker<ContextState *>::GetResult' hides overloaded virtual function [-Woverloaded-virtual]
    virtual Napi::Value GetResult(const Napi::Env &env) = 0;
                        ^
../src/HIDAsync.cc:92:33: note: in instantiation of template class 'PromiseAsyncWorker<ContextState *>' requested here
class OpenByPathWorker : public PromiseAsyncWorker<ContextState *>
                                ^
/Users/tod/projects/node/node-hid/node_modules/node-addon-api/napi.h:2117:37: note: hidden overloaded virtual function 'Napi::AsyncWorker::GetResult' declared here: type mismatch at 1st parameter ('Napi::Env' vs 'const Napi::Env &')
    virtual std::vector<napi_value> GetResult(Napi::Env env);
                                    ^
../src/HIDAsync.cc:417:7: warning: private field 'written' is not used [-Wunused-private-field]
  int written = 0;
      ^
3 warnings generated.
  CXX(target) Release/obj.target/HID/src/devices.o
In file included from ../src/devices.cc:1:
In file included from ../src/devices.h:1:
../src/util.h:112:25: warning: 'PromiseAsyncWorker<ContextState *>::GetResult' hides overloaded virtual function [-Woverloaded-virtual]
    virtual Napi::Value GetResult(const Napi::Env &env) = 0;
                        ^
../src/devices.cc:91:30: note: in instantiation of template class 'PromiseAsyncWorker<ContextState *>' requested here
class DevicesWorker : public PromiseAsyncWorker<ContextState *>
                             ^
/Users/tod/projects/node/node-hid/node_modules/node-addon-api/napi.h:2117:37: note: hidden overloaded virtual function 'Napi::AsyncWorker::GetResult' declared here: type mismatch at 1st parameter ('Napi::Env' vs 'const Napi::Env &')
    virtual std::vector<napi_value> GetResult(Napi::Env env);
                                    ^
1 warning generated.
  CXX(target) Release/obj.target/HID/src/read.o
  CXX(target) Release/obj.target/HID/src/util.o
  SOLINK_MODULE(target) Release/HID.node
gyp info ok

@Julusian
Copy link
Contributor Author

Julusian commented May 5, 2023

OK, Ill take a look into that now.
strangely, I don't remember seeing any warnings anywhere, but I may not have checked the build logs for macos

If you can hold off on doing a release until this is fixed and ideally #490 too that would be great. I think #490 could do with some additional changes, I shall open a new version of it for those

@todbot
Copy link
Contributor

todbot commented May 5, 2023

Thanks. And apologies for being away from this for so long. The #490 one is tricky because of multi-arch builds. I'll leave this one for now and try testing it.

@todbot
Copy link
Contributor

todbot commented May 5, 2023

Also, another issue with this PR (that's brought up elsewhere) is the need to update to node-gyp@latest for this PR. I had done that for other reasons and didn't see the issue until I had done a fresh checkout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Strings like the product string should be available on an opened device
2 participants