Planning refactors by RCoder01 · Pull Request #116 · MRoboSub/mrobosub

RCoder01 · 2025-09-24T23:49:04Z

This PR contains a lot of semi-independent refactors, so I'd like all yall's opinions on which are good and which are not.

Unrelated to planning:

Remove out monitoring from launch files
Add some more type hinting to mrobosub_lib to appease mypy
Remove unused args param from localization main (why was it there to begin with?)
Run the autoformatter on some files

mrobosub_planning changes (not necessarily exhaustive, please read through the diff):

Move a bunch of stuff from typing types to built-in types (typing.Type to type, typing.Union[A, B] to A | B, typing.Optional[T] to T | None, etc.)
- Possible because ros2 uses more modern python versions
Refactor some of the abstract_state hierarchy to remove duplicate logic between states
Update state doc comments to reflect current requirements/usage
Add time and state_runtime methods to TimedState so we don't have to keep writing self.io.node.get_clock().now().nanoseconds / 1e9
periodic_io.Interface
- Rename periodic_io.Captain to periodic_io.Interface and make it take in a Node in the constructor instead of being a node itself
- Make a Captain node class in captain.py
- Rename State.io_node to State.io to reflect that Interface is not a node
  - If states need to access the node, they can do so with self.io.node
- Update captain main function to reflect changes to the node
Add pitch stuff to periodic_io
- idk why it wasn't there in ros1 code but added for completeness (even though we never use it in practice)
Update zed/bot_cam service calls in Interface to use simpler, synchronous calls
- I'm like 99% sure it's equivalent to creating a call_async future and immediately calling rclpy.spin_until_future_complete
- I wanted to factor out activate_bot_cam, activate_zed, deactivate_cameras into one like set_cameras(bot_cam_enable: bool, zed_enable: bool) that each of the three called but the order in which activate_zed/bot_cam call the services is opposite (better to disable the other camera first, before enabling to ensure bandwith is never overloaded)
renames periodic_io.py to io_interface.py

imzaynb · 2025-10-07T23:45:35Z

mrobosub_planning/mrobosub_planning/umrsm.py

It looks like most of the changes to this file are typing changes now that we use Python 3.12. As well as the change from io_node to io holding a node as a member variable.

imzaynb · 2025-10-07T23:46:14Z

mrobosub_planning/mrobosub_planning/io_interface.py

+    """
+
+    def __init__(self, node: Node):
+        self.logger = node.get_logger()


Great change!!

imzaynb · 2025-10-07T23:52:12Z

mrobosub_planning/mrobosub_planning/io_interface.py

I support the name change from periodic_io.py --> io_interface.py.

This file changes from being a node to having a node passed into it. It also adds pitch because it wasn't there before.

mrobosub_planning/mrobosub_planning/captain.py

imzaynb · 2025-10-07T23:58:02Z

mrobosub_planning/mrobosub_planning/abstract_states.py

Other than overhauling DoubleTimedState, changes to this file are mainly updating instance of io_node --> io.node and updating the typing to be consistent with Python 3.12.

DoubleTimedState didn't actually change all that much; ForwardAndWait changed from being a subclass of TimedState to being a subclass of DoubleTimedState, which significantly simplified its implementation. To reflect this change, I swapped the order of the two classes in source which confused git, so it looks like both classes changed significantly.

imzaynb

Looks good. Though I do want @alexanderbowler to look at these changes before we merge since he was in charge of migrating this from ROS1.

HenryLeC · 2025-10-08T18:29:25Z

mrobosub_planning/mrobosub_planning/umrsm.py



-TransitionMap = Dict[Type[Outcome], Type[State]]
+TransitionMap = dict[type[Outcome], type[State]]


Is this the recommended way to type python types, I always used tje upercase typing version instead of the lowercase python version

Uppercase version is from the typing module, lowercase just uses the built-in types. There's no longer any reason to import the uppercase names in recent python versions because the built-in types now support the type[generic] syntax.

https://docs.python.org/3/library/typing.html#typing.Dict

"Deprecated since version 3.9"

muskaan-mittal · 2025-10-21T23:39:34Z

mrobosub_planning/mrobosub_planning/captain.py

+                rate.sleep()
+
+
+def main() -> None:


does main return None? doesn't it return an int

This function doesnt?

main returns whatever you want in python

muskaan-mittal · 2025-10-21T23:44:56Z

mrobosub_planning/mrobosub_planning/common_states.py

+    def __init__(self, prev_outcome: Outcome, io: Interface):
+        super().__init__(prev_outcome, io)
+        self.io.reset_target_twist()
+        self.rate = self.io.node.create_rate(50)


i am again unsure about the use of rate here

I think in this case it is fine because stop is special, in a general state we definitely do not want to rate sleep tho

muskaan-mittal · 2025-10-21T23:52:15Z

mrobosub_planning/mrobosub_planning/umrsm.py


        Returns the Outcome from calling handle() on StopState.
        """
        rate = self.node.create_rate(hz)


