Skip to content

add trapezoidal profile tests #174

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

LucienMorey
Copy link
Contributor

I was going mad playing with things to get state space stuff to work, and ported these to confirm that I am the problem. Fair warning that I just sent it into chat gpt and then roughly checked the logic for a few of the tests to make sure it seemed reasonable. It seems like it did a reasonable job spitting everything out with the right casing and the like but give me a yell if I need to change anything or it just needs to go in the bin.

@LucienMorey LucienMorey force-pushed the add_trap_profile_test branch from 5db58aa to e1fd4a7 Compare May 24, 2025 13:14
@LucienMorey LucienMorey requested a review from auscompgeek May 26, 2025 22:26
@virtuald
Copy link
Member

virtuald commented Jun 2, 2025

My guess is that something is using an uninitialized variable somewhere? But, there's nothing obvious that sticks out. You might try printing out the repr() for the constraints and profiles before/after the calls that you do to see if there's anything weird that stands out.

I don't have a windows development environment so it's hard for me to diagnose this.

@virtuald virtuald force-pushed the add_trap_profile_test branch from 96452fe to 651163a Compare June 2, 2025 14:58
@virtuald
Copy link
Member

virtuald commented Jun 3, 2025

The test failures look to be bugs in semiwrap.

@virtuald
Copy link
Member

virtuald commented Jun 3, 2025

Might still be bugs in semiwrap, but it wasn't what I thought it was.

@LucienMorey
Copy link
Contributor Author

I have been meaning to keep playing with this, but just haven't had a moment over the last week. I don't immediately know how to chase bugs in semiwrap. Is it helpful for me to start porting more of the wpilib tests to python to increase the surface area of weirdness in case other failures make the cause of this more obvious?

I don't have a Windows environment either, but some of the students on my team do. Maybe I can bring them along for the ride during testing when we have some downtime.

@virtuald
Copy link
Member

virtuald commented Jun 4, 2025

Porting wpilib tests to python definitely would be a generally useful thing. Probably don't put them all into a giant PR though.

@virtuald
Copy link
Member

virtuald commented Jun 4, 2025

I'm an idiot and need more sleep. This was fixed in wpilibsuite/allwpilib#7894

The problem is that TotalTime in C++ returns m_endDecel and it is not initialized by default, so it can technically return whatever the compiler wants until calculate is called. It's weird that it only fails on Windows, but I guess that's just what undefined behavior means in this case. :)

virtuald added 2 commits June 4, 2025 01:00
- build-system.requires: pyntcore==2025.3.2.4
- build-system.requires: robotpy-hal==2025.3.2.4
- build-system.requires: robotpy-native-apriltag==2025.3.2.1
- build-system.requires: robotpy-native-ntcore==2025.3.2.1
- build-system.requires: robotpy-native-romi==2025.3.2.1
- build-system.requires: robotpy-native-wpihal==2025.3.2.1
- build-system.requires: robotpy-native-wpilib==2025.3.2.1
- build-system.requires: robotpy-native-wpimath==2025.3.2.1
- build-system.requires: robotpy-native-wpinet==2025.3.2.1
- build-system.requires: robotpy-native-wpiutil==2025.3.2.1
- build-system.requires: robotpy-native-xrp==2025.3.2.1
- build-system.requires: robotpy-wpimath==2025.3.2.4
- build-system.requires: robotpy-wpinet==2025.3.2.4
- build-system.requires: robotpy-wpiutil==2025.3.2.4
- build-system.requires: wpilib==2025.3.2.4
- lib updated to 2025.3.2-54-g7a3df61
- project.dependencies: pyntcore==2025.3.2.4
- project.dependencies: robotpy-hal==2025.3.2.4
- project.dependencies: robotpy-native-apriltag==2025.3.2.1
- project.dependencies: robotpy-native-ntcore==2025.3.2.1
- project.dependencies: robotpy-native-romi==2025.3.2.1
- project.dependencies: robotpy-native-wpihal==2025.3.2.1
- project.dependencies: robotpy-native-wpilib==2025.3.2.1
- project.dependencies: robotpy-native-wpimath==2025.3.2.1
- project.dependencies: robotpy-native-wpinet==2025.3.2.1
- project.dependencies: robotpy-native-wpiutil==2025.3.2.1
- project.dependencies: robotpy-native-xrp==2025.3.2.1
- project.dependencies: robotpy-wpimath==2025.3.2.4
- project.dependencies: robotpy-wpinet==2025.3.2.4
- project.dependencies: robotpy-wpiutil==2025.3.2.4
- project.dependencies: wpilib==2025.3.2.4
- pyntcore updated to 2025.3.2.4
- repo updated to https://frcmaven.wpi.edu/artifactory/development
- robotpy-apriltag updated to 2025.3.2.4
- robotpy-cscore updated to 2025.3.2.4
- robotpy-hal updated to 2025.3.2.4
- robotpy-halsim-gui updated to 2025.3.2.4
- robotpy-native-apriltag updated to 2025.3.2.1
- robotpy-native-ntcore updated to 2025.3.2.1
- robotpy-native-romi updated to 2025.3.2.1
- robotpy-native-wpihal updated to 2025.3.2.1
- robotpy-native-wpilib updated to 2025.3.2.1
- robotpy-native-wpimath updated to 2025.3.2.1
- robotpy-native-wpinet updated to 2025.3.2.1
- robotpy-native-wpiutil updated to 2025.3.2.1
- robotpy-native-xrp updated to 2025.3.2.1
- robotpy-romi updated to 2025.3.2.4
- robotpy-wpimath updated to 2025.3.2.4
- robotpy-wpinet updated to 2025.3.2.4
- robotpy-wpiutil updated to 2025.3.2.4
- robotpy-xrp updated to 2025.3.2.4
- wpilib updated to 2025.3.2.4
@virtuald virtuald force-pushed the add_trap_profile_test branch from 8fadde7 to a21af24 Compare June 4, 2025 05:05
@auscompgeek
Copy link
Member

Ah of course, that last test was added in that PR. Ha.

Co-authored-by: David Vo <auscompgeek@users.noreply.github.com>
@LucienMorey LucienMorey force-pushed the add_trap_profile_test branch from 4469cfd to d22b077 Compare June 5, 2025 04:54
Copy link
Member

@auscompgeek auscompgeek left a comment

Choose a reason for hiding this comment

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

Looks reasonable, including the dev build version bump at a glance.

@virtuald
Copy link
Member

I believe there is an updated 2025 release on the way. Unless there are objections, I think this PR should wait for that release and then be merged after it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants