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.

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

Pull request to integrate Granboard #8

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

S-I-M-O-N
Copy link

Code for Granboard added.
Change of noble repository to abandonware as it is not in the production anymore.
Added some environmental variables for more flexibility.
Keep the link after a match is finished or cancelled.

@thordy
Copy link
Contributor

thordy commented Nov 20, 2024

Hi @S-I-M-O-N,
Thank you so much for the PR! I will review it as soon as I'm able to!

Would also love to hear about how you are using kcapp, and what your experience with Granboard has been?
Feel free to drop me a message on Slack or email me [email protected]

Copy link
Contributor

@thordy thordy left a comment

Choose a reason for hiding this comment

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

Added some minor comments

granboard.js Outdated
var rawValue = data.toString();
if (rawValue == "BTN@") {
playerChangeCallback();
} else if (rawValue == "GB5;101") {
Copy link
Contributor

Choose a reason for hiding this comment

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

@S-I-M-O-N any idea what this specific value is? Maybe add a comment to explain why we ignore it

Copy link
Author

Choose a reason for hiding this comment

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

It is an initializing value the board sends after subscribing to the notify. As we do not know if this message is the same for all versions of the board, I updated the code to reject all undefined messages. See new commit.

granboard.js Show resolved Hide resolved
kcapp-smartboard.js Show resolved Hide resolved
@@ -102,7 +111,67 @@ function connectToMatch(data) {
} else {
debug("Already connected to board...");
leg.emit('announce', { type: 'success', message: 'Already Connected' });
leg.on('leg_finished', disconnectListener.bind(this));
smartboard.initialize(peripheral, config.smartboard_button_number,
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the GranBoard really require us to reconnect here?
For the Unicorn board, I believe this won't work, since we are already connected.

If it does we should check here for which board currently is in use, and then only initialize if it's a GranBoard, otherwise skip.

It also seems like this is exactly the same as lines 48:87, so we could also extract those out into a separate method to reduce code duplication

Copy link
Author

Choose a reason for hiding this comment

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

Yes, the majority of the code of the first branch is duplicated here.

Reconnection does not seem necessary, but the listener is also in the initialize function and thus it was the easiest way of establishing this.

It might work with less code, but except for one error thrown because we are already connected the code just works.

It also allows to keep kcapp-smartboard running as a daemon which will connect to new legs and to the board if you switch it on again. No need to launch it for each session.

Copy link
Contributor

Choose a reason for hiding this comment

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

So you need to run the initialize function for each leg on the Granboard? Does it also keep track of legs played and stuff like that and needs a reset?
For the Unicorn Smartboard it just emits whatever you throw, so it doesn´t need to reestablish it. I also don´t currently have easy access to a Unicorn board to test this change, but I'm a bit worried that calling initialize again here might cause issues, since we are already connected and established above.

Regarding reconnecting, this was solved by having the "Reconnect Smartboard" button in the Frontend

  • From a Leg, click "Options" => "Change Order", this will bring up the following dialog
    image
    Here you can click "Reconnect Smartboard" and it should make the request to reconnect the board if it was restarted/disconnected somehow.

This button will only be available if a Venue is selected when starting the match with smartboard = true, so make sure you configure your venues correctly by going to <kcapp>/offices and adding/editing a venue and enabling smartboard
image

Could you see if this works for your case as well?

Copy link
Author

Choose a reason for hiding this comment

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

The reconnect button only triggers the message that the board has already been connected. In fact it is (showing a green light instead of red), but kcapp is not listening for any events from the board and thus not registering them. Besides on a smartphone the Change order menu is not usable as the pop-up is too large to be completely visible. Panning or zooming is also not possible.

Are you sure it works with more than one leg on the Unicorn board? The score is in the object dart which is handled inside the initialize function.

In fact the the if else only handles two options:

  1. No connection has been established and thus scanning and connecting is triggered.
  2. A connection has already been established and scanning and connecting can be omitted.

But in either case you should do the rest. To be honest I thus do not understand the original code, in my opinion there is something missing in the else part.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants