Conversation
Yep.
That's my guess. Recommend ignoring for now. |
|
Should we investigate the windows failure before merging? This time, it was the system test rather than asyncapi. It appeared to go away after I bumped WPIWebSockets, but idk if that's because of the library changes or just a flaky test. To be safe, I restarted the jobs on the previous commit, so we can see what happens. Edit: The tests passed after a rerun, so it appears to be a flaky test :/ |
Let's hold off on merging for now. I can reproduce the failure here depending on when I start the test vs Webots. I think the simulator is actually outrunning the robot code in this case. I'm working on a fix and will push it to this branch for you to review when it's ready. |
This works but still needs be cleaned up.
Define robot time to start when the first periodic method is called and keep that time in sync with the sim time.
|
@CoolSpy3, please review. It seems to be working as of my latest push. It's passed CI twice in a row on all three platforms. The third try is running as I write this. |
|
Alright, I've started taking a look at it, but we're wrapping up finals week, so I have a couple of other things I have to finish first. I might get to take a look at it tomorrow. If not, it'll probably have to wait until Sunday. |
|
Focus on finals. No rush. |
CoolSpy3
left a comment
There was a problem hiding this comment.
In general, I think these changes are good, however, I think that it adds a bit too much required complexity for an extension which is supposed to interface well with the WPILib HALSim.
There are two main features that I think should be added:
- A flag that can be set in the robot code to disable the synchronization behavior.
- I think that the restriction that the robot code and simulator stay exactly in sync is useful for a CI environment, but not for an irl use case environment where controllers may be hot-swapped/reloaded during testing. I think the code should be updated to synchronize on deltas rather than absolute timestamps (so that the exact robot/sim time when the controller/robot code starts is irrelevant). [This is a more complex feature, and the current behavior is functional, so I'm fine with leaving it to a future PR.]
- Both of these should be documented in the README with links to the appropriate synchronization code (in
exampleor the futurelib199implementation).
|
@CoolSpy3, let me know if I've addressed your comments. If I have, please merge. Thanks! |
This is a bunch of miscellaneous bug fixes for things I found when writing simulation docs. This is a continuation of my changes from the
swervesimbranch, so (similar to the PR in WPIWebSockets) the changes are not that concise. There's also been a Webots bug fixed since then, so some of the changes are for a workaround which is later backed out. Overall, these changes should be more straightforward, so I don't think it's as necessary to view the commit diffs individually. You still can though if it's helpful.I'm going to keep this as a draft until I go through the other libraries in case I think of anything else.
Atm, this is also untestedI got the system test working (locally, see note about CI), but a lot of these changes are fairly straight forward, so... its probably fine :D I can maybe run some tests once the other libraries are brought up-to-date. It would also be nice to test with any new Rev/Phoenix sim stuff, so keep an eye on DeepBlueRobotics/lib199#44 and DeepBlueRobotics/lib199#62.