feat(ars548): updated the driver according to new documentation #253
Conversation
Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>
|
Will open the PR once I confirm it works on the bench (need to delete some prints as well) |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #253 +/- ##
==========================================
- Coverage 27.18% 27.00% -0.18%
==========================================
Files 104 105 +1
Lines 9464 9527 +63
Branches 2326 2339 +13
==========================================
Hits 2573 2573
- Misses 6455 6518 +63
Partials 436 436
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>
Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>
Signed-off-by: Kenzo Lobos-Tsunekawa <[email protected]>
| std::size_t capacity_; | ||
| std::size_t size_{0}; |
There was a problem hiding this comment.
These are already members of buffer_, so storing them here is redundant and I'd suggest removing them.
| print_info("cycle_time = " + std::to_string(cycle_time_ms)); | ||
| print_info("time_slot = " + std::to_string(time_slot_ms)); |
There was a problem hiding this comment.
It might be more readable to print " ms" after these values
| using nebula::drivers::continental_ars548::max_odometry_hz; | ||
| using nebula::drivers::continental_ars548::min_odometry_hz; | ||
|
|
||
| if (last_odometry_stamp_ == 0.0) { | ||
| last_odometry_stamp_ = msg->header.stamp.sec + msg->header.stamp.nanosec * 1e-9; | ||
| } else { | ||
| double current_time = msg->header.stamp.sec + msg->header.stamp.nanosec * 1e-9; | ||
| double dt = current_time - last_odometry_stamp_; | ||
| last_odometry_stamp_ = current_time; | ||
| odometry_ring_buffer_.push_back(dt); | ||
| } | ||
|
|
||
| double estimated_hz = 1.0 / odometry_ring_buffer_.get_average(); | ||
|
|
||
| if ( | ||
| odometry_ring_buffer_.is_full() && (estimated_hz < static_cast<double>(min_odometry_hz) || | ||
| estimated_hz > static_cast<double>(max_odometry_hz))) { | ||
| rclcpp::Clock clock{RCL_ROS_TIME}; | ||
| RCLCPP_WARN_THROTTLE( | ||
| logger_, clock, 5000, | ||
| "The current odometry rate is %.2f Hz. The sensor requires a rate in the range of 10Hz to " | ||
| "50Hz", | ||
| estimated_hz); | ||
| } | ||
|
|
There was a problem hiding this comment.
This code is duplicated two more times below, so I'd suggest to refactor this into its own class (including last_odometry_stamp_ and odometry_ring_buffer_, called something like CallbackRateLimiter or similar).
| "The current odometry rate is %.2f Hz. The sensor requires a rate in the range of 10Hz to " | ||
| "50Hz", |
There was a problem hiding this comment.
Please print the actual constants here instead of hard-coded 10Hz and 50Hz.
| class RingBuffer | ||
| { | ||
| public: | ||
| explicit RingBuffer(std::size_t capacity) : capacity_(capacity) { buffer_.resize(capacity_); } |
There was a problem hiding this comment.
resize initializes all values to T's default initialization (0 for numeric types) and could be replaced by reserve instead, removing the need for size_ = std::min(size_ + 1, capacity_); in L39.
| uint16 cycle_time | ||
| uint16 time_slot | ||
| uint16 country_code | ||
| string frequency_band |
There was a problem hiding this comment.
For usability, could you add string constants like string FREQUENCY_BAND_LOW="low" etc. to this definition?
Same for country_code 🙇
PR Type
Related Links
Description
Review Procedure
Remarks
Pre-Review Checklist for the PR Author
PR Author should check the checkboxes below when creating the PR.
Checklist for the PR Reviewer
Reviewers should check the checkboxes below before approval.
Post-Review Checklist for the PR Author
PR Author should check the checkboxes below before merging.
CI Checks