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

[cmd] Revert "Call wrapped command initSendable (#6471)" #7353

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

rzblue
Copy link
Member

@rzblue rzblue commented Nov 6, 2024

Closes #7352
This reverts commit 7bc0380.

Calling initSendable on the wrapped command is fundamentally flawed as the wrapper is the command being sent.

…pilibsuite#6471)"

This reverts commit 7bc0380.

Calling initSendable on the wrapped command is fundamentally flawed as the wrapper is the command being sent.
@rzblue rzblue requested a review from a team as a code owner November 6, 2024 02:00
Copy link
Contributor

github-actions bot commented Nov 6, 2024

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

@KangarooKoala
Copy link
Contributor

This PR modifies commands. Please open a corresponding PR in Python Commands and include a link to this PR.

Looks like the original PR wasn't done in Python, since I don't see anything sendable-related in wrappercommand.py.

@Starlight220
Copy link
Member

I mean, all this started from #6292. The wrapped command's sendable representation should be sent - the problem is the scheduling lambdas (which need to point to the wrapper command instead)

@rzblue
Copy link
Member Author

rzblue commented Nov 6, 2024

There's not a good way of doing that currently.
One thing we could do instead is remove WrapperCommand entirely and make its usages mutate the command instead of wrapping- this should be fine to do because you wouldn't be able to use the inner command anyway, and the wrapping behavior largely comes from the limitations present when Command was an interface.

This doesn't work (easily) for finallyDo/handleInterrupt, unfortunately.

This ultimately comes back to the whole problem of internal visibility of compositions- currently, by design, everything is opaque, which makes it near impossible to get that visibility back when you want it (e.g. for telemetry)

@Starlight220
Copy link
Member

Why would you not be able to use the original command in case of mutation? It would be a silent state change rather than a loud error.

@KangarooKoala
Copy link
Contributor

Perhaps I'm misinterpreting, but I think what @rzblue meant is that because you already can't use the original command, it's fine for us to mutate it since no previous behavior could have relied on having the original still around.

I'm still a bit iffy about mutating the original command, though, since it could cause confusion if people are reusing commands that then get decorated, and I also don't see how we would be able to do it because of the need to override the methods. (In ignoringDisable(), withInterruptBehavior(), finallyDo(), and handleInterrupt()- withName() is fine)

@Starlight220
Copy link
Member

Starlight220 commented Nov 6, 2024

Would implementing withName as overriding getName rather than calling setName change something?

@KangarooKoala
Copy link
Contributor

It would make the implementation more similar to the others, but it would prevent a regular setName() from working and I'm not sure if the other implementations are necessarily how we want them.

@rzblue
Copy link
Member Author

rzblue commented Nov 6, 2024

Would implementing withName as overriding getName rather than calling setName change something?

No, and it still wouldn't fix the core issue here- WrapperCommand is a composition, and augments the behavior of the wrapped command. WrapperCommand cannot safely "unwrap" itself without violating it's own invariants.

I also don't see how we would be able to do it because of the need to override the methods. (In ignoringDisable(), withInterruptBehavior(), finallyDo(), and handleInterrupt()- withName() is fine)

finallyDo and handleInterrupt are hard to solve with mutation because they can be called multiple times and each subsequent call needs to add behavior, not replace the behavior.

ignoringDisable and withInterruptBehavior can be implemented by storing a value in Command and mutating that. runsWhenDisabled and getInterruptionBehavior would then return that value.

Peter brought up that we could use function pointers stored in the base class and mutate those- this is doable but a bit tricky to reason about and a large departure from the previous implementation.
Godbolt toy implementation of what we'd need for finallyDo, for fun: https://godbolt.org/z/rb3dn16o3

@PeterJohnson PeterJohnson merged commit af65281 into wpilibsuite:main Nov 6, 2024
42 checks passed
@rzblue rzblue deleted the revert-7bc0380 branch December 20, 2024 22:27
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.

command.withName crashes when run from dashboard
4 participants