search

jmccrohan / pysolarmanv5

A python module to interact with Solarman Data Logging Sticks
MIT License
116 stars 25 forks source link

Remove adding socket kwargs as a string #34

Closed danfoster closed 1 year ago

danfoster commented 1 year ago

I'm not sure what the original intention of adding the socket kwarg as an empty string here, but as you can see if you create the PySolarmanV5Async, it always fails with the following:

  File "/workspaces/home-assistant-core/config/deps/solis/src/solis/cli.py", line 21, in main
    ctx.obj["solis"] = await Solis.create(ip, serial, port)
  File "/workspaces/home-assistant-core/config/deps/solis/src/solis/solis.py", line 142, in create
    self = cls(ipaddr, serial, port)
  File "/workspaces/home-assistant-core/config/deps/solis/src/solis/solis.py", line 150, in __init__
    self._modbus = pysolarmanv5.PySolarmanV5Async(
  File "/workspaces/home-assistant-core/config/deps/pysolarmanv5/pysolarmanv5/pysolarmanv5_async.py", line 49, in __init__
    super(PySolarmanV5Async, self).__init__(address, serial, **kwargs)
  File "/workspaces/home-assistant-core/config/deps/pysolarmanv5/pysolarmanv5/pysolarmanv5.py", line 106, in __init__
    self._sock_fd = self.sock.fileno()
AttributeError: 'str' object has no attribute 'fileno'

Therefore this PR proposes removing this line.

githubDante commented 1 year ago

The intention is to avoid socket creation by PySolarmanV5. The code here was added with #33 and indeed must be wrapped in something like this:

        elif isinstance(self.sock, socket.socket):
            self._poll = select.poll()
            self._sock_fd = self.sock.fileno()
            self._auto_reconnect = False if kwargs.get('socket') else kwargs.get('auto_reconnect', False)
            self._data_queue = Queue(maxsize=1)
            self._data_wanted = Event()
            self._reader_exit = Event()
            self._reader_thr = Thread(target=self._data_receiver, daemon=True)
            self._reader_thr.start()

otherwise two simultaneous connections will be opened to the data logger.

danfoster commented 1 year ago

Ah nice! Yes, that logic does sound familiar. Thanks for correcting me. Looking like you've already got an WIP branch with the correct approach for the fix, so I'll close this.

githubDante commented 1 year ago

Yes, I had to write data logger simulator in order to catch other issues as well. Looks good for now. Don't know what @jmccrohan will say about these changes though.

jmccrohan commented 1 year ago

@danfoster @githubDante Thanks for this. I was working on tidying up for release. Send on your branch as a PR and I'll merge.