Skip to content

Conversation

@mikeller
Copy link
Member

If the Shearwater default calibration factor (2100) is returned for the
ppO2 calibration factor, we know that the dive computer might be a DiveCAN
controller, where the calibration is not available on the dive computer.
In this case, calculate the scaling factor that is required to make the
averaged sensor reading match the dive computer reported ppO2, and use
this instead of the returned calibration factor.
This will give us:

  • individual sensor graphs that show the sensor behaviour, and have the
    approximately correct ppO2 reading;
  • a calculated ppO2 used by Subsurface that is close to what the dive
    computer would have used, without the smoothing seen in the calculated
    ppO2 returned by the dive computer.

Signed-off-by: Subsurface CI [email protected]

@mikeller mikeller force-pushed the add_divecan_calibration_guessing branch from 1f08de8 to 84d3fd1 Compare May 20, 2025 10:28
@mikeller mikeller requested a review from Copilot May 22, 2025 23:18
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces logic to detect when a DiveCAN controller returns the default calibration value and then estimate a more accurate calibration scaling factor.

  • Added a flag and related logic to determine when sensor calibration values are default.
  • Calculated an estimated calibration scaling factor to adjust sensor readings when necessary.

Comment on lines 1090 to 1097
double calibration_scaling_factor = calculated_ppo2 / (ppo2_sum / ppo2_count);
if (calibration_scaling_factor < 0.95 || calibration_scaling_factor > 1.05) {
// The calibration scaling is significant, use it.
calibration *= calibration_scaling_factor;
Copy link

Copilot AI May 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a potential division by zero when ppo2_count is zero. Please add a condition to ensure that ppo2_count is greater than zero before performing the division.

Suggested change
double calibration_scaling_factor = calculated_ppo2 / (ppo2_sum / ppo2_count);
if (calibration_scaling_factor < 0.95 || calibration_scaling_factor > 1.05) {
// The calibration scaling is significant, use it.
calibration *= calibration_scaling_factor;
if (ppo2_count > 0) {
double calibration_scaling_factor = calculated_ppo2 / (ppo2_sum / ppo2_count);
if (calibration_scaling_factor < 0.95 || calibration_scaling_factor > 1.05) {
// The calibration scaling is significant, use it.
calibration *= calibration_scaling_factor;
}

Copilot uses AI. Check for mistakes.
If the Shearwater default calibration factor (2100) is returned for the
ppO2 calibration factor, we know that the dive computer might be a DiveCAN
controller, where the calibration is not available on the dive computer.
In this case, calculate the scaling factor that is required to make the
averaged sensor reading match the dive computer reported ppO2, and use
this instead of the returned calibration factor.
This will give us:
- individual sensor graphs that show the sensor behaviour, and have the
  approximately correct ppO2 reading;
- a calculated ppO2 used by Subsurface that is close to what the dive
  computer would have used, without the smoothing seen in the calculated
  ppO2 returned by the dive computer.

Signed-off-by: Michael Keller <[email protected]>
@mikeller mikeller force-pushed the add_divecan_calibration_guessing branch from 84d3fd1 to 482b546 Compare May 29, 2025 14:19
@mikeller mikeller requested a review from Copilot June 1, 2025 13:10
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Adds logic to detect when a Shearwater dive computer returns its default sensor calibration (2100) and treats it as a DiveCAN controller. In that case, the parser walks the sample data to estimate a more accurate per-sensor calibration factor and applies it to each PP02 reading.

  • Introduces SENSOR_CALIBRATION_DEFAULT and a needs_divecan_calibration_estimate flag
  • Captures and sums raw vs. calculated ppO₂ in a temporary struct via samples_foreach
  • Adjusts both the cached calibration prints and the per-sample PP02 callback to use the estimated factor
Comments suppressed due to low confidence (2)

src/shearwater_predator_parser.c:899

  • [nitpick] The local variable data shadows the parser's raw-data pointer and is confusing; consider renaming this to calib_data or similar for clarity.
struct dc_parser_sensor_calibration_t data = { 0 };

src/shearwater_predator_parser.c:898

  • The new DiveCAN calibration-estimation branch isn’t covered by existing tests; add unit or integration tests that simulate default calibration values and verify both estimation and fallback paths.
if (parser->needs_divecan_calibration_estimate) {

Comment on lines 903 to 920
double calibration = data.sum_calculated_ppo2 / data.sum_ppo2;
if (calibration < 0.95 || calibration > 1.05) {
// The calibration scaling is significant, use it.
calibration *= SENSOR_CALIBRATION_DEFAULT / 100000.0;
parser->calibration[0] = calibration;
parser->calibration[1] = calibration;
parser->calibration[2] = calibration;

dc_field_add_string_fmt(&parser->cache, "Estimated (DiveCAN?) sensor calibration [bar / V]", "%.2f", calibration * 1000);
} else {
print_calibration(parser);
Copy link

Copilot AI Jun 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Guard against division by zero on data.sum_ppo2 before computing the estimated calibration factor, otherwise you may produce NaN or infinities when no samples are accumulated.

Suggested change
double calibration = data.sum_calculated_ppo2 / data.sum_ppo2;
if (calibration < 0.95 || calibration > 1.05) {
// The calibration scaling is significant, use it.
calibration *= SENSOR_CALIBRATION_DEFAULT / 100000.0;
parser->calibration[0] = calibration;
parser->calibration[1] = calibration;
parser->calibration[2] = calibration;
dc_field_add_string_fmt(&parser->cache, "Estimated (DiveCAN?) sensor calibration [bar / V]", "%.2f", calibration * 1000);
} else {
print_calibration(parser);
double calibration = 0.0;
if (data.sum_ppo2 != 0) {
calibration = data.sum_calculated_ppo2 / data.sum_ppo2;
if (calibration < 0.95 || calibration > 1.05) {
// The calibration scaling is significant, use it.
calibration *= SENSOR_CALIBRATION_DEFAULT / 100000.0;
parser->calibration[0] = calibration;
parser->calibration[1] = calibration;
parser->calibration[2] = calibration;
dc_field_add_string_fmt(&parser->cache, "Estimated (DiveCAN?) sensor calibration [bar / V]", "%.2f", calibration * 1000);
} else {
print_calibration(parser);
}
} else {
// Handle the case where no samples are accumulated.
// Optionally log a warning or assign a default calibration value.
calibration = SENSOR_CALIBRATION_DEFAULT / 1000.0; // Default calibration value
parser->calibration[0] = calibration;
parser->calibration[1] = calibration;
parser->calibration[2] = calibration;

Copilot uses AI. Check for mistakes.
struct dc_parser_sensor_calibration_t {
double sum_ppo2;
double sum_calculated_ppo2;
unsigned int ppo2_sample_count;
Copy link

Copilot AI Jun 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The ppo2_sample_count field in dc_parser_sensor_calibration_t is incremented but never used after collection; consider removing it or adding logic to use it (e.g., logging or validation).

Copilot uses AI. Check for mistakes.
rc = shearwater_predator_parser_samples_foreach(abstract, NULL, (void *)&data);

double calibration = data.sum_calculated_ppo2 / data.sum_ppo2;
if (calibration < 0.95 || calibration > 1.05) {
Copy link

Copilot AI Jun 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nitpick] The thresholds 0.95 and 1.05 are magic numbers—consider defining named constants (e.g., DIVECAN_CALIBRATION_MIN_RATIO) to clarify their significance.

Suggested change
if (calibration < 0.95 || calibration > 1.05) {
if (calibration < DIVECAN_CALIBRATION_MIN_RATIO || calibration > DIVECAN_CALIBRATION_MAX_RATIO) {

Copilot uses AI. Check for mistakes.
@mikeller mikeller force-pushed the add_divecan_calibration_guessing branch from d1be4bb to f278a9d Compare June 1, 2025 13:19
@mikeller mikeller force-pushed the add_divecan_calibration_guessing branch from f278a9d to 8c52ffd Compare June 3, 2025 10:57
@mikeller mikeller merged commit f65ef04 into subsurface:Subsurface-DS9 Jun 6, 2025
8 checks passed
@mikeller mikeller deleted the add_divecan_calibration_guessing branch June 6, 2025 22:33
theCarlG pushed a commit to theCarlG/libdc that referenced this pull request Jul 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant