Gamecontroller can Bind to a Different Port Based on Command Line Argument

[Mr-Anyone]

Description

Game controller can now bind on a different port

Testing Done

Ran the following commands:

./tbots.py test kickoff_play_test --gamecontroller_port 50000 --enable_thunderscope
./tbots.py run thunderscope --gamecontroller_port 50001 

and when each of the command from above is running, I ran

ps aux | grep gamecontroller

Resolved Issues

resolves #3160

Length Justification and Key Files to Review

N/A

Review Checklist

It is the reviewers responsibility to also make sure every item here has been covered

• Function & Class comments: All function definitions (usually in the .h file) should have a javadoc style comment at the start of them. For examples, see the functions defined in thunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.
• Remove all commented out code
• Remove extra print statements: for example, those just used for testing
• Resolve all TODO's: All TODO (or similar) statements should either be completed or associated with a github issue

[nimazareian]

I just tried this, and I'm not sure if it works as expected. Trying to launch 2+ concurrent AI vs AIs causes a crash due to a port binding error.

In the example you've listed in your PR description, since one of the targets you're running is running with bazel test with the "exclusive" tag added to its build file, the test actually ends up finishing before thunderscope is launched. A longtime issue we've had is actually #2619 which is similar to the problem I've described.

This is somewhat of a different, but very related, problem to #3160. Above mainly discusses an issue where we all of our simulated pytests need to have the "exclusive" tag to avoid having multiple of them running concurrently and the gamecontroller port being busy (by default Bazel runs tests in parallel). Fixing this issue will likely help speed up our CI/CD workflow for simulated pytests.

On the other hand, #3160 discusses the problem we faced during Robocup where most teams connect their computer to the same network which has access to the internet. Furthermore, during a game or field testing, all teams on the field connect to the same LAN. Our goal is to basically almost always launch the gamecontroller on some non-official port/multicast IP address that other teams or other members of our team aren't listening to for gamecontroller commands. This will allow us to send gamecontroller commands that only effects the fullsystem being ran locally. This should be the case UNLESS we're playing an actual game where we're not launching our own gamecontroller locally and we're using the provided field gamecontroller. Hence #3161 discussing a new "robocup" mode used during actual games with a non-local gc. To achieve this, we will probably need to update RobotCommunication aswell to not always listen to the official gamecontroller/referee port/IP:

Software/src/software/thunderscope/robot_communication.py

Lines 114 to 119 in aaa16cb

	self.receive_ssl_referee_proto = tbots_cpp.SSLRefereeProtoListener(
	SSL_REFEREE_ADDRESS,
	SSL_REFEREE_PORT,
	lambda data: self.current_proto_unix_io.send_proto(Referee, data),
	True,
	)

Let me know if this doesn't make sense or you want any clarifications. Tagging @itsarune in case I've missed anything.

On a separate note, could you also update this socket connection to have a more descriptive error message:

Software/src/software/networking/ssl_proto_communication.py

Lines 37 to 39 in 1528173

	# bind to all local interfaces, TCP
	self.socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
	self.socket.connect(("", port))

For example:

        try:
            # bind to all local interfaces, TCP
            self.socket = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
            self.socket.connect(("", port))
        except ConnectionRefusedError:
            raise ConnectionRefusedError(
                f"SSL Socket connection refused on port {port}. Is binary already running in a separate process?"
            )

[itsarune]

Suggested change

	referee_addresse: int = None,
	referee_address: int = None,

[Mr-Anyone]

done.

[itsarune]

bump: looks like it didn't get committed

[itsarune]

@nimazareian covered everything, I don't have anything more to add onto nima's conversation

[itsarune]

REFEREE_IP is supposed to be a constant, we shouldn't be modifying it inside here

[Mr-Anyone]

On the other hand, #3160 discusses the problem we faced during Robocup where most teams connect their computer to the same network which has access to the internet. Furthermore, during a game or field testing, all teams on the field connect to the same LAN. Our goal is to basically almost always launch the gamecontroller on some non-official port/multicast IP address that other teams or other members of our team aren't listening to for gamecontroller commands. This will allow us to send gamecontroller commands that only effects the fullsystem being ran locally. This should be the case UNLESS we're playing an actual game where we're not launching our own gamecontroller locally and we're using the provided field gamecontroller. Hence #3161 discussing a new "robocup" mode used during actual games with a non-local gc. To achieve this, we will probably need to update RobotCommunication aswell to not always listen to the official gamecontroller/referee port/IP

