Add encoder calibration check utility and documentation#57
Add encoder calibration check utility and documentation#57GiulioRomualdi merged 9 commits intomainfrom
Conversation
- Implemented `check-encoder-calibration` utility to compare live encoder readings against a reference TOML snapshot. - Added Markdown report generation for calibration results. - Updated Doxygen documentation to include the new utility. - Modified CMake configuration to include the new utility.
There was a problem hiding this comment.
Pull request overview
This PR adds a new check-encoder-calibration utility that reads live encoder positions from EtherCAT slaves via SDO and compares them against a previously saved TOML reference snapshot, producing a Markdown report showing the differences.
Changes:
- Implemented
CheckEncoderCalibrationclass (with PIMPL idiom) that parses a reference TOML file, reads live encoder data from EtherCAT slaves, and generates a detailed Markdown comparison report. - Added comprehensive documentation including a README, Doxygen page, and updates to the main documentation index.
- Updated CMake and Doxygen configuration to include the new utility.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/CheckEncoderCalibration/src/main.cpp | Entry point for the new utility |
| utils/CheckEncoderCalibration/src/CheckEncoderCalibration.cpp | Core implementation: TOML parsing, SDO reads, Markdown report generation |
| utils/CheckEncoderCalibration/include/CheckEncoderCalibration/CheckEncoderCalibration.h | Public header with class declaration and Doxygen documentation |
| utils/CheckEncoderCalibration/README.md | User-facing documentation for the utility |
| utils/CheckEncoderCalibration/CMakeLists.txt | Build configuration for the new utility |
| utils/CMakeLists.txt | Adds the new subdirectory to the build |
| doc/main.md | Adds the new utility to the main documentation page |
| doc/check_encoder_calibration.md | Doxygen documentation page for the utility |
| doc/Doxyfile | Includes new utility sources and docs in Doxygen input |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const auto now = std::chrono::system_clock::now(); | ||
| const std::time_t t = std::chrono::system_clock::to_time_t(now); | ||
| std::tm tm{}; | ||
| localtime_r(&t, &tm); |
There was a problem hiding this comment.
localtime_r is POSIX-specific and not available on Windows. The same call appears on line 404. If cross-platform support is ever needed, consider using a portable alternative. This is consistent with the README's note that it's Linux-only, but worth noting.
There was a problem hiding this comment.
You are right!
| // ---- Helper: format an integer (with explicit sign for deltas) ---- | ||
| static std::string fmtInt64(int64_t v) | ||
| { | ||
| return std::to_string(v); |
There was a problem hiding this comment.
fmtInt64 is a trivial wrapper around std::to_string that adds no formatting logic (unlike fmtDeltaInt64 which adds a sign prefix). Consider removing it and calling std::to_string directly at the call sites, or adding explicit sign handling to make its purpose clearer and consistent with fmtDeltaInt64.
| return std::to_string(v); | |
| std::ostringstream o; | |
| o << v; | |
| return o.str(); |
|
|
||
| // ---- 4) For each slave, read reference + live data ---- | ||
| std::vector<SlaveReport> reports; | ||
| bool allOk = true; |
There was a problem hiding this comment.
The variable allOk is initialized to true and never set to false in the loop body — even when readEncoderFromToml fails or when SDO reads fail (since readEncoderFromSlave always returns true). This means the function will report success even if multiple slaves had errors. Either remove allOk (since it's not actually tracking errors) or set it to false when encoder reads fail so the caller knows something went wrong.
| bool allOk = true; |
| static bool readEncoderFromSlave(EthercatManager& mgr, | ||
| int slave, | ||
| uint16_t idxConfig, | ||
| uint16_t idxData, | ||
| EncoderChannelData& out) | ||
| { |
There was a problem hiding this comment.
readEncoderFromSlave always returns true (line 142) regardless of whether any SDO reads failed. The return value is also ignored at the call sites (lines 291-292). Either make it return false when critical reads fail, or change the return type to void to avoid misleading callers into thinking the return value is meaningful.
utils/CheckEncoderCalibration/include/CheckEncoderCalibration/CheckEncoderCalibration.h
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
|
||
| using namespace CiA402; | ||
|
|
||
| static std::tm getLocalTime(const std::time_t& t) |
There was a problem hiding this comment.
The same static getLocalTime() is also copied in StoreHomePosition.cpp. Consider extracting it into a shared utility header
….txt and CheckEncoderCalibration to include new headers
|
@isorrentino The PR is ready to be reviewed again |
check-encoder-calibrationutility to compare live encoder readings against a reference TOML snapshot.