Improved Error Handling

[psit]

Hi!

It would be great to have a little better error handling, especially two scenarios, I have just encountered: 1.) on Linux, you need write access to the devices to open them. The corresponding error message is printed to stderr, but HidDevice.open() simply fails (returns false). An exception or error code would be great to indicate missing privileges. 2.) when a device is detached, read() returns with -1, indicating an error. The corresponding error message (HidDevice.getLastErrorMessage()) even tells you, that the device is not attached - but again as this is a localized String, it is hard to react on it. E.g. if the read fails, because the device is not attached, I would like to stop reading at all. On another error, I could want to retry.

Kind regards, Peter

[gary-rowe]

Sounds reasonable. It's an API change but there is scope to improve the error return codes, probably as a descriptive enum.

I'm currently quite busy working on MultiBit HD (the reason I wrote hid4java) so it may be a couple of weeks before I could get around to it. If you're able to offer up a pull request that matches the kind of thing you're after I'd be happy to review and merge it so you can get the result you want faster.

[psit]

Hi Gary,

I guess this task would require some native library coding - which I am not capable of (that why I use java APIs for native tasks). Guess I will have to wait for 0.4.0 - which is ok as the issue is annoying, but not that much of a problem.

[gary-rowe]

Due to hid4java 0.4.0 getting accepted into Fedora I have to bump this API change into 0.5.0

[gary-rowe]

This has taken me much longer than I'd have normally expected to address. Can you verify that this is still required?

[psit]

As I said initially, this is annoying but once you get it, you can work around it a little bit. The missing permissions are usually a one-time-fail, but the error on read is quite significant, as only the error message tells you, that the device has gone, but this is hard to catch programmatically. For an API, I would expect to have status codes or Exceptions that tell me, what happened, so I can either tell the user or even react on the real problem automatically. So, short answer: yes, this is still needed - if not by me then by anybody using this API in the future. Thanks for your effort.

[gary-rowe]

Thanks for getting back to me. In the case of the device disappearing, I take it that you're not getting the Attach/Detach events?

[psit]

I do get the events if autoscan is on. As this is interfering with my keyboard (scroll-lock, num-lock are blinking on every USB ping), I disabled it (see issue #17). Further, the read() returns -1 immediately the moment I unplug the device, while the detach event is triggered on the next autoscan interval (which is on average 250 ms later AND on another thread). So using the detach events for the specific read error is not really an option in my opinion. A simple DeviceDetached Exception or maybe a more generic IOException would be better (or in C-Style using negative integers to give more detail on the error - but this is ... C-style and my personal opinion).

[gary-rowe]

OK, thanks for the extra detail. I'll look into a "same thread" solution as you have highlighted.

[andyrozman]

Since this is OOP language, throwing exception would be acceptable, and even expected.

[gary-rowe]

Yeah, I'm thinking in terms of a runtime exception, but there is the principle of least surprise for users of 0.4.0 and below when suddenly they start getting exceptions where before it was fairly quiet.

[andyrozman]

It would be better if it's not Runtime Exception, since those are unexpected. If you say that some method throws Exception (normal one). then users will expect that exception can be thrown and they will have to change their code accordingly... Better like that, then throwing Runtime Exception, since then people will really have problems.

[gary-rowe]

Understood - although there is a general trend away from checked exceptions in Java.

My angle is that I could offer a flag to allow the user to set a preference for exception (new behaviour) vs status code with eventual event (current behaviour). However I think that would violate the KISS principle which overrides Least Surprise.

OK, I'll start with a checked exception and clear documentation.

[andyrozman]

It probably depends on how big the project is... My project is not that big, so I always use Checked Exceptions. The project on which I work, at my job is very big and it uses both types of exception, which is sometimes a real problem, because sometimes exceptions slip through and are catched at last line of defence (just before user sees them) instead of where they should be...

[psit]

I'm not a fan of runtime exceptions. The programmer needs to check for them anyway, but has to know that they might be thrown (Think about NumberFormatExceptions - I always check for them "manually" as they are runtime exceptions). Checked exceptions "force" the programmer to care about them. And as this is an I/O operation, these exceptions are very common and programmers are used to checked exceptions in I/O operations. Further, they improve code readability - compared to numeric return codes ("What was the meaning of -127 again?"). Thank you gary-rowe for your effort on this cool API!

[gary-rowe]

Bumped into milestone 0.6.0 so I can dedicate a proper block of time to it.

[gary-rowe]

I doubt I'll be implementing this any time soon.