robotpy / robotpy-wpilib-utilities

Useful utility functions/objects for RobotPy
BSD 3-Clause "New" or "Revised" License
11 stars 19 forks source link

magicbot: Allow typed feedbacks using return type hints #208

Closed auscompgeek closed 1 month ago

auscompgeek commented 7 months ago

This inspects the return type hint on @feedback methods to determine what topic type they should publish. This allows:

If the types are incompatible at runtime, the robot code will crash, e.g. in tests:

_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
../../../.venv/lib/python3.11/site-packages/pytest_reraise/reraise.py:72: in __call__
    raise e
../../../pyfrc/pyfrc/test_support/controller.py:40: in _robot_thread
    robot.startCompetition()
../../../robotpy-wpilib-utilities/magicbot/magicrobot.py:370: in startCompetition
    self._disabled()
../../../robotpy-wpilib-utilities/magicbot/magicrobot.py:456: in _disabled
    self._do_periodics()
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <robot.MyRobot object at 0x10687a0f0>

    def _do_periodics(self) -> None:
        """Run periodic methods which run in every mode."""
        watchdog = self.watchdog

        for method, setter in self._feedbacks:
            try:
                value = method()
            except:
                self.onException()
            else:
>               setter(value)
E               TypeError: set(): incompatible function arguments. The following argument types are supported:
E                   1. (self: ntcore._ntcore.IntegerPublisher, value: int, time: int = 0) -> None
E
E               Invoked with: <ntcore._ntcore.IntegerPublisher object at 0x1019ce5f0>, 0.0

../../../robotpy-wpilib-utilities/magicbot/magicrobot.py:729: TypeError

Returning a non-homogeneous tuple will result in the same error message as previously, for example:

Traceback (most recent call last):
  File "/home/davo/dev/frc/robotpy/.venv/lib64/python3.10/site-packages/wpilib/_impl/start.py", line 247, in _start
    self.robot.startCompetition()
  File "/home/davo/dev/frc/robotpy/robotpy-wpilib-utilities/magicbot/magicrobot.py", line 370, in startCompetition
    self._disabled()
  File "/home/davo/dev/frc/robotpy/robotpy-wpilib-utilities/magicbot/magicrobot.py", line 456, in _disabled
    self._do_periodics()
  File "/home/davo/dev/frc/robotpy/robotpy-wpilib-utilities/magicbot/magicrobot.py", line 729, in _do_periodics
    setter(value)
ValueError: Can only put bool/int/float/str/bytes or lists/tuples of them

Locals at innermost frame:

{ 'method': <bound method ChassisComponent.get_module_positions of <components.chassis.ChassisCom...
  'self': <robot.MyRobot object at 0x7f7fc3304cc0>,
  'setter': <bound method PyCapsule.setValue of <ntcore._ntcore.NetworkTableEntry object at 0x7f7...
  'value': ( SwerveModulePosition(distance=0.000000, angle=-1.570796),
             SwerveModulePosition(distance=0.000000, angle=-1.570796),
             SwerveModulePosition(distance=0.000000, angle=-1.570796),
             SwerveModulePosition(distance=0.000000, angle=-1.570796)),
  'watchdog': <robotpy_ext.misc.simple_watchdog.SimpleWatchdog object at 0x7f7fc3908910>}

To appease type checkers without majorly refactoring the magic_tunable module further, this also adds support for int and struct tunables. This fixes the inferred type being wrong for tunable(1) (it claimed it was an int tunable, when reading it gave floats instead). Tunables with empty array defaults using type hints will be supported in a later PR.

I recommend reviewing this PR commit by commit.

auscompgeek commented 2 months ago

@virtuald I mostly want to check, is checking hasattr WPIStruct the intended way to check whether a type can be used as a struct topic/log type?

virtuald commented 2 months ago

Yes that is the intent, https://github.com/robotpy/mostrobotpy/blob/main/subprojects/robotpy-wpiutil/wpiutil/wpistruct/dataclass.py#L130 does that, and https://github.com/robotpy/mostrobotpy/blob/868f50a724399cfdb3cc45b0067bbb20e9e7e401/subprojects/robotpy-wpiutil/wpiutil/src/wpistruct/wpystruct.h#L230

auscompgeek commented 2 months ago

Hm, okay. Personally I expected this to be an implementation detail hidden away by functions, like how dataclasses has is_dataclass and fields functions.

auscompgeek commented 1 month ago

Brief testing with some additional @magicbot.feedback in my team's code uncovered I hadn't handled the case of things like a return type hint of a 4-tuple (of a single type). Accounted for that too with a test.