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

Fix discover #20

Merged
merged 1 commit into from
Jan 25, 2024
Merged

Fix discover #20

merged 1 commit into from
Jan 25, 2024

Conversation

jNullj
Copy link
Contributor

@jNullj jNullj commented Jan 24, 2024

Fixes issue where discovery timeout as no new ssdp object is created which results in Network timeout, Yeelight not response.
Might fix #18
Also fixes issue where multiple discovery attempts are made results in Network timeout, Yeelight not response as the old ssdp connection is never closed which results with the new ssdp connection can't bind, which later results in timeout for the response.
This started showing for me after updateing with npm to latest yeelight after trying to upgrade ssdp2 in jNullj/light-on-tray#58

Fixes issue where discovery times out as no new ssdp object is created.
Also fixes issue where multiple discovery attempts are made.
@song940 song940 merged commit 800a0a5 into song940:master Jan 25, 2024
@song940
Copy link
Owner

song940 commented Jan 25, 2024

thank you very much for your contribution and the PR submitted to our project.

Regarding the issue you mentioned about old ssdp connection is never closed which results with the new ssdp connection can't bind I would like to elaborate on our design considerations and future improvement plans.

  1. Regarding SSDP Connection Management: You have correctly pointed out the issue in the current implementation where old SSDP connections are not properly closed during multiple device discovery processes. This is indeed a problem we need to address, as it can prevent new SSDP connections from being established successfully, affecting the efficiency and reliability of device discovery.

  2. Design of Yeelight.discover: Our initial design intention was to allow Yeelight.discover to find multiple devices, which is particularly important in multi-device environments. Therefore, we cannot simply decide internally when to close the SSDP connection; instead, we need to provide a mechanism for developers to control the timing of closing the connection according to their needs.

  3. Suggestions in Your PR: We noticed that your modification might lead to Yeelight.discover only being able to discover one device. While this might be useful in certain scenarios, it limits the application of the library in multi-device environments. We need to find a balance that addresses resource management issues without sacrificing the diversity of functionality.

  4. Documentation Update: We have added explanations about this behavior in the README and emphasized the importance of proper management of SSDP connections. We hope this will help developers better understand and use our library.

  5. Future Improvements: We are considering adding an option for automatic closure timeout in the options parameter of Yeelight.discover. This will provide developers with more flexibility and help avoid issues due to forgetting to close connections.

If you have any further thoughts or suggestions, please feel free to share them with us. We look forward to continuously perfecting this project together with the community.

Thank you again for your contribution and support!

@song940
Copy link
Owner

song940 commented Jan 25, 2024

We have added Yeelight#find:

const Yeelight = require('yeelight2');

(async () => { 
  const light = await Yeelight.find();
  console.log(light.name);
  light.toggle();
})();

You can use it instead Yeelight#discover.

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

Successfully merging this pull request may close these issues.

Node red crashing....... NPM Version not updated?
2 participants