python-snapcast icon indicating copy to clipboard operation
python-snapcast copied to clipboard

Data reception for notifications

Open TheIdenty opened this issue 5 years ago • 6 comments

I was wondering why the server class doesn't update when something changes in snapcast. So I investigated a bit further and discovered that the data_received function https://github.com/happyleavesaoc/python-snapcast/blob/434d121ad5ca7a7b742fe5de3d1792c0787afe93/snapcast/control/protocol.py#L38 doesn't get called when data should come in. But when I send data to the server (e.g. change volume) the data_received function is triggered, before data gets send out. And surprise: the received data is exactly the notification I expected a bit earlier. I have absolutly no idea how that can happen and how to fix it - thats beyond my python knowledge. Is this something inherent to my systems or is somebody else having the same issues? What could be done to solve this?

TheIdenty avatar Feb 27 '20 20:02 TheIdenty

This is just what was happening to me with the volume, I can help fix it if it's something that interests you @happyleavesaoc

SantiagoSotoC avatar Jun 03 '24 23:06 SantiagoSotoC

I would be interested in a small example to reproduce the issue.

luar123 avatar Jun 05 '24 17:06 luar123

Well I have an example, also with the example I discovered that the callback of the client is never called, only the one of the group to which it belongs.

It is ideal that you have only one client on the server and that it is active, it was the way I tested it. I think that is enough for an example

import asyncio
import snapcast.control

loop = asyncio.get_event_loop()
server = loop.run_until_complete(snapcast.control.create_server(loop, 'localhost'))

# print all client names
for client in server.clients:
  print(client.friendly_name)

# set volume for client #0 to 50%
client = server.clients[0]
# add callback for client #0 volume change
def client_callback(snap_client):
    print("in client callback")

def group_callback(snap_group):
   print("in group callback")


async def set_volume_client(percent: int):
    await  client.set_volume(percent)
    print("Client instance saved value "+ str(client.volume))
    

async def set_volume_server(percent):
    await server.client_volume(client.identifier, {'percent': percent, 'muted': False})
    print("Client instance saved value "+ str(client.volume))
    
async def testloop():
  client.set_callback(client_callback) 
  client.group.set_callback(group_callback)
  while(1):
      menu = int(input("Pick an option:\n"
                 "1. Change Volume From Server\n"
                 "2. Change Volume From Client \n"
                 "3. Quit\n\n"))
      if menu == 1:
          percent = int(input("Please enter new volume\n"))
          await set_volume_server(percent)
      elif menu == 2:
          percent = int(input("Please enter new volume\n"))
          await set_volume_client(percent)
      elif menu == 3: 
          exit()
      else:
         print("invalid option")



test = loop.create_task(testloop())
 
loop.run_forever()

SantiagoSotoC avatar Jun 06 '24 22:06 SantiagoSotoC

I think the problem is the difference between how the notification is handled and a response. snapserver sends a notification when another client makes a change and this works correctly, but when our client requests a change snapserver responds with a response.

Attached is the code of the SnapcastProtocol class that is relevant.

I can fix this if you like ? @happyleavesaoc

Attached is the snapcast doc where this is referenced.

https://github.com/badaix/snapcast/blob/develop/doc/json_rpc_api/control.md#requests-and-notifications

   def handle_data(self, data):
        """Handle JSONRPC data."""
        if 'id' in data:
            self.handle_response(data)
        else:
            self.handle_notification(data)

    def handle_response(self, data):
        """Handle JSONRPC response."""
        identifier = data.get('id')
        self._buffer[identifier]['data'] = data.get('result')
        self._buffer[identifier]['error'] = data.get('error')
        self._buffer[identifier]['flag'].set()

    def handle_notification(self, data):
        """Handle JSONRPC notification."""
        if data.get('method') in self._callbacks:
            self._callbacks.get(data.get('method'))(data.get('params'))

SantiagoSotoC avatar Jun 06 '24 23:06 SantiagoSotoC

Thanks, now I get it. Two thoughts: 1. I would say this is the intended behavior. If control client A sends a request to snapserver all other control clients get a notification so they can update their internal state. However, control client A gets only a response so it knows if the request was successful. There is no need for notification or callbacks as it already knows what was changed. Do you have another use-case that is not covered by this? home-assistant example: The client callback function is simply self.schedule_update_ha_state which simply tells HA to update the internal datebase from the snapcast client object. When the volume is changed trough

    async def async_set_volume_level(self, volume: float) -> None:
        """Set the volume level."""
        await self._client.set_volume(round(volume * 100))
        self.async_write_ha_state()

no callback is expected, but a similar function self.async_write_ha_state()is used to update HA internal state.

Right now the server response is ignored for most requests and python-snapcast just assumes it was successful. This of course should be changed. E.g. in set_volume the internal state is set before the request:

self._client['config']['volume']['percent'] = percent
await self._server.client_volume(self.identifier, new_volume)

and the group callback is called anyway. So the proper way of doing this would be to first send the request to the server, check the response, and only if it was successfull update the internal state and call group callback. Also there should be a return value, so the caller knows if it was successful. (See stream_add_stream() for an implementation with error check.)

luar123 avatar Jun 08 '24 16:06 luar123

I think that one thing is what snapcast is expected to do and another what the library should do.

Because this behavior makes that we have to consider the same code twice and worry twice if the volume changes.

It makes sense that if a client changes the callback is called to notify.

If you read the README it is what is intuited, a callback is used to notify the client that the volume changed and it does not differentiate if it is from an external client or the library itself.

There is no use case where the current use case does not work, but it could improve the usability of the library.

In that I can help improve the error handling of the methods.

SantiagoSotoC avatar Jun 08 '24 18:06 SantiagoSotoC