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

Move watcher logic from adapters to the adapter manager #40

Open
cr opened this issue Mar 24, 2016 · 9 comments
Open

Move watcher logic from adapters to the adapter manager #40

cr opened this issue Mar 24, 2016 · 9 comments

Comments

@cr
Copy link
Contributor

cr commented Mar 24, 2016

Managing watchers requires significant portions of code in adapters that is beginning to look like massive boilerplate. Instead, managing watchers, range checking and notification should happen centrally in the adapter manager.

I suggest to have adapters declare on getter registration whether or not a getter support updates. One way to go about this might be to modify the return type to pass an optional mpsc channel that the adapter uses to inform the adapter manager of every update to the getter's value:

fn add_getter(&self, setter: Channel<Getter>) -> Result<Option<Sender<T>>, Error>;

That way we can completely remove register_watch() from the Adapter: Send trait and can have a well-tested central implementation of all the watcher logic.

@Yoric
Copy link
Collaborator

Yoric commented Mar 24, 2016

I'm 99% sure that this will not work for all devices.

On the other hand, I'm also pretty sure that we can provide the boilerplate as a utility class WatcherManager that will work for most devices.

@cr
Copy link
Contributor Author

cr commented Mar 24, 2016

How so?

@Yoric
Copy link
Collaborator

Yoric commented Mar 24, 2016

How so what?

@cr
Copy link
Contributor Author

cr commented Mar 24, 2016

In what way will it fail for certain devices?

@Yoric
Copy link
Collaborator

Yoric commented Mar 25, 2016

Consider a device that:

  1. would require costly communications to send everything to the box, either because it is updated extremely frequently, or because the communications are large, or because the battery is limited;
  2. is smart enough to filter the data on-device.

Examples:

  • a camera offering built-in face detection or object tracking;
  • an alarm clock or anything with a countdown;
  • a sub-foxbox;
  • an oscillation detector;
  • ...

More generally, if the device can perform the filtering, it should do it, because it will do it better and more efficiently than the box. So, API-wise, it should be done by the Adapter, otherwise we are going to lose the ability to talk efficiently to a number of devices. In particular, we won't be able to implement the clock.

On the other hand, since most devices do not have such intelligence, the boilerplate for other devices can be provided as a utility class.

@cr
Copy link
Contributor Author

cr commented Mar 25, 2016

Thanks for spelling it out for me! In the end it is up to the adapter when it sends the update notification, so if a device requires some form of rate limiting there, the adapter has full control over the update timing.

I can't yet picture how your solution with a utility class would look from the adapter dev perspective.

@cr
Copy link
Contributor Author

cr commented Apr 4, 2016

What I was trying to say is that the adapter does not need to send an update message to the manager on every value update. It can well retain its update intelligence, but I don't see why all the watcher management logic has to be replicated in every adapter.

@Yoric
Copy link
Collaborator

Yoric commented Apr 4, 2016

Well, once we have put the management of Option<Range> in the adapter, all that's left is watcher unregistration. I haven't found a convincing way to move it out of the adapter yet.

@Yoric
Copy link
Collaborator

Yoric commented Apr 6, 2016

@cr, I forgot to mention, but if this problem blocks you for the Philipps Hue, I'm pretty sure that you can land a first version without support for Watch. We'll work on implementing watch once this is landed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants