mvdwetering / ynca

Python library to communicate with Yamaha AV Receivers that support the YNCA protocol
MIT License
12 stars 5 forks source link

Add a 40s idle timeout as per the YNCA spec #15

Closed andrewmk closed 3 months ago

andrewmk commented 3 months ago

This change makes the YNCA simulator disconnect after 40 seconds of no commands as per the YNCA spec document. I'm a complete novice with threading so this might not be the best way to do it but it seems to work.

Summary by CodeRabbit

coderabbitai[bot] commented 3 months ago

Walkthrough

The recent update introduces a sophisticated threaded mechanism for managing client connections within the server architecture. This enhancement not only allows for efficient output queue management and thread termination but also incorporates a smart timeout feature that disconnects clients based on their inactivity duration, ensuring smoother operation and resource optimization.

Changes

Files Summary
ynca/.../server.py Introduced threaded handling for client connections, output queue management, thread termination, and a timeout mechanism for disconnections.

🐇✨
In the land of code, where the bits do roam,
A rabbit hopped in, making server its home.
With threads it danced, and queues it played,
Whispering to connections, "Here, you shall stay."
But fear not the idle, for time is wise,
A gentle timeout, under moonlit skies.
🌙💻

Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

Share - [X](https://twitter.com/intent/tweet?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A&url=https%3A//coderabbit.ai) - [Mastodon](https://mastodon.social/share?text=I%20just%20used%20%40coderabbitai%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20the%20proprietary%20code.%20Check%20it%20out%3A%20https%3A%2F%2Fcoderabbit.ai) - [Reddit](https://www.reddit.com/submit?title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&text=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code.%20Check%20it%20out%3A%20https%3A//coderabbit.ai) - [LinkedIn](https://www.linkedin.com/sharing/share-offsite/?url=https%3A%2F%2Fcoderabbit.ai&mini=true&title=Great%20tool%20for%20code%20review%20-%20CodeRabbit&summary=I%20just%20used%20CodeRabbit%20for%20my%20code%20review%2C%20and%20it%27s%20fantastic%21%20It%27s%20free%20for%20OSS%20and%20offers%20a%20free%20trial%20for%20proprietary%20code)

Tips ### Chat There are 3 ways to chat with CodeRabbit: - Review comments: Directly reply to a review comment made by CodeRabbit. Example: - `I pushed a fix in commit .` - `Generate unit-tests for this file.` - `Open a follow-up GitHub issue for this discussion.` - Files and specific lines of code (under the "Files changed" tab): Tag `@coderabbitai` in a new review comment at the desired location with your query. Examples: - `@coderabbitai generate unit tests for this file.` - `@coderabbitai modularize this function.` - PR comments: Tag `@coderabbitai` in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples: - `@coderabbitai generate interesting stats about this repository and render them as a table.` - `@coderabbitai show all the console.log statements in this repository.` - `@coderabbitai read src/utils.ts and generate unit tests.` - `@coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.` Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. ### CodeRabbit Commands (invoked as PR comments) - `@coderabbitai pause` to pause the reviews on a PR. - `@coderabbitai resume` to resume the paused reviews. - `@coderabbitai review` to trigger a review. This is useful when automatic reviews are disabled for the repository. - `@coderabbitai resolve` resolve all the CodeRabbit review comments. - `@coderabbitai help` to get help. Additionally, you can add `@coderabbitai ignore` anywhere in the PR description to prevent this PR from being reviewed. ### CodeRabbit Configration File (`.coderabbit.yaml`) - You can programmatically configure CodeRabbit by adding a `.coderabbit.yaml` file to the root of your repository. - The JSON schema for the configuration file is available [here](https://coderabbit.ai/integrations/coderabbit-overrides.v2.json). - If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: `# yaml-language-server: $schema=https://coderabbit.ai/integrations/coderabbit-overrides.v2.json` ### CodeRabbit Discord Community Join our [Discord Community](https://discord.com/invite/GsXnASn26c) to get help, request features, and share feedback.
mvdwetering commented 3 months ago

This seems a quite complicated way to achieve a timeout. Is there no simpler way?

Disconnecting clients after some timeout is quite common and would expect that there would be a way to set timeouts and then the server would take care of this (even on the basic TCPServer example this is based on).

andrewmk commented 3 months ago

There might well be a simpler way - I'm by no means an expert Python programmer. I looked into the source of the readline method that blocks and the class it's part of and couldn't see a way of making it non-blocking but as I say I'm not an expert. That would be a lot simpler if you could do it.

mvdwetering commented 3 months ago

I browsed a bit through the Python docs and found a way to just set a timeout value. I added it in PR #16 .

Just curious, is there a special reason you need the timeout? I never really had a need so never implemented it.

andrewmk commented 3 months ago

I added it because that's how actual Yamaha amplifiers work and it's in the YNCA spec, and someone developing a driver for Yamaha AVRs hasn't got access to one himself so having a simulator that behaves as closely as possible to a real one is a real boon. I was so pleased when I came across your project and it had the simulator.

andrewmk commented 3 months ago

Presumably you're still working on the PR as I can't see how the timeout is set?

mvdwetering commented 3 months ago

I made a separate PR for it as I was not sure how it would work if I tried to add to this one. It was merged in this commit https://github.com/mvdwetering/ynca/commit/2e301ac78f804e1b54bd517d08e58a0665fa2f06

Basically add a timeout to the RequestHandler class and handle the TimeoutError exception to print a proper message..

In case you did not see already, You might be interested in the PRACTICALITIES.md document in the docs directory. I try to keep track of unexpected behavior I encountered with some specific models or just unexpected behavior in the protocol.

Btw keep in mind that, while it is quite helpful, this is in no way a perfect simulator. It just responds with whatever data it has seen before there is (almost) no validation at all. E.g. setting an input to "INVALID INPUT" will be accepted without error. Also things like the automatic sending of values when powering on a zone or switching to an input subunit are not implemented. There is probably more, but those came to mind.

andrewmk commented 3 months ago

Wow that was easy. I couldn't see that despite looking through the docs and source code. I'm glad you found the proper way of doing it.

I realise the simulator isn't perfect but I think it will help the chap writing a driver to have something that insists on disconnecting - he'll have to deal with that with a real AVR. Thanks.