Doesn't this contradicts what nima said? Am I misunderstanding something? Are we setting the port instead then but keeping the multicast address constant?

[itsarune]

Nima's right but in the implementation, don't modify REFEREE_IP. That is a constant (which is why it's all capitalized) we use during real games for connecting to the Gamecontroller and we should leave it so that it always point to that ip address. By convention, we never modify constants during runtime.

Instead, use a new local variable:

Suggested change

	self.REFEREE_IP = referee_addresse
	if referee_addresse is None:
	self.REFEREE_IP = "224.5.23.1"
	multicast_address = referee_address if referee_address else Gamecontroller.REFEREE_IP

[Mr-Anyone]

Shouldn't this be self.multicast_address instead of multicast_address, since we are accessing this variable in line 180?

[itsarune]

Yeah, you're correct

[itsarune]

good start! left a few comments to try to clean up the logic a bit

[itsarune]

Suggested change

	def get_referee_port_staticmethod(gamecontroller: Gamecontroller):
	def get_referee_port(gamecontroller: Gamecontroller):

it doesn't need to be called staticmethod

[Mr-Anyone]

we kind of need a static method of a function. I feel like a static method would make more sense. see the comments below for more justification.

[itsarune]

why not make it a class function?

Suggested change

	def get_referee_port_staticmethod(gamecontroller: Gamecontroller):
	def get_referee_port(self):

[itsarune]

oh there already is a get_referee_port. why do we have this function at all?

[Mr-Anyone]

This function is used situation like the following:

https://github.com/Mr-Anyone/Thunderbot_Software/blob/cf668bfeaff698097aadce1d01c60b4c731322c6/src/software/thunderscope/thunderscope_main.py#L322.

In otherwords, the instance of gamecontroller has type None, but we want a function or staticmethod to return the default port of the referee.

Since the gamecontroller has type None, we cannot call self.get_referee_port, but Gamecontroller.get_referee_port_staticmethod instead. In other words, if the gamecontroller has type None, we want to listen to the default port.

[nimazareian]

Hmm interesting. In that case I personally would say Gamecontroller.get_referee_port_static might be a more suited name.

[Mr-Anyone]

I have changed the name.

[itsarune]

awesome work! just a minor nit

[nimazareian]

Could you run fix_formatting.sh locally, and if it failed to format python files, could you try running our python formatter black manually? You can find the command for it in the formatting script.

[nimazareian]

Hmm interesting. In that case I personally would say Gamecontroller.get_referee_port_static might be a more suited name.

[Mr-Anyone]

I've just ran fix_formatting.sh. It seems that my system is missing an dynamic library libtinfo-dev. This is solved through apt-get.

[nimazareian]

This is a duplicated call (same as line 40)

[nimazareian]

Also, locally I tried updating this line so we also use a random port for this as well, but I'm not sure why I'm still unable to to launch two Thunderscopes at once: ./tbots.py run thunderscope --disable_communication --run_diagnostics --run_blue --interface eno1 --launch_gc

This was mainly out of curiosity and isn't something we need to be able to achieve.

[Mr-Anyone]

done.

[nimazareian]

Good work wrapping this up! 💯

[itsarune]

for the future: the better way is to use the python ternary operator:

Suggested change

	referee_port=Gamecontroller.get_referee_port_static(gamecontroller),
	referee_port=gamecontroller.get_referee_port() if gamecontroller else REFEREE_PORT,

why?
because if you're reading this code, there's an extra level of indirection where you have to peruse the implementation of the Gamecontroller module to figure out the REFEREE_PORT whereas it's very clear which referee port we bind to in the suggested code. We don't use get_referee_port_static enough around our codebase for this pattern to simplify the code

[itsarune]

I'm not going to block merging for this change since you'll lose your approvals but keep it in mind for the future

[Mr-Anyone]

This is actually life changing.

I think a lot of the time I need to do something like that, but I just don't know that feature well enough to use it. we could get rid of this weird static method.

[Mr-Anyone]

Also, a lot of the time, I want initialize a variable when the being passed into the function is None.

[itsarune]

Suggested change

	parser.error("--launch_gc has to be ran with --run_blue argument")
	parser.error("--launch_gc has to be ran with --run_blue or --run_yellow argument")

[itsarune]

go ahead and merge, you can address my nits another time

[Mr-Anyone]

I think the refactor of Gamecontroller.get_referee_port_staticmethod could be a good first issue.