i dont think rate can be used here

I think in this case it is fine because of the difficulty of refactoring to not use sleep and there is a separate thread spinning in captain.py

mrobosub_planning/mrobosub_planning/captain.py

HenryLeC

LGTM!

HenryLeC · 2025-11-18T23:54:31Z

mrobosub_planning/mrobosub_planning/captain.py

+                rate.sleep()
+
+
+def main() -> None:


main returns whatever you want in python

HenryLeC · 2025-11-19T00:05:09Z

mrobosub_planning/mrobosub_planning/common_states.py

+    def __init__(self, prev_outcome: Outcome, io: Interface):
+        super().__init__(prev_outcome, io)
+        self.io.reset_target_twist()
+        self.rate = self.io.node.create_rate(50)


I think in this case it is fine because stop is special, in a general state we definitely do not want to rate sleep tho

mrobosub_planning/mrobosub_planning/io_interface.py

HenryLeC · 2025-11-19T00:23:00Z

mrobosub_planning/mrobosub_planning/umrsm.py


        Returns the Outcome from calling handle() on StopState.
        """
        rate = self.node.create_rate(hz)


I think in this case it is fine because of the difficulty of refactoring to not use sleep and there is a separate thread spinning in captain.py

RCoder01 added 2 commits September 24, 2025 19:47

Planning refactors

7a47e61

type checking

c125522

Base automatically changed from ros2 to devel October 5, 2025 17:12

Merge branch 'devel' into planning_refactors

bc739d4

RCoder01 requested review from HenryLeC, alexanderbowler, imzaynb and muskaan-mittal October 5, 2025 21:41

imzaynb reviewed Oct 7, 2025

View reviewed changes

Merge branch 'devel' into planning_refactors

547d072

imzaynb reviewed Oct 7, 2025

View reviewed changes

mrobosub_planning/mrobosub_planning/captain.py Outdated Show resolved Hide resolved

imzaynb reviewed Oct 7, 2025

View reviewed changes

remove type ignore (mypy is fixed)

4535410

imzaynb self-requested a review October 7, 2025 23:59

imzaynb previously approved these changes Oct 8, 2025

View reviewed changes

update comment for ros2

99d1b5c

RCoder01 dismissed imzaynb’s stale review via 99d1b5c October 8, 2025 00:02

HenryLeC reviewed Oct 8, 2025

View reviewed changes

RCoder01 added 3 commits October 9, 2025 15:10

Rename periodic_io

c4564bb

Update typing

672dc80

Update launch files

bbda80d

muskaan-mittal requested changes Oct 21, 2025

View reviewed changes

alexanderbowler previously approved these changes Oct 22, 2025

View reviewed changes

mrobosub_planning/mrobosub_planning/captain.py Outdated Show resolved Hide resolved

Add fix to shutdown via a timer

bf2f475

alexanderbowler dismissed their stale review via bf2f475 October 22, 2025 00:48

alexanderbowler and others added 3 commits October 21, 2025 20:53

fix mypy

dbd1f24

Merge branch 'devel' into planning_refactors

4510a6b

Remove timeouts on service calls (they dont exist in jazzy)

860b68d

HenryLeC approved these changes Nov 19, 2025

View reviewed changes



		TransitionMap = Dict[Type[Outcome], Type[State]]
		TransitionMap = dict[type[Outcome], type[State]]

Conversation

RCoder01 commented Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imzaynb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RCoder01 Oct 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

HenryLeC left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

RCoder01 commented Sep 24, 2025 •

edited

Loading

RCoder01 Oct 8, 2025 •

edited

Loading