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

Add group support #11

Open
finnkauski opened this issue Jul 12, 2020 · 5 comments
Open

Add group support #11

finnkauski opened this issue Jul 12, 2020 · 5 comments
Assignees
Labels
enhancement New feature or request help wanted Extra attention is needed

Comments

@finnkauski
Copy link
Owner

Is your feature request related to a problem? Please describe.
Ideally we would be able to use light groups as well as individual lights.

Describe the solution you'd like
Ideally we could have another structure of some kind where we store lights akin to:

pub enum LightEntities {
  Light {...},
  LightStrip {...}
}

LightGroup { 
  name: String,
  lights: Vec<LightEntities>
}

Describe alternatives you've considered
There are a few ways to implement this. Unclear yet what to do specifically.

@finnkauski finnkauski added enhancement New feature or request help wanted Extra attention is needed labels Jul 12, 2020
@finnkauski finnkauski self-assigned this Jul 12, 2020
@finnkauski finnkauski added this to To do in Main issue tracker Jul 12, 2020
@ssnover
Copy link
Contributor

ssnover commented Aug 22, 2020

Do you want to support all of the Group APIs initially? A number of the APIs are concerned around creation and deletion of groups, which is something that I think tends to be easier from the app, although I can certainly see a use case for creating an alternate workflow. Nominally, I use this crate for controlling a system that's already been set up, so it's more concerned with automations that happen periodically rather than one-time setup.

For my use cases, I think there's more value in getting the "runtime" APIs defined than the "config" APIs. Would you be interested in a proposal for a crate API that leaves out the latter and only introduces the former?

@finnkauski
Copy link
Owner Author

finnkauski commented Aug 23, 2020

I think you're right, we don't need the configuration side of it done yet. I think if you were setting things up initially we should encourage use of the official app as they have a team behind them to maintain all of it! 😃

I think we should definitely focus on the runtime for now to make sure we cover as wide of an API as possible without the focus on the setup. In my use cases I am using this to control my lights with my drumkit so I'm very much focused on adding more granular control of groups at runtime rather than setting them up originally.

Any API proposal is welcome. I think as long as the intention to support the full scope of the Hue API is possible with whatever API you propose thats fine by me.

@finnkauski
Copy link
Owner Author

Note that the bridge struct has been updated recently with the storage of ID's for each light. This might need to be rethought for the groups to work. The lights_ids field is there just for the purposes of supporting the Bridge::to_all function as we send light state by id. In general there is definitely scope for nice abstraction in there somewhere.

@ssnover
Copy link
Contributor

ssnover commented Aug 24, 2020

I've been working on this in the back of my mind today. I think the get all groups is pretty straightforward. The question of how the API should look revolves more around how to make groups play nicely alongside with individual lights. It'd be nice to have a single method on the Bridge object that was something like send_go_to_state(endpoint: Controllable, state: SendableState) where the Controllable could be either a light group object or a light object.

Right now, the Bridge has state_to(&self, id: u8, new_state: &SendableState) which using the id from the lights makes it ambiguous to use the same method for both groups and individual lights. I think it'd be cleaner to be able to unify the API for lights and groups. Right now the information on how to send lives in the bridge and lights are represented with an enum. The bridge's API could be extended and unified in two ways:

  1. The API could be extended to keep that pattern where instead of an id: u8 an enum ControlEndpoint (bad name, but that's a later step) could be used. The function matches through the enums and gets the info it needs to send the command to the correct Hue API endpoint.
  2. Alternatively, a trait could be defined and then implemented for a light object and for a light group object. Then the objects themselves know their API endpoint and give them to the bridge when it needs it.

I like number 1, both because it's closer to what you currently have and I think it's a little easier for user code I think. Number 2 could be nice if the number of objects this particular API needed to support got large since the match statement might get pretty large. I don't have many of Hue's products yet, but I don't think this is a problem based on a cursory glance through their docs.

Also, that drumkit setup sounds sweet!

@finnkauski
Copy link
Owner Author

finnkauski commented Aug 26, 2020

I think the first one sounds better to me, in the later case if we make it generic over a trait I can't really remember what the consequences of that would be for the API (do we need to using Dynamic alloc)... Anyway, the key here is that the lights will either be in a group or not so the enum shouldn't grow beyond that? Mainly as it only represents either a light or a group. So the behaviour shouldn't branch more than twice unless there are things in the API that act in a third kind of way. Or am I missing something in my mental model?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed
Projects
Development

No branches or pull requests

2 participants