Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Refactor and split discover function #73

Open
kbabioch opened this issue Jan 21, 2023 · 0 comments · May be fixed by #184
Open

Refactor and split discover function #73

kbabioch opened this issue Jan 21, 2023 · 0 comments · May be fixed by #184
Labels
enhancement Improving exsisting functionality good first issue Good for newcomers

Comments

@kbabioch
Copy link
Collaborator

The discover function in the discover module currently looks like this:

python-luxtronik/luxtronik/discover.py

Lines 20 to 73 in 72f2a05

def discover() -> list[(str, int)]:
"""Broadcast discovery for Luxtronik heat pumps"""
results: list[(str, int)] = []
# pylint: disable=too-many-nested-blocks
for port in LUXTRONIK_DISCOVERY_PORTS:
LOGGER.debug("Send discovery packets to port %s", port)
server = socket.socket(socket.AF_INET, socket.SOCK_DGRAM, socket.IPPROTO_UDP)
server.setsockopt(socket.SOL_SOCKET, socket.SO_BROADCAST, 1)
server.bind(("", port))
server.settimeout(LUXTRONIK_DISCOVERY_TIMEOUT)
# send AIT magic broadcast packet
server.sendto(LUXTRONIK_DISCOVERY_MAGIC_PACKET.encode(), ("<broadcast>", port))
LOGGER.debug("Sending broadcast request %s", LUXTRONIK_DISCOVERY_MAGIC_PACKET.encode())
while True:
try:
res, con = server.recvfrom(1024)
res = res.decode("ascii", errors="ignore")
# if we receive what we just sent, continue
if res == LUXTRONIK_DISCOVERY_MAGIC_PACKET:
continue
ip_address = con[0]
# if the response starts with the magic nonsense
if res.startswith(LUXTRONIK_DISCOVERY_RESPONSE_PREFIX):
res = res.split(";")
LOGGER.debug("Received response from %s %s", ip_address, str(res))
try:
port = int(res[2])
if port < 1 or port > 65535:
LOGGER.debug(
"Response contained an invalid port, ignoring"
)
port = None
except ValueError:
port = None
if port is None:
LOGGER.debug(
"Response did not contain a valid port number,"
"an old Luxtronic software version might be the reason."
)
results.append((ip_address, port))
LOGGER.debug(
"Received response from %s, but with wrong content, skipping",
ip_address,
)
continue
# if the timeout triggers, go on an use the other broadcast port
except socket.timeout:
break
return results

It works perfectly fine, but the function is potentially too big / complex.

It could be split into (at least) two functions, e.g. one that will send the packets and another one which will process the result. Potentially it could also be refactored to work asynchronously.

The advantage of having two such functions would be:

  • We could get rid of the pylint disable statement, since some of the complexity / indents would go into a dedicated function.
  • We could better test this module. Testing the function currently with pytest is non-trivial, because of the socket handling. On the other hand, having a function that will process data is easier to test.

This is not a high priority. Just writing it down, because I though about it. Also this might be a good start for new comers, so I would suggest to leave this open for someone to pick it up, since there is enough more serious work to do.
@kbabioch kbabioch added enhancement Improving exsisting functionality good first issue Good for newcomers labels Jan 21, 2023
@Bouni Bouni linked a pull request Sep 23, 2024 that will close this issue
Open
@Bouni Bouni linked a pull request Sep 23, 2024 that will close this issue
Refactor discover function #184
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement Improving exsisting functionality good first issue Good for newcomers
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Refactor discover function Bouni/python-luxtronik
1 participant
@kbabioch