MelbourneDeveloper / Device.Net

A C# cross platform connected device framework
MIT License
620 stars 119 forks source link

System.ArgumentOutOfRangeException "Positive number required" #199

Open DKreskowiak opened 3 years ago

DKreskowiak commented 3 years ago

Trying the "Getting Started" code on Windows with an APC Back-UPS 1500 connected over USB.

The line with the call to InitializeAsync() fails:

var upsDevice = await hidFactory.GetDeviceAsync(deviceDefinitions.First()).ConfigureAwait(false);
await upsDevice.InitializeAsync().ConfigureAwait(false);                                <---------- Exception thrown

with the exception in the title. The problem appears to be the initialize logic always assumes the device definition returns a "WriteBufferSize" greater than 0. For some devices, this is not the case. This makes using the library with such devices impossible.

The device definition returned for the APC Back-UPS 1500 returns: Class GUID: 4d1e55b2-f16f-11cf-88cb-001111000030 Device ID: \?\hid#vid_051d&pid_0002#7&49c3947&0&0000#{4d1e55b2-f16f-11cf-88cb-001111000030} Device Type: Hid Label: Manufacturer: American Power Conversion Product ID: 2 Product Name: Back-UPS RS 1500MS2 FW:969.e2 .D USB FW:e2 Read Buffer Size: 5 Serial Number: 4B2042P34953 Usage: 0x4 (4) Usage Page: 0x84 (132) Vendor ID: 0x051D Version: 144 Write Buffer Size: 0

MelbourneDeveloper commented 3 years ago

@DKreskowiak debug the code and find out.

It would be really awesome if you could submit a PR with a unit test that proves this hypothesis

MelbourneDeveloper commented 3 years ago

The problem may be around here

MelbourneDeveloper commented 3 years ago

@DKreskowiak if you're interested in discussing the current state of development, please jump on Discord here

DKreskowiak commented 3 years ago

Hi!

I’m a bit short on time, but I took a couple minutes to look at the InitializeAsync method.

Follow the code and assume CommectedDeviceDefinition.WriteBufferSize has a value of 0. When you get down to:

//Don't open if this is a read only connection

              _writeFileStream = _hidService.OpenWrite(_writeSafeFileHandle, WriteBufferSize.Value);

              if (_writeFileStream.CanWrite)

              {

                  _logger.LogInformation(Messages.SuccessMessageWriteFileStreamOpened);

              }

              else

              {

                  _logger.LogWarning(Messages.WarningMessageWriteFileStreamCantWrite);

              }

OpenWrite expects a non-zero value for a buffer size. This is situation is never checked for above this piece of code. The closest you get is immediately above that piece of code:

if (!WriteBufferSize.HasValue)

              {

                  throw new ValidationException(

                      $"WriteBufferSize must be specified. HidD_GetAttributes may have failed or returned an OutputReportByteLength of 0. Please specify this argument in the constructor");

              }

I think the if condition expression should be (!WriteBufferSize.HasValue || WriteBufferSize == 0)

Dave

