mwittig / node-milight-promise

A node module to control Milight LED bulbs and OEM equivalents such as Rocket LED, Limitless LED Applamp, Easybulb, s`luce, iLight, iBulb, and Kreuzer
MIT License
114 stars 27 forks source link

Best practice regarding Milight instance #8

Closed Pierre-Gilles closed 4 years ago

Pierre-Gilles commented 7 years ago

Hi !

I'm the founder of Gladys ( https://github.com/GladysProject/gladys ), and I'm using your module in my Gladys-milight module.

As Gladys is a long-running program, I would like to know what is your point of view regarding the life cycle of the Milight object ( see below ). Should I rather create this object once when Gladys start, and use it forever, or should I create an instance of this object each time I want to send a command to a lamp ?

I think keeping it forever can be a good idea for maximum performance, but maybe we can lose the connection with the bridge on the long-term ? What's your point of view ? :)

    var light = new Milight({
        ip: 'IP' ,
        delayBetweenCommands: 75,
        commandRepeat: 2
    });

Thanks for your module !

Quick feedback: Your README documentation is great, but you should specify that pieces of code you are showing are in javascript, so the markdown engine of Github can highlight your code better :)

mwittig commented 7 years ago

Hi Pierre-Gilles,

I am sorry for the delay in reply. Thank you very much for using node-milight-promise as part of Gladys. Btw, the project looks exciting to me and maybe I can contribute in some areas, occasionally. As you may know, I have been actively contributing to the pimatic home-automation framework and authored various plugins. May be some of these plugins can be retrofitted into Gladys.

Regarding your question I'd say it is better to create Milight just once. As the Milight communication is just UDP I have never seen socket failures and I am using this like this as part of long running program. The question is, which socket failures might occur which make the socket unusable and require a new socket to be created? The only case I can imagine is that the network interface is removed temporarilly, e.g. performing ifdownand ifup or restarting networking services on the host. I'll check this and eventually improve error handling. If you know about other cases which might apply please let me know.

Thank you very much also for your quick feedback regarding readability of code snippets. I'll change this asap.

Marcus

Pierre-Gilles commented 7 years ago

May be some of these plugins can be retrofitted into Gladys.

Yeah totally :) I'll get a look !

Regarding your question I'd say it is better to create Milight just once.

Thanks, that what I was thinking first, but I just wanted your confirmation. I think for most case that's the best options. Removing the network interface is not something that happens everyday, and I assume the user who is touching to this kind of settings can understand i can break things. Btw if I get an error I can in Gladys try to create another instance and try again.

Thanks for your answer, and thanks for your changes on the README :)

mwittig commented 7 years ago

Yeah totally :) I'll get a look !

see https://github.com/mwittig?tab=repositories&type=source - basically, projects starting with "pimatic-" are plugins. Unfortunely, github won't let you filter by name and type (source) in a single query.

Maybe the "pimatic-amazing-dash-button" is something fancy to start with.

I'll keep you posted re my findings on socket errors and socket error handling.

Pierre-Gilles commented 7 years ago

Awesome ! Thanks ! :)

mwittig commented 7 years ago

Hi, I have no news regarding the socket error handling, but I wanted to draw your attention to my recent work on supporting the new version of Milight bridges which implement a new protcol and also provide support for the new Full colour bulbs. Maybe this is something to integrate into gladys.

Pierre-Gilles commented 7 years ago

Thx for your message here, Nice work!

Is the API still the same ? Do I have just to update the NPM dependency in my gladys module?

mwittig commented 7 years ago

To support the legacy bridges as is, there are no API changes except for the following: https://github.com/mwittig/node-milight-promise#breaking-and-notable-changes

To support the new bridges the API is slightly different I have written a couple of examples (filenames with "v6" suffix) which demonstrate how to use new protocol. The new bridges also support a new type of bulb which provides "full color" control. https://github.com/mwittig/node-milight-promise/tree/master/examples

Pierre-Gilles commented 7 years ago

Ok I see.

Thx for the explanation !

Does it mean I need to handle both bridges separately ? When I discover bridges with the "all" option, do I know if a bridge is v6 or no ?

In Gladys i'll need to store somewhere if the bridge is v6 or not, and then call the right command for the right version of the bridge

mwittig commented 7 years ago

Sorry for the delay in reply.

Does it mean I need to handle both bridges separately ?

You don't need to handle them seprately, but you need to be aware the command syntax for the new and old protocol does not exactly match. See https://github.com/mwittig/pimatic-milight-reloaded/tree/master/devices for an implementation of all-in-one device drivers.

When I discover bridges with the "all" option, do I know if a bridge is v6 or no ?

The result objects contain a "type" property which is either "legacy" or "v6". See https://github.com/mwittig/node-milight-promise#bridge-discovery

In Gladys i'll need to store somewhere if the bridge is v6 or not, and then call the right command for the right version of the bridge

Yes. I'd say the bridge type is stored a long with the ip address of the bridge

mwittig commented 4 years ago

Answered. Closing