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

magicbot: Allow typed feedbacks using return type hints #208

Merged
merged 11 commits into from
Aug 7, 2024

Conversation

auscompgeek
Copy link
Member

@auscompgeek auscompgeek commented Jan 26, 2024

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

  • publishing integers to NetworkTables instead of floating point numbers (previously not possible due to the backwards compatibility in pyntcore's setValue method preferring to upcast integers to doubles),
  • publishing empty arrays to NetworkTables,
  • publishing structs and struct arrays to NetworkTables (not supported by setValue), and
  • deterministically publishing integer or double arrays to NetworkTables (currently setValue always uses the type of the first element).

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.

This allows using feedbacks to create integer and integer array topics.
@auscompgeek auscompgeek changed the title magicbot: Allow integer feedbacks magicbot: Allow typed feedbacks using return type hints Jun 23, 2024
@auscompgeek
Copy link
Member Author

@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?

@auscompgeek
Copy link
Member Author

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 auscompgeek added the magicbot magicbot package label Jul 27, 2024
from ntcore.types import ValueT


class StructSerializable(typing.Protocol):
Copy link
Member Author

Choose a reason for hiding this comment

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

This protocol probably shouldn't belong here...

Copy link
Member

Choose a reason for hiding this comment

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

Probably should end up in wpimath, but I don't see why it can't be in two places.

Copy link
Member Author

Choose a reason for hiding this comment

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

I presume you mean wpiutil? I'll follow up with a PR over there then.

Copy link
Member

@virtuald virtuald left a comment

Choose a reason for hiding this comment

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

Given there are tests and it works, this seems fine to me.

@auscompgeek
Copy link
Member Author

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.

@auscompgeek auscompgeek merged commit 66e5424 into main Aug 7, 2024
18 checks passed
@auscompgeek auscompgeek deleted the typed-feedback branch August 7, 2024 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
magicbot magicbot package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants