Open scarmain opened 8 months ago
I don't think that expected behavior is quite what we want. At the very least, it needs to be clear what the parent command is.
The core issue here is that commands enclose their own state, and the scheduler shouldn't know or care about what the running commands are doing internally. #5250 suggests adding this behavior to the name of the command, but the name shouldn't be dynamic (and thus cannot capture current state)
Seems to me that there are two distinct needs here. One is that we should communicate what commands are running in a scheduler so users can see their triggers and default commands working, and the other is to show what low-level commands are actually executing on each subsystem so users know exactly what's happening (like seeing which entry in a sequential group or which branch of a ConditionalCommand
is running).
We already implement the first one. A command scheduler will send the names of all the scheduled commands, but none of their internals. A subsystem does something similar, sending the name of the currently scheduled command that requires it (or "none" if there isn't one). I think this is useful behavior to keep; rather than replacing it, I'd like to see us add details on subsystem-level commands to complement what's already being conveyed at the scheduler level.
4735 doesn't really help towards the second goal; it only looks at the top level and can't help us when there are nested command groups, conditional, deferred, repeat, or select commands.
Screenshot from robotbuilder2 for reference - I've already put a lot of thought into how to display command groups and convey what the subsystems are actually doing, rather than what the scheduler sees.
We can include all member names in the composition name, though it will cause extremely long names that might be trouble for network and readability - albeit withName
ing commands could massively improve that.
4735 doesn't really help towards the second goal; it only looks at the top level and can't help us when there are nested command groups, conditional, deferred, repeat, or select commands.
It's a level deeper than we currently have. We can recursively nest all commands, but that is a lot of network traffic and has endless edge cases (command names that collide with each other or with other Sendable entries). Not that nesting Sendables is fully implemented atm anyway.
We can recursively nest all commands, but that is a lot of network traffic and has endless edge cases (command names that collide with each other or with other Sendable entries). Not that nesting Sendables is fully implemented atm anyway.
Yes, that's certainly overkill. If a user wants to inspect a particular command, then that command should be sent to the dashboard. It shouldn't be the responsibility of the scheduler, that'll be a lot of network traffic that will probably never be seen. (If shuffleboard was still the only source of data recording, I'd push back on this - but it's better to log the running commands robot-side now)
I like the idea of recursively including member names in the names of compositions, but I agree that it can get unwieldy fast. I don't think it's practical, though, for a few reasons. It can easily result in very long strings that are hard to display and parse, and might cause weird behavior if the commands in the composition change after being sent to a dashboard (calling SmartDashboard.putData(Command)
will use the current name of the command for the networktable key). Something like a JSON blob would be better to show that kind of structured data, but would likely need caching and subtree invalidation for performance so we don't spend millis on generating giant JSON blobs every loop. More engineering work in that route, but probably worth it to communicate full command graphs over the wire.
might cause weird behavior if the commands in the composition change after being sent to a dashboard
I think this type of use should be discouraged anyway. And getName
shouldn't be mutable regardless.
Describe the bug With autonomous commands, most teams make sequences that have the commands go in order. But, this becomes hard to debug as the Schedule object on NetworkTables only shows a name like SequentialCommandGroup, not the active commands under it.
To Reproduce Sample code (must be enabled to work):
result = {"ParallelCommandGroup"}
Expected behavior result = {InstantCommand, NotifierCommand, RepeatCommand, WaitCommand}
Desktop (please complete the following information):
Additional context This issue has been there for years, just thought I should fix it with all the new auto libraries out there now (like PathPlanner, Choreo, etc) to help diagnose issues.
I'm willing to work on this now, just had a question on implementation; Should the base command (say ParallelCommandGroup) still be in the list, or just all the sub components?
I think the way you would do this is use the SetUpdateTable() like ADXL345_I2C instead of the individual builders.