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

GPO listener client connection and device reassignment #517

Merged
merged 1 commit into from
Aug 13, 2023
Merged

GPO listener client connection and device reassignment #517

merged 1 commit into from
Aug 13, 2023

Conversation

syoffe
Copy link
Contributor

@syoffe syoffe commented Dec 2, 2022

Corrected a few errors preventing the GPO listener client connecting to the server and handling device reassignment properly. Made similar changes to src/index.ts for relay listener, too (untested).

@syoffe
Corrected a few errors preventing GPO listener connecting to the serv…
abde0bf 
…er and behaving handling device reassign properly.
github-actions[bot]
github-actions bot reviewed Dec 2, 2022
View reviewed changes
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello there syoffe 👋

Thank you for opening your first PR for the Tally Arbiter project!

We will review it soon!

TallyArbiter fosters an open and welcoming environment for all our contributors. Please adhere to our Code Of Conduct.

@syoffe
Copy link
Contributor Author

syoffe commented Dec 4, 2022

Hello @MatteoGheza,

The automatic build failure for TTGO T7 v14 Mini32 Listener seems to be unrelated to the code changes, with the error being installing websockets:

[58](https://github.com/josephdadams/TallyArbiter/actions/runs/3604854466/jobs/6083972336#step:4:62)
   Downloading WebSockets@2.3.6...
[59](https://github.com/josephdadams/TallyArbiter/actions/runs/3604854466/jobs/6083972336#step:4:63)
  
[60](https://github.com/josephdadams/TallyArbiter/actions/runs/3604854466/jobs/6083972336#step:4:64)
  WebSockets@2.3.6 0 B / 981 B    0.00%
[61](https://github.com/josephdadams/TallyArbiter/actions/runs/3604854466/jobs/6083972336#step:4:65)
  WebSockets@2.3.6 Server responded with: 503 Service Unavailable
[62](https://github.com/josephdadams/TallyArbiter/actions/runs/3604854466/jobs/6083972336#step:4:66)
  Error installing WebSockets: Can't download library: Server responded with: 503 Service Unavailable

@MatteoGheza
Copy link
Collaborator

Thanks for the contribution, and sorry for the delay...
I'll look into this PR later this day, as soon as possibile.
The CI automatic build pipeline needs to be updated, thanks for reporting this to me, even if I think this is a platformIO server-related issue.

After taking a quick look at the code, I'll add some reviews.

MatteoGheza
MatteoGheza reviewed Jan 3, 2023
View reviewed changes
listener_clients/gpo-listener/gpo-listener.py
@@ -344,7 +346,7 @@ def main():
while True:
time.sleep(0.1)
else:
server_connect(str(config_object["server_config"]["ip"]), str(config_object["server_config"]["port"]))
server_connect("http://" + str(config_object["server_config"]["ip"]) + ":" + str(config_object["server_config"]["port"]))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! I forgot to test this condition 🤦🏻

listener_clients/gpo-listener/gpo-listener.py
@@ -262,7 +264,7 @@ def on_reassign(oldDeviceId, newDeviceId, gpoGroupId):
gpo_group["deviceId"] = newDeviceId

saveConfig()

sio.emit('listener_reassign_gpo', (gpoGroupId, oldDeviceId, newDeviceId));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We forgot to document this in the code.
We are trying to deprecate the old listener_reassign_gpo events, migrating to a single event for all the different types of listeners. I'll update this and add some warnings in the codebase.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @MatteoGheza, ah no problem, that makes sense. Let me know if I can help test in any way. Part of the problem with the reassign was using the variable listener_clients[I].gpoGroupId in index.ts, which should have been listener_clients[i].internalId. I guess this is part of the migration to a single interface for all devices.

@josephdadams josephdadams merged commit 5bf28c3 into josephdadams:master Aug 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MatteoGheza MatteoGheza MatteoGheza left review comments

@github-actions github-actions[bot] github-actions left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@syoffe @MatteoGheza @josephdadams