wpilibsuite / allwpilib

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

Add array and/or string support to `SimDevice`/`SimDeviceSim`. #6677

Closed brettle closed 2 weeks ago

brettle commented 1 month ago

Is your feature request related to a problem? Please describe.

For our specific use case, we'd like to create a sim that can effectively and efficiently represent an object's 3D position, orientation, velocity, and angular velocity. That requires at least 12 numbers. We'd like to store the relevant data in a single sim value to ensure it is sent atomically and relatively efficiently.

Describe the solution you'd like

On the robot side:

SimDoubleArray simArr = simDevice.createDoubleArray("kinematicData", kBidirectional, null);

double kinematicData = simArr.get();
// and/or
simArr.set(kinematicData);

On the sim side:

SimDoubleArray simArr = simDeviceSim.getDoubleArray("kinematicData");

double [] kinematicData = simArr.get();
// and/or
simArr.set(kinematicData);

Describe alternatives you've considered

At the moment we will need to create and handle a separately named sim value for each of the (at least) 12 numbers. That will be both cumbersome from a coding perspective and inefficient. We will also need to jump through some hoops to ensure that the numbers used on the receiving end are all from same update. We are particularly concerned about the possible inefficiencies of such a solution because the data will be going over websockets which means each property will need to be created/parsed, including converting each number to and from a string.

If efficiency is an issue, we'll likely resort to sending the data over a separate network connection.

Additional context

If support for arrays of doubles is too much work or would be sent over websockets as a json array, we would probably prefer support for either strings (or byte arrays that will be converted to base64-encoded strings within the json). We could take responsibility for doing the needed conversions.

Fwiw, there appears to already be some low level sim support for arrays which is being used by AddressableLED.

Would a PR that adds support for strings, arrays of bytes, or arrays of doubles be welcome or is that support not present by design?

PeterJohnson commented 1 month ago

The idea of SimDevice, per the name, was to provide some level of flexibility to emulate physical hardware devices very efficiently with very simple APIs. Very efficiently in this context meant atomic accesses, rather than mutex-locked, thus the limitation of word-sized data types. In addition, such data types have minimal JNI overhead and are callback friendly, as well as easily visualized in a GUI. AddressableLED isn’t implemented via SimDevice but rather is a custom implementation.

Notably, in addition to SimDevice, we have NetworkTables, which is a pub/sub framework that supports far more complex data values including arrays, strings, and raw and structured/serialized data such as protobuf.

What I don’t want to do is turn SimDevice into NetworkTables. If the ask is to just provide a mutex-locked (rather than atomic) byte array or string value support, it’s something we can look into, but there may be significant performance downsides, so it should be used sparingly.

It’s somewhat unclear to me what hardware you are simulating that would need this complexity of 3D pose information. Normally low level hardware would not provide something quite like this.

brettle commented 1 month ago

In our particular case, we aren't simulating any real hardware device. Instead we were looking for an easy way to get simulation results (e.g. positions, orientations, velocities, of the robot or particular parts of it) from an external simulator back to a test framework which runs the actual robot code. Since we already have a connection between the test/robot code and the simulator via halsim_ws, we were hoping to piggy-back off of that instead of adding a new communication channel such as NetworkTables.

While I admit that the above use is not what was intended, we also would like to simulate the navX-mxp AHRS device and it does in fact provide complete 3D pose, velocity, and acceleration info (among other data). Support for mutex-locked byte array or string values would allow that data to be transferred efficiently while ensuring that it remains self-consistent.

Based on your response, we're now using NetworkTables for the first case. Fwiw, doing so increased the size of the jar used by the external simulator from 1.6MB to 9.9MB. The absolute size is not a big deal in the larger scheme of things, but the large relative increase is at least worth noting.

I'll let you decide based on the above info whether you want to close this as wont-fix, or keep this open as a possible future feature. Regardless, thanks for the quick response!