wpilibsuite / allwpilib

Official Repository of WPILibJ and WPILibC
https://wpilib.org/
Other
1.08k stars 611 forks source link

Add an `allowedSharedRequirement` parameter for parallel command compositions #5144

Open Starlight220 opened 1 year ago

Starlight220 commented 1 year ago

Currently, parallel command compositions ensure no shared requirements internally. This can be a problem for in-subsystem command factories. We should have an optional arg to relax this restriction.

Open questions: should it be a single boolean parameter to completely remove the check, or accept the exact subsystem(s) that may be shared? If the latter, a single parameter or vararg? Do we require a subsystem that's passed as allowed for sharing but no composition member requires it?

prateekma commented 1 year ago

Do you have an example where this is a problem currently?

Starlight220 commented 1 year ago

The shooter commands in the RapidReactCommandBot example involves two commands that require this subsystem composed in parallel. Situations like this are common. The two solutions are splitting the subsystem to two individually-requirable elements, or allowing shared use of a requirement in a parallel composition.

TheTripleV commented 1 year ago

I think adding support for nested subsystems would be better than an escape hatch from requirements checking.

Nested subsystems: A subsystem can register subystems within it. In a command, requiring a subsystem requires the specified subsystem and all of it's children.

Ex: A telescoping arm could be structured in code as an arm subsystem containing a pivot subsystem and a telescope subsystem.

Oblarg commented 1 year ago

Nested subsystem support doesn't actually fix this problem - it doesn't add anything you can't already do by dividing to child subsystems. The parent subsystem isn't adding any functionality.

The escape hatch from within subsystem scope makes sense; if we're composing commands from inside of a subsystem it doesn't make sense to enforce access guards on the subsystem hardware - we can multi-allocate it in any number of ways from that scope, anyway, so the guard isn't really protecting anything there.