Add: benchmark for ping1d and ping360 on CI by RaulTrombin · Pull Request #62 · bluerobotics/ping-rs

RaulTrombin · 2024-10-04T20:51:52Z

helps

RaulTrombin · 2024-11-24T16:22:08Z

@patrickelectric @joaoantoniocardoso Should we add this in the near future? Benchmarking in navigator-rs helped improve sensor readings and reliability.

ES-Alexander

Reviewed the calculation part - as requested from internal discussion :-)

The calculation logic seems generally ok (except maybe for the min_sample_based variable?), but I feel like it belongs somewhere accessible to users.

Some comments around code cleanliness and maintainability.

ES-Alexander · 2026-01-21T19:47:56Z

+const FIRMWARE_MIN_TRANSMIT_DURATION: u16 = 1;
+
+fn calculate_sample_period(desired_range: f64, number_of_samples: u16, speed_of_sound: f64) -> u16 {
+    const SAMPLE_PERIOD_TICK_DURATION: f64 = 25e-9;


Suggested change

const SAMPLE_PERIOD_TICK_DURATION: f64 = 25e-9;

ES-Alexander · 2026-01-21T19:56:14Z

+    ((2.0 * desired_range)
+        / (number_of_samples as f64 * speed_of_sound * SAMPLE_PERIOD_TICK_DURATION))
+        .ceil() as u16


Suggested change

((2.0 * desired_range)

/ (number_of_samples as f64 * speed_of_sound * SAMPLE_PERIOD_TICK_DURATION))

.ceil() as u16

// Ensure sample period covers the full desired range

((2.0 * desired_range)

/ (speed_of_sound * number_of_samples as f64 * SAMPLE_PERIOD_TICK_DURATION))

.ceil() as u16

Explanatory comment for why ceil is desirable

I think it is easier to follow if the sample-related variables are next to each other (assuming the code still works fine like that - I'm not very familiar with Rust)

ES-Alexander · 2026-01-21T19:59:15Z

+fn calculate_transmit_duration_max(sample_period: u16) -> u16 {
+    let sample_based_max = sample_period as f64 * 64.0 * SAMPLE_PERIOD_TICK_DURATION * 1e6;
+    sample_based_max.min(FIRMWARE_MAX_TRANSMIT_DURATION as f64) as u16
+}


This should presumably have some kind of doc-string/comment to explain why there is a maximum?

ES-Alexander · 2026-01-21T20:06:13Z

+const SAMPLE_PERIOD_TICK_DURATION: f64 = 25e-9;
+const FIRMWARE_MAX_TRANSMIT_DURATION: u16 = 1000;
+const FIRMWARE_MIN_TRANSMIT_DURATION: u16 = 1;


These seem like they should have unit information (e.g. in the variable names in not SI units, and/or in explanatory comments).

ES-Alexander · 2026-01-21T20:20:26Z

+fn calculate_transmit_duration(range: f64, speed_of_sound: f64, sample_period: u16) -> u16 {
+    // Calculate initial value
+    let mut auto_duration = ((8000.0 * range) / speed_of_sound).round();
+
+    // Ensure minimum based on sample period (convert to microseconds)
+    let min_sample_based = (2.5 * sample_period as f64 * SAMPLE_PERIOD_TICK_DURATION * 1e6).ceil();
+    auto_duration = auto_duration.max(min_sample_based).round();
+
+    // Clamp between FIRMWARE_MIN_TRANSMIT_DURATION and max based on sample period
+    let max_duration = calculate_transmit_duration_max(sample_period);
+    auto_duration = auto_duration
+        .max(FIRMWARE_MIN_TRANSMIT_DURATION as f64)
+        .min(max_duration as f64);
+
+    auto_duration.round() as u16
+}


Code with no explanation is hard to follow, evaluate, and maintain.

ES-Alexander · 2026-01-21T20:32:07Z

+const SAMPLE_PERIOD_TICK_DURATION: f64 = 25e-9;
+const FIRMWARE_MAX_TRANSMIT_DURATION: u16 = 1000;
+const FIRMWARE_MIN_TRANSMIT_DURATION: u16 = 1;
+
+fn calculate_sample_period(desired_range: f64, number_of_samples: u16, speed_of_sound: f64) -> u16 {
+    const SAMPLE_PERIOD_TICK_DURATION: f64 = 25e-9;
+    ((2.0 * desired_range)
+        / (number_of_samples as f64 * speed_of_sound * SAMPLE_PERIOD_TICK_DURATION))
+        .ceil() as u16
+}
+
+fn calculate_transmit_duration_max(sample_period: u16) -> u16 {
+    let sample_based_max = sample_period as f64 * 64.0 * SAMPLE_PERIOD_TICK_DURATION * 1e6;
+    sample_based_max.min(FIRMWARE_MAX_TRANSMIT_DURATION as f64) as u16
+}
+
+fn calculate_transmit_duration(range: f64, speed_of_sound: f64, sample_period: u16) -> u16 {
+    // Calculate initial value
+    let mut auto_duration = ((8000.0 * range) / speed_of_sound).round();
+
+    // Ensure minimum based on sample period (convert to microseconds)
+    let min_sample_based = (2.5 * sample_period as f64 * SAMPLE_PERIOD_TICK_DURATION * 1e6).ceil();
+    auto_duration = auto_duration.max(min_sample_based).round();
+
+    // Clamp between FIRMWARE_MIN_TRANSMIT_DURATION and max based on sample period
+    let max_duration = calculate_transmit_duration_max(sample_period);
+    auto_duration = auto_duration
+        .max(FIRMWARE_MIN_TRANSMIT_DURATION as f64)
+        .min(max_duration as f64);
+
+    auto_duration.round() as u16
+}


All of this seems like potential convenience functionality that should likely be available in the ping360 part of the library, not hidden away in a benchmark.

Do the sample period and transmit duration not need to be calculated as part of ping-viewer functionality?

ES-Alexander · 2026-01-21T20:37:53Z

+
+    // Ensure minimum based on sample period (convert to microseconds)
+    let min_sample_based = (2.5 * sample_period as f64 * SAMPLE_PERIOD_TICK_DURATION * 1e6).ceil();
+    auto_duration = auto_duration.max(min_sample_based).round();


Suggested change

auto_duration = auto_duration.max(min_sample_based).round();

auto_duration = auto_duration.max(min_sample_based);

Not necessary if the inputs have already been rounded or ceiled, right?

ES-Alexander · 2026-01-21T20:44:34Z

+    let mut auto_duration = ((8000.0 * range) / speed_of_sound).round();
+
+    // Ensure minimum based on sample period (convert to microseconds)
+    let min_sample_based = (2.5 * sample_period as f64 * SAMPLE_PERIOD_TICK_DURATION * 1e6).ceil();


It's hard to be sure without units specified, but it seems like this differs from Ping Viewer by 1e9 (i.e. * 1e6 instead of / 1000)?

RaulTrombin force-pushed the bench branch 2 times, most recently from ead2d29 to a98fbea Compare October 4, 2024 21:00

RaulTrombin requested review from joaoantoniocardoso and patrickelectric October 4, 2024 21:04

RaulTrombin force-pushed the bench branch 2 times, most recently from 899602f to ed531f5 Compare January 13, 2026 17:06

benches: Add bench for hardware in loop devices, ping1d and ping360

b5476c0

RaulTrombin force-pushed the bench branch from ed531f5 to b5476c0 Compare January 13, 2026 22:45

ES-Alexander suggested changes Jan 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add: benchmark for ping1d and ping360 on CI#62

Add: benchmark for ping1d and ping360 on CI#62
RaulTrombin wants to merge 1 commit into
bluerobotics:masterfrom
RaulTrombin:bench

RaulTrombin commented Oct 4, 2024

Uh oh!

RaulTrombin commented Nov 24, 2024

Uh oh!

ES-Alexander left a comment

Uh oh!

ES-Alexander Jan 21, 2026

Uh oh!

ES-Alexander Jan 21, 2026

Uh oh!

ES-Alexander Jan 21, 2026

Uh oh!

ES-Alexander Jan 21, 2026

Uh oh!

ES-Alexander Jan 21, 2026

Uh oh!

ES-Alexander Jan 21, 2026

Uh oh!

ES-Alexander Jan 21, 2026

Uh oh!

ES-Alexander Jan 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	auto_duration = auto_duration.max(min_sample_based).round();
	auto_duration = auto_duration.max(min_sample_based);

Conversation

RaulTrombin commented Oct 4, 2024

Uh oh!

RaulTrombin commented Nov 24, 2024

Uh oh!

ES-Alexander left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants