search

wpilibsuite / allwpilib

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

[wpilib] Make HID classes implement Sendable #6782

Open spacey-sooty opened 4 days ago

spacey-sooty commented 4 days ago

Resolves https://github.com/wpilibsuite/allwpilib/issues/5191

Alextopher commented 3 days ago

I wonder if the smart dashboard type should be unique for each controller - so a dashboard would be able to easily disambiguate which kind of controller it is. Or it could stay as a "HID" with a string field listing which class it is.

spacey-sooty commented 3 days ago

I wonder if the smart dashboard type should be unique for each controller - so a dashboard would be able to easily disambiguate which kind of controller it is. Or it could stay as a "HID" with a string field listing which class it is.

It doesn't make it easier to distinguish even if they are different types, as people often have multiple of one type of controller plugged in

Alextopher commented 3 days ago

"Disambiguate" as in if I drag a HID object onto the viewing window it could automatically render a PS5 or XBOX controller depending on which is being sent.

spacey-sooty commented 3 days ago

"Disambiguate" as in if I drag a HID object onto the viewing window it could automatically render a PS5 or XBOX controller depending on which is being sent.

This feels like it would take smart dashboard changes as well, if someone else is willing to take up the work there I'm happy to make the change here but unless I see that Ill leave this as is.

Alextopher commented 3 days ago

This feels like it would take smart dashboard changes as well

How so? The '"HID"' SmartDashboard type doesn't exist anywhere yet. We're defining the API in this PR.

My understanding of the .type field in network tables is that dashboard designers can expect to switch on that value and it communicates what fields (are expected to) exist in a table. If each controller is of "HID" type but they all have different fields (because the buttons have different names) then I think that breaks the model.

Starlight220 commented 2 days ago

the smart dashboard type should be unique for each controller

I'm not a fan of this; it adds a ton of new types that dashboards need to implement specially -- and given how easy it is to add controller types to WPILib I'm not sure it's maintainable for dashboards.

it could stay as a "HID" with a string field listing which class it is

I like this option much more. It allows dashboards a simpler implementation of falling back to a default controller if the one used isn't defined. Publishing buttons and axes etc as arrays (similar to how they're pushed to DataLog) with a const array of names would also be a good step; it'd allow to deduplicate a lot of code on both the sendable and the dashboard side.