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

README: Add warning about writing data to HID products #106

Merged
merged 3 commits into from
Nov 5, 2019

Conversation

Qbicz
Copy link
Member

@Qbicz Qbicz commented Oct 18, 2019

In response to #105

@Qbicz
Copy link
Member Author

Qbicz commented Oct 18, 2019

@Youw @z3ntu @amosbird can you review?

README.md Outdated
@@ -139,6 +142,10 @@ int main(int argc, char* argv[])
}
```

You can also use [hidtest/hidtest.c](https://github.com/libusb/hidapi/blob/master/hidtest/hidtest.c)
as a starting point for your programmes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

programs instead of programmes? I know it's a bit British vs American English but it's more fitting in this context.

Copy link
Member Author

Choose a reason for hiding this comment

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

applications ;)

README.md Outdated
which communicates with a heavily hacked up version of the Microchip USB
Generic HID sample looks like this (with error checking removed for
simplicity):

`Warning: Only run the code you understand, only against devices which firmware
you control. Writing data at random to your HID devices can break them.`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would use *s instead of `s, so to make it bold because it looks a bit weird in this preformatted style.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@Youw Youw left a comment

Choose a reason for hiding this comment

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

only against devices which firmware
you control

This statement is a bit too strong - you may not nesessary control the FW, but safely write, e.g. if you have official spec, etc.

I was thinking, maybe add explicit check for dev/ven before hid_write in the example

@Qbicz
Copy link
Member Author

Qbicz commented Oct 21, 2019

This statement is a bit too strong - you may not nesessary control the FW, but safely write, e.g. if you have official spec, etc.

@Youw done.

I was thinking, maybe add explicit check for dev/ven before hid_write in the example

I think that would be too much, since to write to the device in the sample you have to enumerate devices, check vid/pid and hardcode it to the sample. Or do you mean interactive check like with gets()?

@Youw
Copy link
Member

Youw commented Oct 21, 2019

I was thinking something like:

#define SAMPLE_VEN_ID ...
#define SAMPLE_DEV_ID ...
unsigned short ven_id = SAMPLE_VEN_ID;
unsigned short dev_id = SAMPLE_DEV_ID;

int main(/*...*/) {
  if (argc /* ... */ ) {
    // ...
    ven_id = // parse arguments, etc.
    // ...
  }

  handle = hid_open(ven_id, dev_id, NULL);
  // ...

  // a WARNING comment here, what we're know what we're doing, but only with a specific device
  if (ven_id == SAMPLE_VEN_ID && dev_id == SAMPLE_DEV_ID) {
    // ...
    res = hid_write(handle, buf, 65);
    // ...
  }
  // hid_close
  // hid_exit
}

So we can include #14 and a little more, like explicitly encourage to hid_close which is not in an example.


Maybe this is a little too large change, but anyway nice to have.

@Qbicz
Copy link
Member Author

Qbicz commented Nov 3, 2019

@Youw thanks for your comments, I added hid_close() call to the sample as you suggested, so this is now explicitly visible.

Copy link
Member

@Youw Youw left a comment

Choose a reason for hiding this comment

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

and let's merge it after #115

README.md Outdated Show resolved Hide resolved
Co-Authored-By: Ihor Dutchak <ihor.youw@gmail.com>
@Qbicz Qbicz merged commit 7e91963 into master Nov 5, 2019
@Youw Youw deleted the sample-code-warning branch November 15, 2019 12:18
@mcuee mcuee added the documentation Improvements or additions to documentation label Jul 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants