Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add getLatestMessageOfEveryReceivedArbId() function #37

Merged
merged 3 commits into from
Nov 11, 2024

Conversation

NoahAndrews
Copy link
Member

@NoahAndrews NoahAndrews commented Nov 7, 2024

@doleksy, the Napi types come from this library, used to write native node.js addons in C++: https://github.com/nodejs/node-addon-api/

@LandryNorris This uses REVrobotics/CANBridge#41, which is small enough that I was comfortable merging with only Dave's approval, but you should at least look at what it does.

The goal here is to help me reliably determine what devices are on the bus in a manner that is both bulletproof and performant.

@NoahAndrews
Copy link
Member Author

NoahAndrews commented Nov 7, 2024

Benchmarking results

Hardware

My pretty-fast laptop connected to 20 SPARK Flexes running alpha firmware

Procedure

Measured how long getTimestampsForAllReceivedMessages() takes to run in node.js/electron after the REV Hardware Client (RHC) finishes querying the parameters from all 20 Flexes. Querying all parameters from 20 Flexes results in a lot of arbitration IDs getting sent on the bus (the total number of arbitration IDs in the returned map ended up reaching 2449 in my testing).

Result

Under these conditions, the function took 1-2ms to run.

Analysis

I think this was a reasonably tough stress test. The heartbeat only gets sent every 20ms, so 1-2ms is definitely a safe amount of time to block other CAN operations from happening for the relatively infrequent context in which I need to use this method, and it leaves plenty of headroom to account for slower computers and/or even more IDs in the map.

Leaving this as a draft until I've tested this with code that actually makes use of the result, and benchmarked that. I'll also make a note to test this on our low-performance testing laptop.

@NoahAndrews
Copy link
Member Author

Just realized that if we also include the actual data, this would become a bulletproof way to display the diagnostic CAN traffic, and we could fully kill the firehose.

@NoahAndrews
Copy link
Member Author

For now, I've just copied the code that assembles a CanMessage from receiveMessage() so I don't have to think about memory management, but there's a decent amount of code in this library that should be de-duplicated at some point (it's been worked on by a total of three people, none of whom are C++ devs).

I have not done any performance testing on the new function yet.

@NoahAndrews NoahAndrews force-pushed the add-getTimestampsForAllReceivedMessages-function branch from 8133f83 to f98f277 Compare November 10, 2024 04:08
@NoahAndrews NoahAndrews changed the title Add getTimestampsForAllReceivedMessages() function Add getLatestMessageOfEveryReceivedArbId() function Nov 11, 2024
@NoahAndrews
Copy link
Member Author

NoahAndrews commented Nov 11, 2024

getLatestMessageOfEveryReceivedArbId() was originally about an order of magnitude slower than getTimestampsForAllReceivedMessages() was, but adding a maxAgeMs parameter and giving it a value of 10 seconds allowed me to get back to 1-2ms levels of performance under the stress test, returning 105 entries. I did get a weird outlier where it took 9.4ms, but that isn't a big enough spike or common enough for me to want to spend more time on it. My guess is that something else on the slower side was holding the lock.

This was necessary to achieve acceptable performance
@NoahAndrews NoahAndrews marked this pull request as ready for review November 11, 2024 20:11
@NoahAndrews
Copy link
Member Author

NoahAndrews commented Nov 11, 2024

Going to merge this after approval of the latest changes from Landry, since Dave is away. The functionality works well, with very decent performance (though REVrobotics/CANBridge#42 would make it better).

@NoahAndrews NoahAndrews merged commit a20aa53 into main Nov 11, 2024
@NoahAndrews NoahAndrews deleted the add-getTimestampsForAllReceivedMessages-function branch November 11, 2024 22:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants