Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new integration-test executable intended to automatically validate localization output from WPILib DataLog files, and cleans up a minor formatting issue in an existing localization integration test.
Changes:
- Fixes a trailing formatting/line issue in
localization_test2.cc. - Adds
auto_check_localizationintegration-test executable and wires it into the integration-test CMake target list. - Implements initial log-reading + yaw-threshold checking logic for localization verification.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/test/integration_test/localization_test2.cc |
Minor formatting/line cleanup at end of main(). |
src/test/integration_test/CMakeLists.txt |
Adds new auto_check_localization executable target. |
src/test/integration_test/auto_check_localization.cc |
New integration test: reads .dlog and checks Pose3d yaw against a threshold. |
| bool decode_record(const wpi::log::DataLogRecord& record, | ||
| const wpi::log::StartRecordData& start_data, | ||
| NTValue& out_value) { |
There was a problem hiding this comment.
start_data is an unused parameter in decode_record(), which will trigger -Wunused-parameter (and fail the build with warnings-as-errors). Either remove this parameter (and stop calling reader.GetStart(...)), or explicitly mark it unused (e.g., omit the name or cast to void) if it will be needed later for type-based decoding.
| absl::ParseCommandLine(argc, argv); | ||
|
|
||
| std::string log_path = "path_to_log.dlog"; | ||
| wpi::log::DataLogReader reader(log_path); |
There was a problem hiding this comment.
absl::ParseCommandLine() is called but log_path is hard-coded to "path_to_log.dlog", so CLI args currently cannot select a log file. Consider introducing an ABSL_FLAG for the log path (and validating it exists) to make this integration test runnable in CI and locally without editing source.
| std::map<std::string, std::vector<std::pair<int64_t, NTValue>>> nt_timeseries; | ||
|
|
||
| for (const auto& record : reader) { | ||
| if (record.type != wpi::log::RecordType::kData) continue; | ||
|
|
||
| NTValue value; | ||
| if (!decode_record(record, reader.GetStart(record.name), value)) continue; | ||
|
|
||
| nt_timeseries[std::string(record.name)].emplace_back(record.timestamp, value); |
There was a problem hiding this comment.
This builds a full in-memory nt_timeseries of every entry/value in the log even though the later check only cares about Pose3d yaw. For large .dlog files this can be unnecessarily memory-heavy; consider performing the yaw-threshold check while streaming records (and only storing series when actually needed).
| } | ||
|
|
||
| double delta_rot_threshold = 0.05; | ||
| for (auto& [name, series] : nt_timeseries) { |
There was a problem hiding this comment.
In this loop the structured-binding variable name is never used, which will typically trigger -Wunused-variable under -Wall and fail the build with warnings-as-errors. Either use name (e.g., include it in the output) or iterate only over the map values to avoid introducing an unused binding.
| for (auto& [name, series] : nt_timeseries) { | |
| for (auto& entry : nt_timeseries) { | |
| auto& series = entry.second; |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Viba Mohan <happymorning552@gmail.com>
No description provided.