ol-iver / denonavr

Automation Library for Denon AVR receivers.
MIT License
174 stars 66 forks source link

Setting the sound mode appears to take time to complete, causing successive commands to be ignored #274

Closed rccoleman closed 7 months ago

rccoleman commented 8 months ago

When sending a set of commands via HA, I found that commands that follow an attempt to set the sound mode are ignored unless I add a 2s delay after that command. Specifically, the last line here in soundmode.py:

    async def async_set_sound_mode(self, sound_mode: str) -> None:                  
        """                
        Set sound_mode of device.                                        

        Valid values depend on the device and should be taken from
        "sound_mode_list".                  
        """              
        if sound_mode == ALL_ZONE_STEREO:
            await self._async_set_all_zone_stereo(True)              
            return

        if self.sound_mode == ALL_ZONE_STEREO:   
            await self._async_set_all_zone_stereo(False)
        # For selection of sound mode other names then at receiving sound modes
        # have to be used                                          
        # Therefore source mapping is needed to get sound_mode                   
        # Create command URL and send command via HTTP GET
        command_url = self._device.urls.command_sel_sound_mode + sound_mode
        telnet_command = (            
            self._device.telnet_commands.command_sel_sound_mode + sound_mode
        )                           
        # sent command                 
        success = self._device.telnet_api.send_commands(telnet_command)
        if not success:                   
            await self._device.api.async_get_command(command_url)
        await asyncio.sleep(2) 

1 second is not long enough, but 2 seconds seems reliable for my Denon X4400.

I found this when storing the state of a media_player entity in a scene and then trying to restore that state by applying that scene. I think that a case can be made to abstract this apparent requirement in this package to avoid exposing it to clients, or to not introduce delays here and expect the client to know that they need to wait. I prefer the former, but I can see both.

I wrote this up in the HA repo initially while I was investigating, but I'm opening this as the temporary change that I've made is in this package.

After some manual testing with telnet, it looks like I can issue the MS? command while within that 2s window and it returns the current state, but I just don't get a response to volume set (MV) commands. Unfortunately, because it's starting and ending in the same mode, I can't tell if the command has completed.

ol-iver commented 7 months ago

Thanks posting this issue. I haven't notice this so far, maybe because I do not change the sound mode of my receiver very often.

However, the 2 seconds waiting time sound rather like a workaround for your particular problem then a real fix.

It might make sense to wait for an confirmation of each telnet command and block sending new commands in the meantime in order to solve this issue generally. Many commands are sending reponses, but I'm not sure if that's the case for all the commands the library is using. In case the issue is related to telnet only, it might be an alternative to not use telnet for this command, but running the HTTP request instead.

Afaik you found another solution for your issue by adding a delay to the HA sequence. I would rather look for a generic solution of the problem than adding a sleep to the async_set_sound_mode method.

rccoleman commented 7 months ago

This is clearly a hack and workaround and I'm not really suggesting it as a solution. In fact, after looking in vain for some positive feedback that the sound mode had changed, I don't think there is a reasonable solution for the reasons that you mentioned. This came up on Discord while I was helping someone store and restore a volume level and it seemed like a scene should work. When it didn't, I went down the rabbit hole to find out why. The alternative is to just store and restore the volume (and whatever other parameters) rather than using a scene. My recollection is that switching out of Telnet mode didn't help, but I could be wrong. It just specifically ignores in this specific case for a short period of time.

In light of that, I don't have concerns about just closing this, but wanted to see if anyone else had thoughts on it.

ol-iver commented 7 months ago

The receiver sends confirmation messages after each command (unless you send a command which does not change its state). Thus, it is possible to lock the telnet send method until a confirmation arrives. You can see that confirmations for changing the sound mode take longer than those for changing the volume for example.

I implemented this solution which should work for parallel calls and indepent from the receivers reponse speed too.

rccoleman commented 7 months ago

That's great, thanks! I didn't see a response in an interactive telnet session or when I instrumented the library, but perhaps I missed it. I think the issue was this bit, as the sound mode doesn't change if only the volume is being set:

(unless you send a command which does not change its state)

It still takes time even though it's not actually changing the state, and the receiver appears to go through the same process regardless of where it came from.