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.

Sign up for GitHub

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

Jump to bottom

[cmd] Send composition member names #4735

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

[cmd] Send composition member names #4735

wants to merge 2 commits into from
+141 −21

Conversation

Starlight220
Copy link
Member

No description provided.

@Starlight220 Starlight220 requested a review from a team as a code owner November 29, 2022 13:23
@Starlight220 Starlight220 linked an issue Nov 30, 2022 that may be closed by this pull request
Enhance Command Sendable implementation #4734
Closed
@PeterJohnson
Copy link
Member

Please provide a brief summary of what this change does.

@sciencewhiz
Copy link
Contributor

Is this intended to address wpilibsuite/betatest#136? It looks like dashboard updates would be required to take advantage of most of this.

@Starlight220
Copy link
Member Author

This would address that issue, making WrapperCommand copy the name of the wrapped command.
This PR improves the Sendable representation of compositions, adding their members as a Sendable property.

I'm not sure why dashboard updates would be needed.

PeterJohnson
PeterJohnson requested changes Dec 14, 2022
View reviewed changes
Copy link
Member

@PeterJohnson PeterJohnson left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I don't like how much work this is doing. All of the getter code will be called every periodic loop. For all of these cases, is there a reason the array can't be generated in initSendable (so once) and just captured and returned by the getter? Ideally we'd have a way to even avoid that, e.g. by passing a non-callable, to just set once, but that functionality isn't present, and adding it may break vendors, so I'm going to punt that to 2024.

@Starlight220 Starlight220 mentioned this pull request Dec 15, 2022
Merged
@Starlight220 Starlight220 marked this pull request as draft December 15, 2022 17:51
@Starlight220 Starlight220 added the state: blocked Something is blocking action. label Dec 18, 2022
@Starlight220
Copy link
Member Author

Blocked until there's an efficient way to not recompute array properties.

@Starlight220 Starlight220 changed the title enhance Command Sendable impls [cmd] Send composition members Dec 18, 2022
@Starlight220 Starlight220 mentioned this pull request Mar 2, 2023
Merged
@Starlight220 Starlight220 linked an issue Sep 17, 2023 that may be closed by this pull request
Include more detail on group contents in command group getName() #5250
Open
@Starlight220 Starlight220 removed the state: blocked Something is blocking action. label Oct 2, 2023
@Starlight220 Starlight220 marked this pull request as ready for review October 2, 2023 16:03
@Starlight220
Copy link
Member Author

Unblocked by #5158.

@Starlight220 Starlight220 requested a review from rzblue November 5, 2023 05:21
@Starlight220

This comment has been minimized.

Sign in to view
@Starlight220 Starlight220 requested review from a team as code owners December 1, 2023 07:25
@Starlight220 Starlight220 force-pushed the sendcommand branch 2 times, most recently from a408761 to c6347ea Compare December 1, 2023 07:28
@calcmogul calcmogul added the component: command-based WPILib Command Based Library label Dec 2, 2023
@rzblue rzblue mentioned this pull request Dec 10, 2023
Open
@Starlight220 Starlight220 mentioned this pull request Dec 11, 2023
Closed
@Starlight220 Starlight220 mentioned this pull request Mar 27, 2024
Open
@Starlight220 Starlight220 changed the title [cmd] Send composition members [cmd] Send composition member names Apr 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@calcmogul calcmogul Awaiting requested review from calcmogul

@PeterJohnson PeterJohnson Awaiting requested review from PeterJohnson

@rzblue rzblue Awaiting requested review from rzblue

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
component: command-based WPILib Command Based Library
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Include more detail on group contents in command group getName() Enhance Command Sendable implementation
5 participants
@Starlight220 @PeterJohnson @sciencewhiz @calcmogul @rzblue