Suggestion to decouple HidDevice from HidDeviceManager

[gabrielpaim]

Hi guys,

I'd like to suggest a decoupling between HidDevice and HidDeviceManager. I think the device should not know about its manager's existence, so to say. The device only uses this reference on the write method, to update the manager. There may be not so many benefits for a typical application, but general decoupling is overall good, from an architecture perspective.

In my opinion, it would be better to have a mutable callback, instead of a hard reference. Something like this:

public interface OnDataSentHandler {
    void handle(int numDataSent);
}

And on the HidDevice:

private OnDataSentHandler onDataSent;
public void setOnDataSentHandler(OnDataSentHandler onDataSent) {
    this.onDataSent = onDataSent;
}

public int write(byte[] message, int packetLength, byte reportId, boolean applyPadding) {
   ...
   int result = HidApi.write(hidDeviceStructure, message, packetLength, reported);
   if (onDataSent != null) onDataSent.handle(result);
}

The HidDeviceManager can set this callback to do its thing after creating the device.

What do you think? Thank you for your effort on this lib, great work by the way :D

[gary-rowe]

Thank you for the suggestion, and the kind words - much appreciated!

As part of of #95 I've introduce another backlink to HidManager for a data read operation (and subsequent event transmission to listeners). At the time it did feel a bit hacky. The problem is that the HidManager knows about listeners, but the HidDevice doesn't and there needs to be a way to communicate between them.

While your suggestion is correct, my (very minor) concern is if it's not the HidManager getting the manager then what other class could take it's place and implement the interface? Java (and other OOP languages) generally promote endless interfaces with only a single implementation for purity. That said, there is a case for a mock class to assist with automated testing which is currently a significant weakness in the library.

What do you think?

[gabrielpaim]

Hi Gary.

You are right to assume that the HidManager should implement the handler. I am not opposed to that. My point was not to have this direct dependency from HidDeviceManager on HidDevice. For instance, one should not need this manager in order to test the HidDevice alone. The proposed handler decouples this dependency while having almost no drawback.

The HidDeviceManager.getAttachedDevices() could implement the handler like this (code adapted from current develop branch):

...

HidDeviceInfoStructure hidDeviceInfoStructure = root;
do {
  // Wrap in HidDevice
  HidDevice device = new HidDevice(hidDeviceInfoStructure); // create the device without the dependency
  device.onDataSent = new OnDataSentHandler() {
    @Override
    public void handle(int numDataSent) {
      // numDataSent is not used in current implementation, but it could be. 
      afterDeviceWrite(); // This method could be private
    }
  };

  hidDeviceList.add(device);
  // Move to the next in the linked list
  hidDeviceInfoStructure = hidDeviceInfoStructure.next();
} while (hidDeviceInfoStructure != null);
...

Java 8 lambdas make this approach of using callbacks more sugared, as it could be written like this:

device.onDataSent = (numDataSent) -> afterDeviceWrite();

But Gary, feel comfortable to reject this suggestion if it does not make sense for you. You have been putting a lot of effort to bring this amazing library to the community, I am thankful for that :)

[gary-rowe]

Hi @gabrielpaim,

Thanks for the extra detail. I've had a close look into implementing this and unfortunately it's become a bit sticky.

At present the HidDeviceManager is responsible for maintaining the collection of attached devices and also for acting as the intermediary to HidServicesListenerList which handles various asynchronous events. As a result of the recent work in #95 a new HidEvent method has been added to cover data read polling.

With that background, the problem with decoupling HidDeviceManager from HidDevice shows up in two areas. First, conceptually it's generally OK to have a back reference in a one-to-many relationship (HidManager has many HidDevice). Second, replacing HidManager with, say, OnDataSentHandler interface breaks the event mechanism by converting it to a direct callback on the main polling thread.

Typically users don't create HidDevice themselves, they rely on HidDeviceManager to do it and so providing extra callback handling isn't currently exposed and may not be useful. I also have to bear in mind that the library has to support Java 7 language levels and so the (generally excellent) closures available in Java 8+ aren't something I can support at this time. (I've had to make many frustrating choices in the name of KISS and backwards compatibility).

So it's with regret that I'll have to close this issue. Once again, thank you for taking the time to work on this aspect. Please feel free to suggest anything else though - I'm always open to new approaches and improvements if I can fit them in.