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

🐛 Phantom Multiple Port Selection #255

Merged
merged 3 commits into from
Jan 19, 2024

Conversation

kunwarsahni01
Copy link
Member

There is an issue where sometimes the CLI thinks that there are multiple V5 Brains plugged in and prompts the user to choose one port when there is only one actual Brain plugged in. This fix checks an edge case in the CLI to confirm that any Brains are not accidentally marked as controllers and should fix the multiple Brains issue. This will require actual in depth testing with hardware including Brains and Controllers, across all platforms (along with multiple Brains and Controllers plugged in and not plugged in).

@AlexHunton2
Copy link
Contributor

Currently looking into through #248. Identified source of the problem at same place you did. Have yet to hardware test to find the necessary information contained in port info to identify different pieces of hardware. Furthermore, different operating systems possibly have different information that they are working with (Function would return 2 ports as joystick on my mac, yet 1 system, 1 user for windows). I was hoping to find port info that would identify the machine better than looking at naming/descriptions as those seem like they can change in unexpected ways. Were you able to identify those?

@kunwarsahni01
Copy link
Member Author

I'm not really sure what is a better way to identify the devices but from my understanding it looks like all the port logic was written from a windows perspective with edge cases tacked on to handle the other OSes, maybe we need to look at seperation. In MacOS land at least this could be done so much easier as the device info clearly states VEX and V5.

@AndrewLuGit
Copy link
Contributor

I added another commit to this branch to implement the port detection logic I described for macOS, as well as making it work better on my computer.

@AlexHunton2
Copy link
Contributor

2023 Current Update of Port Detection Issues

January 25th, 2023 by Alex Hunton

Overall Problem

"Brain and controllers are often not detected through ports. Something is wrong with the way we detect if a port is a vex device or not. I have seen examples where irrelevant arguments (such as --slot) have affected whether or not the brain is detected by the CLI or not.
If a user is connected to ONE brain or controller through a USB hub, the CLI will still prompt them for a port even though only one device is connected
If multiple devices are connected, the prompt displayed is really strange. Sometimes it has the user only able to choose one port. Sometimes the user has no ports to choose. Sometimes it displays the same port multiple times."
-Ben #248

Problem List

    1. One Brain -> Multiple Ports Given
    1. One Brain -> None Ports Given
    1. One Brain -> Duplicate Ports
    1. Multiple Brains (devices) - > Strange Prompt
    1. General MacOS Shenanigans with Ports

Source of the Problem:

pros-cli/pros/serial/devices/vex/v5_device.py

Solution 1:

Attempts to solve problems 1 through 3
Identifies ports with system p_type and looks at the description to see if "Brain" is at all contained.

elif p_type.lower() == 'system':

	# check if ports contain the word Brain in the description and return that port
	for port in ports:
		if "Brain" in port.description:
			return [port]
	return [ports[0], *joystick_ports]

-Kunwar (55593a3)

Solution 2:

Functionality for Mac, since Mac has special case where detection of Brains fall into to the joystick category for whatever reason, this adds a special filter for that case.

def filter_v5_ports_mac(p, device):

	return (p.device is not None and p.device.endswith(device))


# Special logic for macOS
if platform.system() == 'Darwin':

	user_ports = [p for p in ports if filter_v5_ports_mac(p, '3')]
	system_ports = [p for p in ports if filter_v5_ports_mac(p, '1')]
	joystick_ports = [p for p in ports if filter_v5_ports_mac(p, '2')]

else:

	user_ports = [p for p in ports if filter_v5_ports(p, ['2'], ['User'])]
	system_ports = [p for p in ports if filter_v5_ports(p, ['0'], ['System', 'Communications'])]
	joystick_ports = [p for p in ports if filter_v5_ports(p, ['1'], ['Controller'])]
# Fallback for when a brain port's location is not detected properly

if len(user_ports) != len(system_ports):

	if len(user_ports) > len(system_ports):
		system_ports = [p for p in ports if p not in user_ports and p not in joystick_ports]
	else:
		user_ports = [p for p in ports if p not in system_ports and p not in joystick_ports]

-Andrew (2a0b85d)

Solution 3:

Fixes to click prompting by listing all ports. Should help solve Problem 2.
-Andrew (9c00498)

How Effective Are These Solutions?

  • Testing with one device on my Mac problems 1, 2, 3, and 5 are solved. However, the code is a complete mess and the solutions available are difficult to understand.

The Better Solution?

  • Separate everything into files for device specific operating systems so that port detection is handled for the case by case basis of the different operating systems.
  • Should allow for specific edge cases that are specific to the operating system to be locally understood in the file.
  • Major Con: A lotta work

Copy link
Member

@BennyBot BennyBot left a comment

Choose a reason for hiding this comment

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

Since this fixes a majority of cases, we will use it for now. Investagtion of multiple brains connected will start in the future.

@Jackman3323
Copy link

Tested on M2 and intel, works as advertised on both, no issues.

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.

6 participants