thegecko / webusb

Node.js implementation of the WebUSB Specification
https://thegecko.github.io/webusb/
MIT License
183 stars 27 forks source link

Resolve with stall status instead of rejecting #37

Closed dsanders11 closed 5 years ago

dsanders11 commented 5 years ago

Per the spec, transfers should resolve with status stall if there's a stall.

The spec also mentions babble for transfers in, but I didn't implement that in this PR as it wasn't my immediate need.

Open question, it's not clear to me if transfers in can stall but have received partial data. If that's true, then data should be set for those.

dsanders11 commented 5 years ago

@thegecko, this is going to require updating the types so it compiles, but I only have a passing understanding of TypeScript so I'm not going to open a PR on DefinitelyTyped because I'm not sure what to update in usb-tests.ts.

This is the change I made to make it compile. There may be some other errors which should use this type as well, I didn't exhaustively check. Here's the relevant line in node-usb which defines the error.

 import { EventEmitter } from "events";

+export class libusbException extends Error {
+  errno: number;
+}
+
 export class Device {
   timeout: number;
   busNumber: number;
@@ -25,7 +29,7 @@ export class Device {
   open(defaultConfig?: boolean): void;
   close(): void;
   interface(addr: number): Interface;
-  controlTransfer(bmRequestType: number, bRequest: number, wValue: number, wIndex: number, data_or_length: any, callback: (error?: string, buf?: Buffer) => void): Device;
+  controlTransfer(bmRequestType: number, bRequest: number, wValue: number, wIndex: number, data_or_length: any, callback: (error?: libusbException, buf?: Buffer) => void): Device;
   getStringDescriptor(desc_index: number, callback: (error?: string, buf?: Buffer) => void): void;
   setConfiguration(desired: number, cb: (err?: string) => void): void;
   reset(callback: (err?: string) => void): void;
@@ -104,7 +108,7 @@ export class InEndpoint extends EventEmitter implements Endpoint {
   timeout: number;
   descriptor: EndpointDescriptor;
   constructor(device: Device, descriptor: EndpointDescriptor);
-  transfer(length: number, callback: (error: string, data: Buffer) => void): InEndpoint;
+  transfer(length: number, callback: (error: libusbException, data: Buffer) => void): InEndpoint;
   startPoll(nTransfers?: number, transferSize?: number): void;
   stopPoll(cb?: () => void): void;
 }
@@ -115,8 +119,8 @@ export class OutEndpoint extends EventEmitter implements Endpoint {
   timeout: number;
   descriptor: EndpointDescriptor;
   constructor(device: Device, descriptor: EndpointDescriptor);
-  transfer(buffer: Buffer, cb: (err?: string) => void): OutEndpoint;
-  transferWithZLP(buf: Buffer, cb: (err?: string) => void): void;
+  transfer(buffer: Buffer, cb: (err?: libusbException) => void): OutEndpoint;
+  transferWithZLP(buf: Buffer, cb: (err?: libusbException) => void): void;
 }
thegecko commented 5 years ago

I'm not sure you can add a test to check for a stalled error without mocking the entire library up.

Do you have a way of getting the stalled status to occur (and throw the error) for testing purposes?

dsanders11 commented 5 years ago

Do you have a way of getting the stalled status to occur (and throw the error) for testing purposes?

I do, but it's specific to webcams and requires quite a bit of code, so it's nowhere near a small reproduce case.

thegecko commented 5 years ago

The DefinitelyTyped changes need to be made before this can be merged. You'll need to do that as the only person who can reproduce the case. I can then trust you that it's been tested :)

dsanders11 commented 5 years ago

Created DefinitelyTyped/DefinitelyTyped#31513

thegecko commented 5 years ago

Cool, I've commented on your DT PR

thegecko commented 5 years ago

Once we have the types updated and the new version referenced, I'm happy to merge this :)

thegecko commented 5 years ago

Looks like @types/usb@1.1.7 has been published :)

dsanders11 commented 5 years ago

@thegecko, I've updated this PR to use it, and tested again, everything works as expected.