robotpy-wpilib icon indicating copy to clipboard operation
robotpy-wpilib copied to clipboard

SmartDashboard.putData fails with custom Sendable subclasses

Open auscompgeek opened this issue 5 years ago • 10 comments

robot.py:

import wpilib


class Test(wpilib.Sendable):
    def __init__(self):
        super().__init__()
        wpilib.SendableRegistry.getInstance().add(self, "Test", 1)

    def initSendable(self, builder):
        pass


class Robot(wpilib.TimedRobot):
    def robotInit(self):
        self.test = Test()
        wpilib.SmartDashboard.putData("test", self.test)


if __name__ == "__main__":
    wpilib.run(Robot)

Traceback:

Traceback (most recent call last):
  File "~/.local/share/virtualenvs/rpy-dev/lib/python3.8/site-packages/wpilib/_impl/start.py", line 106, in start
    self.robot.startCompetition()
  File "robot.py", line 16, in robotInit
    wpilib.SmartDashboard.putData("test", self.test)
RuntimeError: return_value_policy = copy, but type frc::SendableBuilderImpl is non-copyable!

It looks like it's the call to initSendable that causes this. If the method is deleted:

Traceback (most recent call last):
  File "~/.local/share/virtualenvs/rpy-dev/lib/python3.8/site-packages/wpilib/_impl/start.py", line 106, in start
    self.robot.startCompetition()
  File "robot.py", line 14, in robotInit
    wpilib.SmartDashboard.putData("test", self.test)
RuntimeError: <__main__.Test object at 0x63b424da04a0> does not override required function "Sendable::initSendable"

From the tracebacks I got, it looks like it throws up in frc::SendableRegistry::Publish.

auscompgeek avatar Feb 06 '20 11:02 auscompgeek

I'm quite confused as to what exactly pybind11 is doing here, given that Sendable::InitSendable takes a reference…

auscompgeek avatar Feb 06 '20 11:02 auscompgeek

I managed to get a native code backtrace:

#1  0x00007ffff34266db in pybind11::detail::type_caster_generic::cast (_src=_src@entry=0xf73fa8, policy=policy@entry=pybind11::return_value_policy::copy, parent=..., parent@entry=..., tinfo=tinfo@entry=0xdc1380,
    copy_constructor=0x0, move_constructor=move_constructor@entry=0x0, existing_holder=0x0) at /home/ubuntu/frc/robotpy/venv/lib/python3.8/site-packages/robotpy_build/pybind11/include/pybind11/cast.h:548
#2  0x00007ffff343fced in pybind11::detail::type_caster_base<frc::SendableBuilder>::cast (parent=..., policy=pybind11::return_value_policy::copy, src=0xf73fa8)
    at /home/ubuntu/frc/robotpy/venv/lib/python3.8/site-packages/robotpy_build/pybind11/include/pybind11/cast.h:902
#3  pybind11::detail::type_caster_base<frc::SendableBuilder>::cast (parent=..., policy=pybind11::return_value_policy::copy, src=...)
    at /home/ubuntu/frc/robotpy/venv/lib/python3.8/site-packages/robotpy_build/pybind11/include/pybind11/cast.h:867
#4  pybind11::make_tuple<(pybind11::return_value_policy)1, frc::SendableBuilder&> (args_#0=...) at /home/ubuntu/frc/robotpy/venv/lib/python3.8/site-packages/robotpy_build/pybind11/include/pybind11/cast.h:1802
#5  0x00007ffff35e6228 in pybind11::detail::simple_collector<(pybind11::return_value_policy)1>::simple_collector<frc::SendableBuilder&> (this=0x7fffffffc2e0)
    at /home/ubuntu/frc/robotpy/venv/lib/python3.8/site-packages/robotpy_build/pybind11/include/pybind11/cast.h:1995
#6  pybind11::detail::collect_arguments<(pybind11::return_value_policy)1, frc::SendableBuilder&, void> ()
    at /home/ubuntu/frc/robotpy/venv/lib/python3.8/site-packages/robotpy_build/pybind11/include/pybind11/cast.h:2139
#7  pybind11::detail::object_api<pybind11::handle>::operator()<(pybind11::return_value_policy)1, frc::SendableBuilder&> (this=0x7fffffffc2b0)
    at /home/ubuntu/frc/robotpy/venv/lib/python3.8/site-packages/robotpy_build/pybind11/include/pybind11/cast.h:2159
#8  rpygen::Pyfrc__Sendable<frc::Sendable>::InitSendable (this=0xf2aae0, builder=...) at /home/ubuntu/frc/robotpy/robotpy-wpilib/wpilib/rpy-include/rpygen/frc__Sendable.hpp:27
#9  0x00007ffff3d2c580 in frc::SendableRegistry::Publish (this=this@entry=0x7ffff3f7bd10 <frc::SendableRegistry::GetInstance()::instance>, sendableUid=<optimized out>,
    table=std::shared_ptr<nt::NetworkTable> (use count 3, weak count 0) = {...}) at /__w/1/s/wpilibc/src/main/native/cpp/smartdashboard/SendableRegistry.cpp:311
#10 0x00007ffff3cbdca4 in frc::SmartDashboard::PutData (key=..., data=0xf2aae0) at /__w/1/s/wpilibc/src/main/native/cpp/smartdashboard/SmartDashboard.cpp:101

auscompgeek avatar Feb 06 '20 12:02 auscompgeek

I think pybind11 is trying to take ownership of the reference (~which is actually a C++ local~), I think we would need a proxy of some kind here.

virtuald avatar Feb 15 '21 06:02 virtuald

It is rather odd that pybind11 wants to take a copy of SendableBuilder...

auscompgeek avatar Feb 15 '21 06:02 auscompgeek

Well, after the function is called, what happens to the python reference to SendableBuilder? Since it's a reference, it's potentially not safe for it to continue to exist?

virtuald avatar Feb 15 '21 06:02 virtuald

Actually, that doesn't make sense. Likely what's happening is...

  • It tries to find an existing python object that references the SendableBuilder
  • Doesn't find it, so it looks at the holder and sees it's a shared_ptr. Since it can't just reference it, it has to copy the instance?

.. that is a bit surprising.

virtuald avatar Feb 15 '21 06:02 virtuald

I thought about this some more tonight, and while the behavior I cited before is still what I think is likely happening, I'm no longer surprised by it (though it is surprising!).

The ideal solution would be a pybind11 holder type that could be explicitly disowned. There are some hacky PRs in pybind11, but there's a py::smart_holder coming soon that I'm optimistic will enable this functionality to be done easily with a temporary std::unique_ptr. I think. We'll see?

virtuald avatar Feb 22 '21 06:02 virtuald

Doesn't find it, so it looks at the holder and sees it's a shared_ptr. Since it can't just reference it, it has to copy the instance?

Note that it still tries to copy it when the SendableBuilder* holders aren't shared_ptr.

auscompgeek avatar Feb 22 '21 13:02 auscompgeek

This is reported to still be broken.

virtuald avatar Mar 15 '22 17:03 virtuald

This upstream issue seems related:

  • https://github.com/pybind/pybind11/issues/1241

auscompgeek avatar Mar 20 '22 07:03 auscompgeek