Skip to content

issue-122: use v8:Buffer in write() and sendFeatureReport() - #171

Closed
amadsen wants to merge 2 commits into
node-hid:masterfrom
amadsen:node-hid-122
Closed

amadsen wants to merge 2 commits into
node-hid:masterfrom
amadsen:node-hid-122

Conversation

@amadsen

@amadsen amadsen commented Nov 23, 2016

Copy link
Copy Markdown

This code still has commented out code that should no longer be needed and extra logging where I was debugging what was going on. It appears to work when writing to my device but it needs testing writing Arrays in hid::write and hid::sendFeatureReport. Submitting the pull request now for testing and code review.

Comment thread nodehid.js
this[i] = binding.HID.prototype[i].bind(this._raw);
for (var i in binding.HID.prototype) {
if (!HID.prototype[i]) {
this[i] = (function(fn){

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.

This is effectively the same as the previous code using .bind(), except that it allowed me to log arguments being passed. It isn't needed unless we value that logging - which should probably be behind some sort of flag.

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.

@todbot An advantage of the test on line 37 is it uses the existence of a method on the javascript prototype as an indicator for whether it should attach the native method to the constructed object - meaning you don't have to keep adding exclusions as you add facades for methods (as seen with 'open' in #178)

Comment thread nodehid.js
throw new Error('Unable to read from a closed HID device');
} else {
return this._raw.read(callback);
}

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.

It looks like I may have a tabs vs. spaces issue. Is 4-space indent the code standard?

Comment thread nodehid.js

// utility method to convert an Array or Buffer to a Buffer for the native C++
// code to work with.
function toBuffer (data) {

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.

@todbot toBuffer() is how this pull request maintains backward compatibility with clients expecting to be able to send an array of bytes. The array is detected and explicitly converted to a buffer (which keeps the native code much cleaner).

Comment thread src/HID.cc

try {
HID* hid = Nan::ObjectWrap::Unwrap<HID>(info.This());
HID* hid = Nan::ObjectWrap::Unwrap<HID>(info.This());

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.

It is possible that removing the try/catch here was a bit optimistic since it could be possible to have the Unwrap or ToObject() fail. On the other hand, the only code that has access to these native calls is the JS facade.

@Julusian

Copy link
Copy Markdown
Collaborator

@todbot this should be closed. The same thing was done by #357

@Julusian

Julusian commented Jun 3, 2025

Copy link
Copy Markdown
Collaborator

An alternate implementation of this has been included since v1.2.0

@Julusian Julusian closed this Jun 3, 2025
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.

2 participants