-
Notifications
You must be signed in to change notification settings - Fork 677
[robotpy] Try to fix opmodes #8498
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
base: 2027
Are you sure you want to change the base?
Conversation
(cherry picked from commit d022ef210f38527bd70d3c487c4a9903f164fb6e) (cherry picked from commit 2def1eb588c191a5c6621e74175c15f17bc009cf)
|
Note I now have this stacked on #8500. They can be landed together here or in separately one at a time |
| cpp_code: | | ||
| []() { | ||
| OpModeOptions rawOptions = DriverStationSim::GetOpModeOptions(); | ||
| std::vector<HAL_OpModeOption> options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this is more for your education than a change we should make here, but over time I've moved towards creating python list objects directly instead of using std::vector because it avoids doing a second set of copies.
See https://github.com/robotpy/mostrobotpy/blob/80bffec30fe78a274e362c92a566e79271c1b8bc/subprojects/robotpy-wpiutil/wpiutil/src/wpistruct/wpystruct_fns.cpp#L136 for what I mean. Note that the function returns py::typing::List<Type> which ensures a nice type signature in autocomplete. Just returning a py::list wouldn't tell the user the type of the element (but just returning the std::vector does because the type caster will take care of that aspect of it.
I don't know how often we expect this to get called so it probably doesn't matter.
| for (auto opmode : rawOptions) { | ||
| HAL_OpModeOption copy{ | ||
| .id = opmode.id, | ||
| .name = wpi::util::copy_wpi_string(opmode.name), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to leak memory because there's no destructor for HAL_OpModeOption?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not directly related to this, but we have a leak in the JNI wrappers for WPILib as well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Realistically, are the python level strings valid through the entire call, and utf8? If they are, you can make the wpi_strings in this be a view over the python strings, rather than a copy. If not then yeah you'll need to free afterwards.
wpilibc/src/main/python/semiwrap/simulation/DriverStationSim.yml
Outdated
Show resolved
Hide resolved
Co-authored-by: David Vo <[email protected]>
I took a stab at trying to fix the stubgen problem for the opmode addition. I essentially "started over" when it came to all the python stuff, and tried to make small commits that @virtuald could look at.
I started at
44cf645632, as this was the last commit that was recently synced up withmostrobotpy.I did a poor mans
rebase -iby cherry-picking all of the non opmode commits since then:I then did a hacky "partial merge" where I grabbed all of the opmode changes excluding the vast majority of
robotpychanges. There are a coulple of robotpy changes, like the pure python OpModeRobot and some bug fixes, but overhwelmingly it definitly "reverts" the yaml changesThe next commit runs
scan-headersandupdate-yamlAS IS, to give a base for what the C++ changes were.One of the core problems with the wrapper is that there is both a
HAL_ControlWord(which gets trimmed toControlWordfor the python binding) and awpi::hal::ControlWord(which also would normally get trimmed toControlWordfor the binding). The same is also trueHAL_RobotMode / wpi::hal::RobotMode. Unfortunately both versions are exported as part of the interfaces as currently written, so it isn't as trivial as just ignoring generating bindings for them without massive cascading effects. I found a way to rename them and then a lot of stuff just seemed to work ™️Another one of the big problems is that
HAL_OpModeOptionusesWPI_Stringas a member type. This fix involved ignoring + adding aproperty_readonlyfor those fields, and creating an inline copy function for GetOpModeOptionsA lot of the "trivial" fixes like ignoring the "Register..Callback" functions or adding
extra_includesremain the same as what is already landed on 2027, but I was able to get away with ignoring less of the bindingsThese changes should allow the stubgen process to actually build now. I tested it with a local bazel-ified version of what
mostrobotpydoes that is hacky and not landable, but was first able to reproduce the failures I'm seeing inmostrobotpy, as well as them going away with these changes.I also added a unit test that matches the C++ one and excercies some of the more annoying fixes, so at the very least that is an improvement.