From: Christian Findlay @.> Sent: Saturday, April 17, 2021 8:47 PM To: MelbourneDeveloper/Device.Net @.> Cc: DKreskowiak @.>; Mention @.> Subject: Re: [MelbourneDeveloper/Device.Net] System.ArgumentOutOfRangeException "Positive number required" (#199)

The problem may be around here https://github.com/MelbourneDeveloper/Device.Net/blob/90aa91e584ec2f7e25002c157ad6bd42e87aa618/src/Hid.Net/Windows/WindowsHidHandler.cs#L92

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/MelbourneDeveloper/Device.Net/issues/199#issuecomment-821909548 , or unsubscribe https://github.com/notifications/unsubscribe-auth/AKU5NGMKHFGNM447HFK6RMLTJITWRANCNFSM43DSWWCQ .

MelbourneDeveloper commented 3 years ago

@DKreskowiak cheers . I won't be looking at this for a while. But if you've got budget, I can help you get the connection working. Hot me up on discord.

brendonparker commented 3 years ago

I just ran into this as well.

Here is a unit test to illustrate: https://github.com/brendonparker/Device.Net/commit/da9be364382e54527877c04d76dbfc03d391e1d0

brendonparker commented 3 years ago

I've created PR #218

However, I've discovered this can be worked around with the current codebase by specifying writeBufferSize when creating the IDeviceFactory via CreateWindowsHidDeviceFactory:

Example:

var factory = new FilterDeviceDefinition(vendorId: 0x0EB8).CreateWindowsHidDeviceFactory(_loggerFactory, writeBufferSize: 100);
brendonparker commented 3 years ago

@MelbourneDeveloper Do you still need a fresh issue? I feel this one covers it pretty well...

In my scenario, I'm trying to read from a USB scale. Setup code looks like this:

var deviceDefinitions = new[]
{
    // Mettler Toledo
    new FilterDeviceDefinition(vendorId: 0x0EB8)
};
var factory = deviceDefinitions.CreateWindowsHidDeviceFactory(loggerFactory: null);
var device = await factory.GetFirstDeviceAsync();
await device.InitializeAsync();

The InitializeAsync call throws an ArgumentOutOfRangeException:

Positive number required. (Parameter 'bufferSize')

   at System.IO.FileStream.ValidateAndInitFromHandle(SafeFileHandle handle, FileAccess access, Int32 bufferSize, Boolean isAsync)
   at System.IO.FileStream..ctor(SafeFileHandle handle, FileAccess access, Int32 bufferSize, Boolean isAsync)
   at Hid.Net.Windows.WindowsHidApiService.OpenWrite(SafeFileHandle writeSafeFileHandle, UInt16 writeBufferSize) in /_/src/Hid.Net/Windows/WindowsHidApiService.cs:line 142
   at Hid.Net.Windows.WindowsHidHandler.<InitializeAsync>b__33_0() in /_/src/Hid.Net/Windows/WindowsHidHandler.cs:line 150
   at System.Threading.Tasks.Task.InnerInvoke()
   at System.Threading.Tasks.Task.<>c.<.cctor>b__274_0(Object obj)
   at System.Threading.ExecutionContext.RunFromThreadPoolDispatchLoop(Thread threadPoolThread, ExecutionContext executionContext, ContextCallback callback, Object state)

The hidService CreateWriteConnection returns a SafeFileHandle where IsInvalid is false (i.e. it is valid).

https://github.com/MelbourneDeveloper/Device.Net/blob/c0ab331b78b156cf3af91722661c35dae993e31e/src/Hid.Net/Windows/WindowsHidHandler.cs#L105

However, the GetDeviceDefinition call later results in a WriteBufferSize of zero. So since the code thinks the device is not readonly, it tries to open a write stream, which then throws an exception when given a buffer length of zero.

https://github.com/MelbourneDeveloper/Device.Net/blob/c0ab331b78b156cf3af91722661c35dae993e31e/src/Hid.Net/Windows/WindowsHidHandler.cs#L119

So I'm not sure how you'd like this fixed. My original PR was to return Stream.Null if you ask for a stream with a zero-length buffer. I suppose an alternative workaround is to set the writeBufferSize argument on the factory to something non-zero.

var deviceDefinitions = new[]
{
    // Mettler Toledo
    new FilterDeviceDefinition(vendorId: 0x0EB8)
};
var factory = deviceDefinitions.CreateWindowsHidDeviceFactory(loggerFactory: null, writeBufferSize: 100);
var device = await factory.GetFirstDeviceAsync();
await device.InitializeAsync();

Or, could do as the original issue reporter suggested and add short circuit the InitializeAsync on line 141, changing it to:

if (IsReadOnly.Value || (WriteBufferSize.HasValue && WriteBufferSize.Value == 0)) return;