Data reception for notifications

[TheIdenty]

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

python-snapcast/snapcast/control/protocol.py

Line 38 in 434d121

def data_received(self, data):

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?

[SantiagoSotoC]

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

[luar123]

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

[SantiagoSotoC]

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]

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'))

[luar123]

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.)

[SantiagoSotoC]

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.