Add/Fix pdp.GetAllCurrents() #6025
Conversation
|
/format |
| @@ -87,6 +94,17 @@ double PowerDistribution::GetCurrent(int channel) const { | |||
| return current; | |||
| } | |||
|
|
|||
| std::vector<double> PowerDistribution::GetAllCurrents() const { | |||
There was a problem hiding this comment.
@PeterJohnson should we make this be a SmallVector too? This could be a pretty hot path.
There was a problem hiding this comment.
I choose std::vector as it was the return type of all the NetworkTables get__Array() like GetDoubleArray(). I'm neutral either way.
The API call here should be the vector/SmallVector though, as I don't see anywhere in the C++ APIs that we return arrays.
Honestly, if speed was a concern, I think in the JNI service, we could go back to my initial prototype of just a straight double[24] array. We don't need a vector there as we could just allocate to the biggest PDP size, and the HAL filling the array means it doesn't need to be initialized too. Since it doesn't leave the function, we don't need the dynamic sizing there. I just didn't see as much array support in C++ (and I'm not a native C++ developer).
There was a problem hiding this comment.
So typically how we do this is we have both a vector and a SmallVector overload of the same function. So you would keep the current function and add a SmallVector-taking one: std::span<double> GetAllCurrents(wpi::SmallVectorImpl<double>& buf).
|
Also, should I improve the LiveWindow to use this new function, saving 75% reads? I think the way you would do this is use the SetUpdateTable() like ADXL345_I2C instead of the individual builders. |
|
Re: Sendable, I think the right way to do this (although we would need to update dashboards to match) is to change it from individual topics to one DoubleArray topic. |
|
I made a new version, where the initSendable now uses the GetAllCurrents() function. Let me know and I can merge this in too. I think this is the best compromise without having to rewrite all the dashboards so close to kickoff. |
calcmogul
added
component: wpilibj
|
I don't like how it changes PowerDistribution to use NTSendable instead of Sendable. Logging Sendables with DataLog is something that would be desirable, and the more classes that use NTSendable, the harder it'll be to migrate. Although, work on that has been suspended until we think more about telemetry. Since there's another easy to implement, but breaking solution, I'm okay with taking on a little bit of tech debt here to avoid breaking stuff, as long as we pay it off by properly implementing it after this season. |
For logging purposes, logging all the PDP channels ends up with 16/24 CAN frame requests for all the currents. Using the functions provided by the low level drivers, we can get this down to 3/4 requests.
I implemented the JNI interface to support both PDPs now (it was capped at 16 channels), and added support for both C++ and Java. I also implemented GetNumChannels() in C++ to have more identical interfaces between C++/Java.
This change is in support of #5314, but not really part of it. I would love to move this to a DataLogBuffer like the driver station data, but the PDP doesn't have a periodic function to run the data logging. Also, it might be nice to change the initSendable functions to use these buffered reads instead of individual channel reads it does now (not exactly sure how to do this).