Data pipeline#17
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a data pipeline for CAN bus data, including support for parsing DBC frames. While the new functionality is significant, I've identified several critical issues related to thread safety, including race conditions that could lead to memory corruption or crashes. There are also performance concerns with busy-waiting loops that will consume unnecessary CPU cycles. Furthermore, the code has some design issues like code duplication and redundant data structures that will make it harder to maintain. I've provided specific comments and suggestions to address these critical and high-severity issues. Please address the thread-safety problems with high priority.
RomeroPablo
left a comment
RomeroPablo
left a comment
There was a problem hiding this comment.
None of the proposed changes are used in the actual project, nor are there any demonstrations or tests for the changes/additions. Please add appropriate UI logging that demonstrates that your proposal works for the intended work load. Additionally, there are a lot of instances of poor code smell, so please manually review and reason about code that is added to the project, especially if it is generated by any LLM. Please provide an end to end architecture and plan demonstrating your intentions with your proposed changes/additions. Make sure you mention how you plan to address threading, concurrency, synchronization, and interfaces for individuals wanting to interface with data provided over the network (slcan packets, h264, glb/gltf files, dbc files, etc.)
|
|
||
| SPSCQueue<uint8_t> spscQueue; | ||
| std::string IP ="3.141.38.115"; | ||
| std::string IP = "172.0.0.1"; //"3.141.38.115"; |
There was a problem hiding this comment.
Leave the IP as 3.141.38.115. With the proposed change, if someone builds and tries to run the program they fail to make a valid connection, and are unable to observe any proposed changes to the networking stack.
| @@ -1,4 +1,3 @@ | |||
| /*[ξ] the photon network interface*/ | |||
There was a problem hiding this comment.
There is no associated hpp (e.g. connector.hpp) for this file. There also appears to be duplicated code in this file and network.cpp/hpp. Either consolidate all code into connector.cpp (and a new connector.hpp), or remove the file.
|
|
||
| class DbcConnector { | ||
| public: | ||
| std::unordered_map<int, std::shared_ptr<DbcMessageInfo>> dbcMap; |
There was a problem hiding this comment.
do not use 'smart' pointers in this code base.
|
|
||
| /* end of network class */ | ||
| std::mutex dbcMapMutex; | ||
| std::unordered_map<uint32_t, std::unique_ptr<DbcMessage>> dbcMap; |
There was a problem hiding this comment.
do not use 'smart' pointers in this code base.
| unsigned PORT = 8187; | ||
|
|
||
|
|
||
| private: |
There was a problem hiding this comment.
consolidate all private functions and variables under the "private" declared earlier in the class
| } | ||
| }; | ||
|
|
||
| static DbcConnector dbcConnector; |
There was a problem hiding this comment.
Do not have a single static declaration of the class in the source file. This is really bad.
| } | ||
|
|
||
| void Network::parser(){ | ||
| void Network::parser() { |
There was a problem hiding this comment.
I think having our slcan parsing and DBC file parsing intermixed in a single functions is really bad, in terms of code smell and perf, wasting cycles on every attempted parse checking for a dbc file that may never be received by an instance does not make sense. An instance will very rarely every query for a DBC file, so there should be some short lived interface that independently queries and receives any desired DBC files. It also makes our parsing harder to reason about and debug.
| writeDBC(canID, name, dlc, sender); | ||
| currentCanId = canID; | ||
| logs("[parser] Registered BO_ " + name + " (CAN ID: " + std::to_string(canID) + ")"); | ||
| printDBCMap(); // ✅ print here after adding new BO_ |
|
|
||
| writeSignal(currentCanId, sig); | ||
| logs("[parser] Registered SG_ " + sig.name + " for CAN ID " + std::to_string(currentCanId)); | ||
| printDBCMap(); // ✅ print here after adding new SG_ |
3 participants
No description provided.