Replace protobuf with JSON #11
Conversation
WalkthroughThe changes primarily involve the removal of the Protobuf library and associated dependencies from the project, simplifying the build configuration. This includes updates to various CMake and source files, where references to Protobuf are eliminated. Additionally, the submodule declaration for Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range, codebase verification and nitpick comments (3)
CMakeLists.txt (1)
6-6: Document the version update.The project version has been updated to
3.2.0. Ensure this change is documented in the project's changelog or release notes.source/bringauto/communication/fleet_protocol/FleetProtocol.cpp (2)
136-142: Ensure comprehensive JSON error handling.The current JSON parsing implementation catches
json::parse_error, which is good. However, consider logging the exception message to provide more context on the failure.- } catch (json::parse_error &) { - logging::Logger::logWarning("Cannot parse car command!"); + } catch (json::parse_error &e) { + logging::Logger::logWarning("Cannot parse car command! Error: {}", e.what());
149-162: Improve JSON field access error handling.The current implementation catches
json::exceptionfor field access errors. Consider logging the specific exception message to aid debugging.- } catch (json::exception &) { - logging::Logger::logWarning("Cannot parse car command!"); + } catch (json::exception &e) { + logging::Logger::logWarning("Cannot parse car command! Error: {}", e.what());
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- .gitmodules (1 hunks)
- CMakeLists.txt (3 hunks)
- README.md (1 hunks)
- cmake/Dependencies.cmake (1 hunks)
- include/bringauto/communication/fleet_protocol/FleetProtocol.hpp (1 hunks)
- source/bringauto/common_utils/EnumUtils.cpp (1 hunks)
- source/bringauto/communication/fleet_protocol/FleetProtocol.cpp (3 hunks)
- source/bringauto/settings/SettingsParser.cpp (1 hunks)
- test/CMakeLists.txt (1 hunks)
Files skipped from review due to trivial changes (4)
- .gitmodules
- README.md
- include/bringauto/communication/fleet_protocol/FleetProtocol.hpp
- test/CMakeLists.txt
Additional comments not posted (5)
cmake/Dependencies.cmake (1)
Line range hint
1-24:
Verify removal of all protobuf dependencies.The
protobuflibrary has been removed from the dependencies. Ensure that all parts of the codebase that previously relied onprotobufare updated to use JSON or are otherwise handled.Verification successful
Protobuf dependencies appear to be removed. The search for
protobufreferences in the entire codebase did not return any results, indicating that all dependencies onprotobufhave likely been removed. If you have specific concerns or need further verification, consider manually checking or running additional checks.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no parts of the codebase still rely on `protobuf`. # Test: Search for any remaining references to `protobuf`. Expect: No occurrences. rg --type cpp 'protobuf'Length of output: 24
Script:
#!/bin/bash # Description: Comprehensive search for any remaining references to `protobuf` in the codebase. # Search for `protobuf` in all file types to ensure it's completely removed. rg 'protobuf'Length of output: 13
source/bringauto/common_utils/EnumUtils.cpp (1)
10-18: Verify usage of the new string-basedvalueToEnumfunction.Ensure that all calls to
EnumUtils::valueToEnumwith a string argument are updated and validated properly.CMakeLists.txt (1)
6-6: Verify the build process after protobuf removal.The
protobufrelated configurations have been removed. Ensure that the build process is unaffected and the project compiles successfully.source/bringauto/communication/fleet_protocol/FleetProtocol.cpp (1)
174-186: Confirm the completeness of JSON status logging.The function logs the generated JSON status string. Ensure that all relevant status fields are included in the JSON object to provide a complete status report.
Verification successful
Confirm the completeness of JSON status logging.
The
generateCarStatusStringfunction appears to include all relevant fields for the car's status in the JSON object. The fields logged are:
- Telemetry: altitude, latitude, longitude, speed, and fuel.
- Next Stop: name, altitude, latitude, and longitude.
- State.
These fields seem comprehensive for generating a car status report. Ensure that these fields align with the expected data structure and requirements.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all relevant status fields are included in the JSON object. # Test: Search for the `generateCarStatusString` function and ensure all fields are logged. ast-grep --lang cpp --pattern $'std::string FleetProtocol::generateCarStatusString(const Status &status) { $$$ statusJson[$_] = $_; $$$ }'Length of output: 145
Script:
#!/bin/bash # Description: Extract the JSON field assignments within the `generateCarStatusString` function to verify completeness. # Use `rg` to search for the function definition and capture the relevant lines. rg -A 20 'std::string FleetProtocol::generateCarStatusString' --glob '*.cpp'Length of output: 2378
source/bringauto/settings/SettingsParser.cpp (1)
91-97: Update command-line options for clarity.The fleet provider option has been updated to "INTERNAL-PROTOCOL." Ensure that this reflects the intended protocol and that users are informed of the change through documentation or release notes.
Verification successful
Documentation reflects updated fleet provider option.
The
README.mdfile includes information about thefleet-provideroption and mentionsinternal-protocol, indicating that the documentation has been updated to reflect the changes in the codebase. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the documentation reflects the updated fleet provider option. # Test: Search for occurrences of "fleet-provider" in documentation files. rg --type markdown --type rst --type txt 'fleet-provider'Length of output: 454
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
source/bringauto/settings/StateSmurfDefinition.cpp (1)
Implications of Removing
changeToStateFunctionThe
changeToStatefunction is actively used in the codebase, specifically inSimVehicle.cpp. Its removal will affect the application's ability to manage state transitions, as it is a critical part of the logic for transitioning between autonomy states.
- File and Location of Usage:
source/bringauto/virtual_vehicle/vehicle_provider/SimVehicle.cppPlease ensure that any changes to this function are carefully considered to maintain the application's state management capabilities.
Analysis chain
Line range hint
28-48: Consider the implications of removingchangeToState.The removal of the
changeToStatefunction indicates a change in how state transitions are managed. Ensure that this does not affect the application's ability to transition between states based on autonomy actions.Run the following script to verify the usage of the
changeToStatefunction in the codebase:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `changeToState` function. # Test: Search for the function usage. Expect: No occurrences. rg --type cpp --type h $'changeToState'Length of output: 562
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (3)
test/smurf/duplicitStopScenario/etalons/fewNodes.logis excluded by!**/*.logtest/smurf/etnaDefaultScenario/etalons/default.logis excluded by!**/*.logtest/smurf/routeChangeScenario/etalons/delayedChange.logis excluded by!**/*.log
Files selected for processing (10)
- README.md (1 hunks)
- include/bringauto/settings/StateSmurfDefinition.hpp (1 hunks)
- source/bringauto/communication/fleet_protocol/FleetProtocol.cpp (4 hunks)
- source/bringauto/communication/terminal/TerminalOutput.cpp (1 hunks)
- source/bringauto/settings/SettingsParser.cpp (2 hunks)
- source/bringauto/settings/StateSmurfDefinition.cpp (2 hunks)
- source/bringauto/virtual_vehicle/vehicle_provider/SimVehicle.cpp (2 hunks)
- test/smurf/duplicitStopScenario/scenarios.json (1 hunks)
- test/smurf/etnaDefaultScenario/scenarios.json (1 hunks)
- test/smurf/routeChangeScenario/scenarios.json (1 hunks)
Files skipped from review due to trivial changes (2)
- README.md
- source/bringauto/communication/terminal/TerminalOutput.cpp
Additional comments not posted (16)
test/smurf/etnaDefaultScenario/scenarios.json (2)
9-9: Clarify the unit intimeout_s.Renaming
timeouttotimeout_sis a good practice as it clarifies the unit of measurement (seconds). Ensure that this change is consistently applied throughout the codebase.
10-15: Review theargumentsarray format.The change from an object to an array format for
argumentsis beneficial for standardizing command-line argument parsing. Ensure that any code parsing these arguments is updated to handle the new format.test/smurf/routeChangeScenario/scenarios.json (2)
9-9: Clarify the unit intimeout_s.Renaming
timeouttotimeout_sensures clarity regarding the unit of measurement. Verify consistent application across the codebase.
10-15: Review theargumentsarray format.Switching
argumentsto an array format aligns with best practices for command-line argument parsing. Ensure that all related parsing logic is updated accordingly.test/smurf/duplicitStopScenario/scenarios.json (2)
9-9: Clarify the unit intimeout_s.The change from
timeouttotimeout_sis consistent with the goal of clear unit representation. Ensure uniformity across the project.
10-16: Review theargumentsarray format.The conversion of
argumentsto an array format is a positive change for command-line argument handling. Ensure that all relevant parsing logic is updated to accommodate this format.include/bringauto/settings/StateSmurfDefinition.hpp (1)
Line range hint
9-13: Verify the impact of removing state constants.The removal of state constants such as
statusSentState,commandReceivedState,startReceivedState,stopReceivedState, andnoActionReceivedStatesuggests a shift in state management logic. Ensure that these changes do not break existing functionality or state transitions elsewhere in the codebase.Run the following script to verify the usage of these removed state constants in the codebase:
source/bringauto/settings/StateSmurfDefinition.cpp (1)
17-26: Review the new state transitions.The state transitions have been simplified. Ensure that the new transitions between
connected,drive,idle, andinStopstates meet the application's requirements and do not introduce any unintended behavior.Run the following script to verify the usage of the
createStateDiagramfunction in the codebase:Verification successful
State Transitions Verification Completed
The new state transitions in
StateSmurfDefinitionappear to be properly integrated into the application's context. ThecreateStateDiagramfunction is used within a conditional compilation block to set up state transitions for a vehicle system, and no unintended behavior is evident from the context provided. Ensure that these transitions align with the application's requirements and are tested under theSTATE_SMURFconfiguration.
- File:
main.cpp- Context: Conditional compilation with
STATE_SMURFfeature toggle.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `createStateDiagram` function. # Test: Search for the function usage. Expect: Occurrences that align with the new state transitions. rg --type cpp --type h $'createStateDiagram'Length of output: 387
Script:
#!/bin/bash # Extract context around the usage of createStateDiagram in main.cpp rg --type cpp -A 10 -B 10 'createStateDiagram' main.cppLength of output: 898
source/bringauto/communication/fleet_protocol/FleetProtocol.cpp (4)
130-136: Review JSON parsing error handling.The JSON parsing includes error handling with logging. Ensure that the logging provides sufficient information for debugging and that the application can recover gracefully from parsing errors.
168-180: Review JSON status generation.The generation of car status as a JSON object is a significant change. Ensure that the JSON structure accurately represents the car's status and that all necessary fields are included.
7-12: Ensure JSON library compatibility.The inclusion of the
nlohmann::jsonlibrary is a key change. Ensure that the library is compatible with the project's requirements and that all necessary dependencies are included in the build configuration.Run the following script to verify the inclusion of the JSON library in the build configuration:
Verification successful
JSON Library is Properly Included in Build Configuration
The
nlohmann_jsonlibrary is correctly included in the build configuration files. It is specified inCMakeLists.txtwith theFIND_PACKAGEcommand and linked appropriately in the project. No further action is needed regarding this concern.
CMakeLists.txt:FIND_PACKAGE(nlohmann_json 3.2.0 REQUIRED)CMakeLists.txt:nlohmann_json::nlohmann_jsontest/CMakeLists.txt:nlohmann_json::nlohmann_jsonScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the inclusion of the JSON library in the build configuration. # Test: Search for the JSON library in the build configuration files. Expect: Proper inclusion. rg --type cmake $'nlohmann::json'Length of output: 32
Script:
#!/bin/bash # Description: Search for alternative references to the JSON library in CMake files. # Look for common CMake commands that might include the JSON library. rg --type cmake 'nlohmann_json|nlohmann|find_package|target_link_libraries' -A 2Length of output: 2583
143-156: Verify mission data extraction from JSON.The extraction of mission data from JSON fields is a critical operation. Ensure that the JSON structure matches the expected format and that all necessary fields are present.
Run the following script to verify the usage of the
processBufferDatafunction in the codebase:Verification successful
Mission data extraction from JSON is consistent with the expected structure.
The code correctly accesses the JSON fields "stops", "name", "position", "latitude", and "longitude" as expected. No issues were found with the extraction logic.
- The JSON structure used in the code snippet aligns with the expected format.
- All necessary fields are present and accessed correctly.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `processBufferData` function. # Test: Search for the function usage. Expect: Occurrences that align with the new JSON structure. rg --type cpp --type h $'processBufferData'Length of output: 388
Script:
#!/bin/bash # Search for any JSON schema or documentation that defines the expected structure of the JSON object. rg --type cpp --type h --type md --type json 'stops|name|position|latitude|longitude'Length of output: 25001
source/bringauto/virtual_vehicle/vehicle_provider/SimVehicle.cpp (2)
Line range hint
114-197: Separation of concerns inevaluateCommand.The removal of
StateSmurfDefinition::changeToStateenhances the separation of concerns by focusing this method solely on command evaluation.
225-227: Conditional compilation for state transitions.The use of conditional compilation for
StateSmurfDefinition::changeToStatemaintains flexibility in state management.source/bringauto/settings/SettingsParser.cpp (2)
91-97: Update to fleet provider settings.The changes from "protobuf" to "INTERNAL-PROTOCOL" and renaming to "JSON fleet provider" align with the transition to JSON.
351-351: Typo correction ingetFormattedSettings.The correction from "fleet-providerr" to "fleet-provider" improves clarity and accuracy.
…874/skip_protobuf_option
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
main.cpp (1)
Line range hint
63-66: LGTM: Improved error handling during initializationThe addition of the try-catch block for initialization improves error handling and aligns with the enhanced error reporting mentioned in the summary. Using std::cerr for error output is a good practice.
Consider adding more context to the error message:
- std::cerr << "[ERROR] Error occurred during initialization: " << e.what() << std::endl; + std::cerr << "[ERROR] Error occurred during Virtual Vehicle initialization: " << e.what() << std::endl;This change provides more specific information about where the error occurred.
source/bringauto/settings/SettingsParser.cpp (1)
97-103: LGTM! Consider adding a comment for clarity.The changes from "protobuf" to "INTERNAL-PROTOCOL" and the recategorization of options under "JSON fleet provider" align well with the PR objective of replacing protobuf with JSON.
Consider adding a brief comment explaining the transition from protobuf to JSON for future maintainers:
// Note: INTERNAL-PROTOCOL refers to the JSON-based communication protocol that replaced the previous protobuf implementationsource/bringauto/communication/fleet_protocol/FleetProtocol.cpp (3)
12-12: Consider usingnlohmann::jsoninstead ofordered_jsonUnless the order of JSON object keys is crucial for your application, you might consider using
nlohmann::jsoninstead ofnlohmann::ordered_jsonfor potential performance benefits.
146-152: Simplify loop using range-based for loopYou can simplify the loop by using a range-based for loop, which improves readability.
Apply this diff to refactor the loop:
- for(int i = 0; i < commandJson.at("stops").size(); i++) { - osm::Route::Station station; - station.name = commandJson.at("stops").at(i).at("name"); - station.latitude = commandJson.at("stops").at(i).at("position").at("latitude"); - station.longitude = commandJson.at("stops").at(i).at("position").at("longitude"); - missions.push_back(station); + for (const auto& stopJson : commandJson["stops"]) { + osm::Route::Station station; + station.name = stopJson.at("name"); + station.latitude = stopJson.at("position").at("latitude"); + station.longitude = stopJson.at("position").at("longitude"); + missions.push_back(station); }
181-181: Avoid logging potentially large JSON stringsLogging
statusJson.dump()may output large strings and impact performance or clutter logs. Consider logging a summary or specific fields instead.Apply this diff to modify the log statement:
- settings::Logger::logInfo("Generating car status: {}", statusJson.dump()); + settings::Logger::logInfo("Generated car status with state: {}", statusJson["state"]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- CMakeLists.txt (0 hunks)
- README.md (0 hunks)
- cmake/Dependencies.cmake (0 hunks)
- main.cpp (1 hunks)
- source/bringauto/communication/fleet_protocol/FleetProtocol.cpp (3 hunks)
- source/bringauto/settings/SettingsParser.cpp (1 hunks)
- test/CMakeLists.txt (0 hunks)
💤 Files with no reviewable changes (4)
- CMakeLists.txt
- README.md
- cmake/Dependencies.cmake
- test/CMakeLists.txt
🧰 Additional context used
🔇 Additional comments (5)
main.cpp (1)
15-15: LGTM: Addition of headerThe inclusion of is appropriate for the new error handling mechanism using std::cerr. This change aligns with the enhanced error reporting mentioned in the summary.
source/bringauto/settings/SettingsParser.cpp (3)
Line range hint
1-424: Summary of changes and recommendationsThe changes in this file successfully update the command-line options and their descriptions to reflect the transition from protobuf to JSON, which aligns with the PR objective. However, there are a few areas that require attention:
- Consider adding a comment explaining the transition from protobuf to JSON for clarity.
- Verify if any updates are needed in the
areCmdArgumentsCorrectmethod to ensure it correctly validates all command-line arguments after the transition.- Review the implementation of
fillFleetSettingsandfillInternalProtocolSettingsto confirm they properly handle the new JSON-based fleet provider settings.These verifications will ensure that the
SettingsParserclass fully supports the new JSON-based approach and maintains consistency throughout the codebase.
Line range hint
321-346: Verify implementation offillFleetSettingsandfillInternalProtocolSettingsWhile no changes are visible in the provided diff for these methods, it's crucial to ensure they correctly handle the new JSON-based fleet provider settings.
Please review the implementation of
fillFleetSettingsandfillInternalProtocolSettingsto confirm they properly handle the transition from protobuf to JSON. Specifically, check if:
- The
FLEET_PROVIDERenum values are up-to-date.- The
INTERNAL_PROTOCOL_SETTINGSare correctly parsed and set.#!/bin/bash # Description: Verify the implementation of fillFleetSettings and fillInternalProtocolSettings # Test: Check the FleetProvider enum definition rg --type cpp 'enum class FleetProvider' # Test: Examine the fillFleetSettings and fillInternalProtocolSettings methods rg --type cpp -A 20 'void SettingsParser::fillFleetSettings' rg --type cpp -A 20 'void SettingsParser::fillInternalProtocolSettings'
Line range hint
107-160: Verify if updates are needed inareCmdArgumentsCorrectWhile no changes were made to this method, it's important to ensure that it still correctly validates all command-line arguments after the transition from protobuf to JSON.
Please review if any updates are needed in the
areCmdArgumentsCorrectmethod, particularly in theallParametersvector, to reflect any changes in the available command-line options.source/bringauto/communication/fleet_protocol/FleetProtocol.cpp (1)
7-7: Including the JSON header fileThe inclusion of
<nlohmann/json.hpp>is necessary for JSON parsing functionality.
source/bringauto/communication/fleet_protocol/FleetProtocol.cpp
Outdated
Show resolved
Hide resolved
source/bringauto/communication/fleet_protocol/FleetProtocol.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
include/bringauto/osm/Route.hpp (1)
28-30: Approve changes with suggestions for improvementThe initialization of
Stationstruct members is a good practice that prevents undefined behavior. However, consider the following suggestions:
- For
latitudeandlongitude, consider using named constants or a special value to indicate an uninitialized state, as 0.0 might be a valid coordinate in some contexts.- Add validation in the constructor or setter methods to ensure
latitudeis between -90 and 90, andlongitudeis between -180 and 180.- Consider using a more descriptive default value for
name, such as "Unnamed Station", if appropriate for your use case.Here's a potential improvement:
struct Station { static constexpr double INVALID_COORDINATE = std::numeric_limits<double>::quiet_NaN(); std::string name {"Unnamed Station"}; double latitude { INVALID_COORDINATE }; double longitude { INVALID_COORDINATE }; void setCoordinates(double lat, double lon) { if (lat < -90 || lat > 90 || lon < -180 || lon > 180) { throw std::invalid_argument("Invalid coordinates"); } latitude = lat; longitude = lon; } };This approach uses NaN as an invalid state and adds validation when setting coordinates.
CMakeLists.txt (1)
6-6: Version update looks good, consider updating documentation.The version bump from 3.2.0 to 3.3.0 is appropriate for the changes described in the PR objectives. This minor version increase correctly reflects the transition from Protobuf to JSON, which is a significant change but doesn't break backwards compatibility.
Don't forget to update any relevant documentation, changelogs, or release notes to reflect this version change and the switch from Protobuf to JSON.
source/bringauto/communication/fleet_protocol/FleetProtocol.cpp (3)
12-12: Consider usingnlohmann::jsoninstead ofnlohmann::ordered_jsonIf the insertion order of the JSON elements is not required, you can use
nlohmann::jsoninstead ofnlohmann::ordered_jsonfor better performance.
175-175: Add a comment to explain the hardcoded fuel valueEven though the hardcoded fuel value is used for testing purposes, consider adding a comment to clarify its intent for future maintainers.
Apply this diff to add a comment:
statusJson["telemetry"]["speed"] = status.getSpeed(); + // Hardcoded fuel value for testing purposes statusJson["telemetry"]["fuel"] = 0.42;
176-179: Document the behavior ofstatus.getNextStop()Since
status.getNextStop()always returns an object with default-initialized members, adding documentation or a comment can inform future developers that it's safe to access its members without null checks.Would you like assistance in adding appropriate documentation or comments to clarify this behavior?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- CMakeLists.txt (1 hunks)
- include/bringauto/communication/Status.hpp (1 hunks)
- include/bringauto/osm/Route.hpp (1 hunks)
- source/bringauto/communication/fleet_protocol/FleetProtocol.cpp (3 hunks)
✅ Files skipped from review due to trivial changes (1)
- include/bringauto/communication/Status.hpp
🧰 Additional context used
📓 Learnings (1)
source/bringauto/communication/fleet_protocol/FleetProtocol.cpp (6)
Learnt from: MarioIvancik PR: bringauto/virtual-vehicle#11 File: source/bringauto/communication/fleet_protocol/FleetProtocol.cpp:171-179 Timestamp: 2024-10-17T07:39:59.450Z Learning: In the virtual vehicle, there is no telemetry for fuel, and the value is used purely for testing purposes.Learnt from: MarioIvancik PR: bringauto/virtual-vehicle#11 File: source/bringauto/communication/fleet_protocol/FleetProtocol.cpp:177-177 Timestamp: 2024-10-17T08:35:13.267Z Learning: In the C++ file `source/bringauto/communication/fleet_protocol/FleetProtocol.cpp`, the method `status.getNextStop()` always returns an object with default-initialized members, so accessing its members without null checks is safe.Learnt from: MarioIvancik PR: bringauto/virtual-vehicle#11 File: source/bringauto/communication/fleet_protocol/FleetProtocol.cpp:133-133 Timestamp: 2024-10-17T08:03:08.175Z Learning: In the `FleetProtocol::processBufferData` method in `FleetProtocol.cpp`, null `bufferData.data` is acceptable because `json::parse` handles it appropriately, so an explicit null check is unnecessary.Learnt from: MarioIvancik PR: bringauto/virtual-vehicle#11 File: source/bringauto/communication/fleet_protocol/FleetProtocol.cpp:146-156 Timestamp: 2024-10-17T08:16:09.783Z Learning: In the `FleetProtocol::processBufferData` method in `FleetProtocol.cpp`, it's acceptable to use `at()` for accessing JSON fields without prior checks, as commands are validated before being sent to the virtual vehicle, and `at()` throws an exception if a key is missing.Learnt from: MarioIvancik PR: bringauto/virtual-vehicle#11 File: source/bringauto/communication/fleet_protocol/FleetProtocol.cpp:155-155 Timestamp: 2024-10-17T07:23:23.169Z Learning: Commands are validated before being sent to the virtual vehicle.Learnt from: MarioIvancik PR: bringauto/virtual-vehicle#11 File: source/bringauto/communication/fleet_protocol/FleetProtocol.cpp:155-155 Timestamp: 2024-10-17T07:23:23.169Z Learning: The `valueToEnum` function sets the action to "invalid" if the received string is not recognized.
🔇 Additional comments (2)
include/bringauto/osm/Route.hpp (1)
28-30: Verify JSON serialization/deserialization for Station structGiven that this PR is part of replacing protobuf with JSON, it's important to ensure that the
Stationstruct can be properly serialized to and deserialized from JSON format.Please confirm that:
- There is JSON serialization/deserialization logic implemented for the
Stationstruct.- The default values (empty string for name, 0.0 for coordinates) are handled appropriately in the JSON conversion process.
- Any JSON parsing code correctly handles missing fields by using these default values.
Consider running the following script to check for JSON-related code:
This will help ensure that the changes to the
Stationstruct are properly integrated with the new JSON-based system.CMakeLists.txt (1)
Line range hint
1-143: Verify Protobuf removal and its impact on CMakeLists.txtThe PR objectives mention replacing Protobuf with JSON, but the visible changes in this file don't reflect any Protobuf-related removals. To ensure consistency and completeness of the changes:
- Confirm that all Protobuf-related configurations have been removed from this file if they existed previously.
- Verify that any dependencies or libraries related to Protobuf (e.g.,
protobuff_lib) have been removed from theTARGET_LINK_LIBRARIESsection.- Check if any Protobuf-related
FIND_PACKAGEcommands have been removed.To help verify these changes, you can run the following script:
This script will help identify any remaining Protobuf references, show any uncommitted changes, and display recent commit history for this file.
✅ Verification successful
Protobuf removal in CMakeLists.txt verified successfully. No Protobuf-related configurations or dependencies remain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining Protobuf-related configurations in CMakeLists.txt # Test: Search for Protobuf-related keywords echo "Searching for Protobuf-related keywords in CMakeLists.txt:" rg -i 'protobuf|protobuff' CMakeLists.txt # Test: Check if there are any uncommitted changes in CMakeLists.txt echo "Checking for uncommitted changes in CMakeLists.txt:" git diff -- CMakeLists.txt # Test: Show the git history for CMakeLists.txt to see recent changes echo "Recent changes to CMakeLists.txt:" git log -n 5 --pretty=format:"%h - %s" -- CMakeLists.txtLength of output: 744
source/bringauto/communication/fleet_protocol/FleetProtocol.cpp
Outdated
Show resolved
Hide resolved
|
Fixes:
https://youtrack.bringauto.com/issue/BAF-874/Virtual-Vehicle-cannot-parse-command
Depends on:
bringauto/mission-module#19
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores