Add Xbox controller support to Thunderscope #3064
Conversation
…into boris/shoot_or_pass_play_fsm_guards � Conflicts: � src/software/jetson_nano/redis/redis_client_test.cpp
…nderloop-safety-updates-v2
…oftware into thunderloop-safety-updates-v2
…most of the manager and overall structure done
|
you can tag issue #2562 |
src/software/thunderscope/robot_diagnostics/handheld_device_manager.py
Outdated
Show resolved
Hide resolved
src/software/thunderscope/robot_diagnostics/handheld_device_manager.py
Outdated
Show resolved
Hide resolved
src/software/thunderscope/robot_diagnostics/handheld_device_manager.py
Outdated
Show resolved
Hide resolved
|
|
||
| def __init__( | ||
| self, | ||
| logger: Logger, |
There was a problem hiding this comment.
I don't think it makes much sense to pass logger as a parameter into a class. HandheldDeviceManager shouldn't depend on logger.
This is what people usually do instead:
https://stackoverflow.com/questions/18052778/should-a-python-logger-be-passed-as-parameter#:~:text=Not%20usually%3B%20it%20is%20typically,is%20different%20for%20each%20module.
Are you trying to log to the same file as DiagnosticsWidget?
There was a problem hiding this comment.
People tend to make logger a global variable I think.
There was a problem hiding this comment.
Yeah we need to completely redo our logging in python. Boris added these logging changes and I didn't take them out. I'm thinking we should use https://github.com/Delgan/loguru
| """Loop that continuously reads and processes events from the connected handheld device.""" | ||
| while True: | ||
| with self.lock: | ||
| self.__read_and_process_event() |
There was a problem hiding this comment.
I am not sure if this is going to be memory safe or what not.
Why not put self.lock in the __read_and_processe_event() instead of here? I think it would yield the same result. Are you trying to make the code look prettier so it doesn't have mutiple indent?
There was a problem hiding this comment.
Yeah it's just for readability so we don't have an extra indent in __read_and_process_event
|
|
||
| for device in list_devices(): | ||
| handheld_device = InputDevice(device) | ||
| if ( |
There was a problem hiding this comment.
During the test with Arun, we encountered an issue where handheld_device.name was not present in device_config_map. This led to some confusion among users regarding the status of the controller.
To improve clarity, could we add a debug log message indicating that a device is found but is not supported? This would help users understand the situation better.
There was a problem hiding this comment.
alternatively, we could strip whitespace and match lowercase controller names to stop these kinds of issues
| ) | ||
| ) | ||
|
|
||
| def __event_loop(self) -> None: |
There was a problem hiding this comment.
Just some thoughts you could ignore and mark as resolve:
I've been reflecting on our code structure for a while. We have several threads, each running its own event loop. The problem is that these threads don’t yield control to one another because we are running a while True: loop. This design doesn’t scale well with many asynchronous tasks since we only need to call read_and_process_event when there's an actual event to process. As a result, we waste resources and compute time by constantly checking for read events.
We might consider using asyncio or a similar approach to handle these events more efficiently. This way, we can reduce the resources spent on checking for read conditions. Although, this might be a good idea, I feel likle it might but too much work for us to rewrite the full IO stack. The performance improvement may not justifiy the need.
There was a problem hiding this comment.
Async would work well in this case. But for Thunderscope in general, I'm not sure there will be a significant performance improvement from switching to async to avoid busy waiting (it actually may add overhead and slow things down) since most of our tasks are either CPU bound or just don't sit around spinning.
e.g. in ProtoPlayer the playback worker thread probably spends way more time unzipping the replay chunks vs. waiting on file I/O.
In RobotCommunication the thread that sends primitives pretty much always has new primitives to process, so that thread is always working (except for a bit of waiting on network I/O).
So converting the work these threads perform into async tasks would just move a bunch of heavy processing to the Qt thread and end up blocking the UI.
Also, right now we don't have enough tasks to warrant using async for scalability benefits (we only have a single thread in RobotCommunication that sends primitives, unlike a web server that might handle hundreds of concurrent network requests).
|
I will take a look of the rest of thing later. Need to work on mini project for CPEN 221. |
| HANDHELD_DEVICE_NAME_CONFIG_MAP = { | ||
| "Microsoft Xbox One X pad": XboxConfig, | ||
| "Microsoft X-Box One S pad": XboxConfig, | ||
| "Microsoft Xbox 360 pad": XboxConfig, |
There was a problem hiding this comment.
Add the following line: Microsoft Xbox One S pad": XboxConfig. This was there before, and I am pretty sure this is a supported device that Boris tested.
There was a problem hiding this comment.
nah I accidently changed it from Microsoft X-Box One S pad to Microsoft Xbox One S pad because I didn't realize the actual device names were listed in this map (i thought it was just a typo)
There was a problem hiding this comment.
I see. what a circular issues I have traced with Arun.
| dribbler_enable=DeviceAbsEvent(event_code=5, max_value=1023.0), | ||
| ) | ||
|
|
||
| HANDHELD_DEVICE_NAME_CONFIG_MAP = { |
There was a problem hiding this comment.
Now that I think about it, does it make sense to even make sense to have this CONFIG_MAP. The only configruation we currently have is XboxConfig.
We could literally just have a list and call it supported_deviced and initialize this XboxConfig if the device we are looking at is in this list.
Are we even going to support different controller in the future?
There was a problem hiding this comment.
This is probably a YAGNI moment. But I'll leave the config map as is bc it's already implemented
There was a problem hiding this comment.
I imagine at some point we might want to add a generic controller or a play station controller or something, so may as well
| time.sleep(DiagnosticsConstants.EVENT_LOOP_SLEEP_DURATION) | ||
|
|
||
| def __read_and_process_event(self) -> None: | ||
| """Try reading an event from the connected handheld device and process it.""" |
There was a problem hiding this comment.
this function should be documented as not being thread-safe (requires mutex locking)
|
Just completed a major refactor of the code
|
…into pr/Bvasilchikov/3064
…into pr/Bvasilchikov/3064
Description
This PR adds XBox controller support to Thunderscope.
Work Done
evdevAPI using a python packageRobotCommunicationto handle using GUI or controller input values and passing that through to the robotsTesting (Needed To Be) Done
MANUALcontrol is set andXboxis toggled in diagnostics.MANUALandXboxResolved Issues
Review Checklist
It is the reviewers responsibility to also make sure every item here has been covered
.hfile) should have a javadoc style comment at the start of them. For examples, see the functions defined inthunderbots/software/geom. Similarly, all classes should have an associated Javadoc comment explaining the purpose of the class.TODO(or similar) statements should either be completed or associated with a github issue