search

tavicu / homebridge-samsung-tizen

Homebridge plugin for Samsung TV's with Tizen OS
MIT License
639 stars 86 forks source link

Store the target state and switch to it eventually #525

Closed jesec closed 2 years ago

jesec commented 2 years ago

Bug: #503

tavicu commented 2 years ago

I'm sorry but I won't be merging this because of multiple reasons:

This change and the decision to change the accessory behaviour should be made in the services (https://github.com/tavicu/homebridge-samsung-tizen/blob/master/lib/services/television.js#L53-L63).

jesec commented 2 years ago

I'm sorry but I won't be merging this because of multiple reasons:

  • You changed the TURNING_TIMEOUT to a lower value. Some TV's takes more than 3 seconds until they accept new commands.
  • You are not informing the user why his action failed.
  • Since the turn on/off never fail in the code when you will have an automation it will not stop the future actions even if the first step failed.
  • Remote should have nothing to do with accessory. The remote should only be a remote. Receiving a command and execute it.
  • The code doesn't fix the problem because you return without failing the function and the service will not enter to the catch. (https://github.com/tavicu/homebridge-samsung-tizen/blob/master/lib/services/television.js#L57-L62)

This change and the decision to change the accessory behaviour should be made in the services (https://github.com/tavicu/homebridge-samsung-tizen/blob/master/lib/services/television.js#L53-L63).

The PR did work flawlessly on my Samsung TV, and I am pretty content with the change. As the user, I see exactly what I want: the actual TV state eventually syncs to the HomeKit-requested state and no more "device is not responsive". The intended action has not failed, and as expected I don't see any failure. But yeah, I think it would make sense to have a TURNING_TIMEOUT value that accommodates older TVs.

Pull request closed - If you wish, you can delete this fork of tavicu/homebridge-samsung-tizen in the settings.

As you may or may not know, I can't make the recommended changes if someone else closed the PR.

Presumptively, now you are working on this issue, and I will leave it to you. Hopefully, we can have the issue fixed in a future release.

tavicu commented 2 years ago

The PR did work flawlessly on my Samsung TV, and I am pretty content with the change. As the user, I see exactly what I want: the actual TV state eventually syncs to the HomeKit-requested state and no more "device is not responsive". The intended action has not failed, and as expected I don't see any failure. But yeah, I think it would make sense to have a TURNING_TIMEOUT value that accommodates older TVs.

I understand your point but I need to take in consideration that not everybody have technical knowledge. The plugin is designed to be used by everybody with just an install and that's how I want it to remain.

Having strange behaviour without a message will result in open issues because as a normal user who don't know how the API works and what the limitations of TV are, you think everything should work correctly. Nobody will think that Samsung don't allow a request in the first X seconds after turning ON, everybody will think there is a problem with the plugin and it didn't send their request to the TV.

As you may or may not know, I can't make the recommended changes if someone else closed the PR.

I did not know that. I closed this PR from the start because I think the start was wrong. As I told in the previous message, the remote should return the error becaus that's it's thing. The next service that uses the Remote should take the decision what it want's to do with that information.

Presumptively, now you are working on this issue, and I will leave it to you. Hopefully, we can have the issue fixed in a future release.

Right now I'm working to migrate the plugin to ES module because other libraries that we use already migrated and drop the support for older versions. With this migration I will also change all the structure of the plugin and reorganize things because a lot of features were implemented since the first version of the plugin and some things doesn't make sense or are overcomplicated.

This issue is on my list and I will try to find the best solution to it.

Please, don't get me wrong. I appreciate your dedication and the fact that you want to help the community. But since I launched the plugin (5 years I think) I was able to make an ideea on what users are using the plugin based on what issues were open. Some of them can't even help with debug logs. The plugin is made for them too, that's why I want to keep things as simple as I can and to have everything working out of the box without needing to learn coding :d

jesec commented 2 years ago

The PR did work flawlessly on my Samsung TV, and I am pretty content with the change. As the user, I see exactly what I want: the actual TV state eventually syncs to the HomeKit-requested state and no more "device is not responsive". The intended action has not failed, and as expected I don't see any failure. But yeah, I think it would make sense to have a TURNING_TIMEOUT value that accommodates older TVs.

I understand your point but I need to take in consideration that not everybody have technical knowledge. The plugin is designed to be used by everybody with just an install and that's how I want it to remain.

Having strange behaviour without a message will result in open issues because as a normal user who don't know how the API works and what the limitations of TV are, you think everything should work correctly. Nobody will think that Samsung don't allow a request in the first X seconds after turning ON, everybody will think there is a problem with the plugin and it didn't send their request to the TV.

As you may or may not know, I can't make the recommended changes if someone else closed the PR.

I did not know that. I closed this PR from the start because I think the start was wrong. As I told in the previous message, the remote should return the error becaus that's it's thing. The next service that uses the Remote should take the decision what it want's to do with that information.

Presumptively, now you are working on this issue, and I will leave it to you. Hopefully, we can have the issue fixed in a future release.

Right now I'm working to migrate the plugin to ES module because other libraries that we use already migrated and drop the support for older versions. With this migration I will also change all the structure of the plugin and reorganize things because a lot of features were implemented since the first version of the plugin and some things doesn't make sense or are overcomplicated.

This issue is on my list and I will try to find the best solution to it.

Please, don't get me wrong. I appreciate your dedication and the fact that you want to help the community. But since I launched the plugin (5 years I think) I was able to make an ideea on what users are using the plugin based on what issues were open. Some of them can't even help with debug logs. The plugin is made for them too, that's why I want to keep things as simple as I can and to have everything working out of the box without needing to learn coding :d

Got it. But the fact that this is in the FAQ of wiki probably means the current behavior is not exactly "expected":

When you are turning the TV ON or OFF there is a delay of three seconds until you can send another command. That's why if you will try to toggle the switch right away after you already did, it will show you as failed and in the console you will get a message that the TV is in powering ON/OFF process.

and as the user, I do think it is rather bizarre that the accessory becomes unresponsive (in the eyes of the HomeKit client) right after turning it on/off. The behavior does not align with TVs that officially support HomeKit. They behave the same way as the behavior w/ this PR.