Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Shearwater: Fix Sensor Calibration for DiveCAN Controllers. #59

Merged
merged 2 commits into from
May 8, 2024
Merged

Shearwater: Fix Sensor Calibration for DiveCAN Controllers. #59

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e65428
Shearwater: Fix Sensor Calibration for DiveCAN Controllers.
mikeller Apr 28, 2024
a7fb341
Shearwater: Add Sensor Calibration Info.
mikeller May 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions src/shearwater_common.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,8 +58,8 @@ shearwater_common_setup (shearwater_common_device_t *device, dc_context_t *conte
return status;
}

// Set the timeout for receiving data (3000ms).
status = dc_iostream_set_timeout (device->iostream, 3000);
// Set the timeout for receiving data (12s).
status = dc_iostream_set_timeout (device->iostream, 12 * 1000);
if (status != DC_STATUS_SUCCESS) {
ERROR (context, "Failed to set the timeout.");
return status;
Expand Down Expand Up @@ -696,6 +696,9 @@ dc_status_t shearwater_common_get_model(shearwater_common_device_t *device, unsi

// Convert and map to the model number.
unsigned int hardware = array_uint16_be (rsp_hardware);

DEBUG(device->base.context, "Hardware type: 0x%04x", hardware);

switch (hardware) {
case 0x0101:
case 0x0202:
Expand Down
49 changes: 19 additions & 30 deletions src/shearwater_predator_parser.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -418,7 +418,6 @@ shearwater_predator_parser_cache (shearwater_predator_parser_t *parser)
dc_parser_t *abstract = (dc_parser_t *) parser;
const unsigned char *data = parser->base.data;
unsigned int size = parser->base.size;
const char *ppo2_source = NULL;

if (parser->cached) {
return DC_STATUS_SUCCESS;
Expand Down Expand Up @@ -728,38 +727,30 @@ shearwater_predator_parser_cache (shearwater_predator_parser_t *parser)
dc_field_add_string_fmt(&parser->cache, "Logversion", "%d%s", logversion, pnf ? "(PNF)" : "");

// Cache sensor calibration for later use
unsigned int nsensors = 0, ndefaults = 0;
unsigned int base = parser->opening[3] + (pnf ? 6 : 86);
parser->calibrated = data[base];

for (size_t i = 0; i < 3; ++i) {
unsigned int calibration = array_uint16_be(data + base + 1 + i * 2);
parser->calibration[i] = calibration / 100000.0;
if (parser->model == PREDATOR) {
// The Predator expects the mV output of the cells to be
// within 30mV to 70mV in 100% O2 at 1 atmosphere. If the
// calibration value is scaled with a factor 2.2, then the
// sensors lines up and matches the average.
parser->calibration[i] *= 2.2;
}
if (data[base] & (1 << i)) {
if (calibration == 2100) {
ndefaults++;
if (parser->calibrated & (1 << i)) {
unsigned int calibration = array_uint16_be(data + base + 1 + i * 2);
parser->calibration[i] = calibration / 100000.0;
if (parser->model == PREDATOR) {
// The Predator expects the mV output of the cells to be
// within 30mV to 70mV in 100% O2 at 1 atmosphere. If the
// calibration value is scaled with a factor 2.2, then the
// sensors lines up and matches the average.
parser->calibration[i] *= 2.2;
}
nsensors++;

static const char *name[] = {
"Sensor 1 calibration [bar / V]",
"Sensor 2 calibration [bar / V]",
"Sensor 3 calibration [bar / V]",
};
dc_field_add_string_fmt(&parser->cache, name[i], "%.2f", parser->calibration[i] * 1000);

}
}
if (nsensors && nsensors == ndefaults) {
// If all (calibrated) sensors still have their factory default
// calibration values (2100), they are probably not calibrated
// properly. To avoid returning incorrect ppO2 values to the
// application, they are manually disabled (e.g. marked as
// uncalibrated).
Copy link
Collaborator

Choose a reason for hiding this comment

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

the comment seems to explain well why this makes sense.
Am I understanding you correctly that 2100 could, in fact, be a valid calibration value?

Copy link
Contributor

Choose a reason for hiding this comment

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

If I remember correctly, there are certain models (those DiveCAN models?) where the calibration is done externally. Thus the Shearwater doesn't even know the calibration values and in that case all values are set to their default value (2100). Using those defaults values for the calibration anyway, will result in ppO2 values that are wrong and that's why they are manually disabled here. Theoretically it's possible that a real calibration results in a value of 2100, but I think that in practice the chance of ALL values being exactly 2100 is extremely low.

Copy link
Member Author

Choose a reason for hiding this comment

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

Am I understanding you correctly that 2100 could, in fact, be a valid calibration value?

Yes, of course 2100 can be a valid calibration value - it just happens to be the default value, and a value that is within the expected output range for a lot of the sensors used in CCRs (i.e. 10 mV corresponds to a ppO2 of 0.21 bar).

To make matters worse, on Shearwater dive computers that were built with a 'DiveCAN' interface to act as controllers on a number of CCR models, the calibration and conversion to ppO2 is done in the CCR and not in the controller, and these models always assume a 'calibration' of 2100 for all sensors that were detected, and return the cell readings in the assumed mV range that this yields. So before this fix Subsurface is not really usable with these divecomputers, as all CCR ppO2 information for them is discarded - and they are not likely to ever be used in open circuit mode.

the comment seems to explain well why this makes sense.

I don't know - we already read and apply the information that we are getting from the dive computer about which sensors have a valid calibration (stored in calibrated), and ignore the ones that don't.
After all, the dive computer uses the information from the sensors that it thinks are calibrated correctly as input for the voting logic which then yields the assumed ppO2 value that is used to drive the decompression calculation - so discarding these values will result in the decompression information in Subsurface being based on data that is different from what was shown to the diver.
To me it seems to be wrong to wilfully throw away information that was assumed by the dive computer to be accurate, and used in its calculation.
We could maybe add a warning if we get a valid calibration, but all values are 2100 - but since this will apply to all dives logged by a DiveCAN controller, and we currently don't have a way to detect these models (except for this), I have not done this.

Copy link
Contributor

Choose a reason for hiding this comment

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

So you say the DiveCAN models have already adjusted the mV values, such that after the "calibration" with the default value of 2100, the resulting ppO2 value is correct, right? That sounds reasonable.

The problem is that I added this feature after receiving reports from users that the ppO2 values were incorrect. If we remove this again, then those users will get incorrect ppO2 values again. Unfortunately I don't remember whether this were DiveCAN models or not. I'll need to look at the discussions around that time again.

I quickly checked a few of my datasets, and I'm certainly seeing some inconsistent results. For example a voted value of 0.70, while the 3 sensors report 0.78, 0.80 and 0.78. That makes no sense to me. How can the voted value be completely outside the range of the 3 sensors? The voted value is stored directly as ppO2 and doesn't need any calibration, so my only explanation is that these default calibration values for the sensors are wrong here.

Do you happen to have data from a DiveCAN model?

Copy link
Member Author

Choose a reason for hiding this comment

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

So you say the DiveCAN models have already adjusted the mV values, such that after the "calibration" with the default value of 2100, the resulting ppO2 value is correct, right? That sounds reasonable.

Yes, that is what I am seeing when downloading from a DiveCAN controller - which makes sense as this way they can supply ppO2 values in a way that doesn't break the format specification, even without knowing the calibration values.

The problem is that I added this feature after receiving reports from users that the ppO2 values were incorrect. If we remove this again, then those users will get incorrect ppO2 values again. Unfortunately I don't remember whether this were DiveCAN models or not. I'll need to look at the discussions around that time again.

Do you have any logs from dive computers that report all cells as calibrated correctly while supplying incorrect calibration values?
And even in this case, the graph of the recorded ppO2 sensor readings is not completely incorrect - it just has an incorrect scaling in ppO2 attached. And as a rebreather diver I would argue that having this graph with an incorrect scaling is preferable (by a lot) over just throwing away the data because we cannot establish the scaling. A lot of post-dive diagnosis, like identifying a misbehaving sensor, can be done without the scaling.

I quickly checked a few of my datasets, and I'm certainly seeing some inconsistent results. For example a voted value of 0.70, while the 3 sensors report 0.78, 0.80 and 0.78. That makes no sense to me. How can the voted value be completely outside the range of the 3 sensors? The voted value is stored directly as ppO2 and doesn't need any calibration, so my only explanation is that these default calibration values for the sensors are wrong here.

It will probably be worthwhile to look at the sequence of values as graphs instead of point values - from what I can tell a lot of dive computers apply a low pass filter to the result of the sensor reading voting logic before using it for decompression calculations, in order to avoid the TTS / next stop values jumping around wildly when the ppO2 fluctuates. So this point value could be a capture of a situation where the sensor readings are increasing, but the ppO2 used for calculations is lagging behind because of the lowpass filter.

Do you happen to have data from a DiveCAN model?

It's not my computer, but I can ask the owner if he's ok to share.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was guilty for this way back in time. My plan was to be able to do analysis on individual sensor values but I never came around to this.

As I remember it, we don't have real ppo2 values from the sensors, just mV and thus we need the calibration value to convert those to ppo2 to be able to report them.

Also, as I remember it, we will always report the "voted" ppo2 value if we have it, which as far as I remember we should always have for dives with sensors, and that value is stored as ppo2.

The current sensor interface doesn't expose the raw mV values. We could adjust the interface to expose those, so one can graph those and look at those even if we don't have valid calibration values.

Maybe they save the DiveCAN calibration values somewhere else? Or maybe they just get the ppo2 and mV values over DiveCAN and just saves the mV and the voted ppo2, in which case we can't without (some) guessing reproduce the ppo2 values used for each sensor.

I feel like it's a quite good feature to be able to analyze the individual sensor values, to gauge response time, and potentially find any early signs of current limitations in the sensors. Now when I've started diving rebreather myself I might get around to implement such a feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

As I remember it, we don't have real ppo2 values from the sensors, just mV and thus we need the calibration value to convert those to ppo2 to be able to report them.

Correct.

Also, as I remember it, we will always report the "voted" ppo2 value if we have it, which as far as I remember we should always have for dives with sensors, and that value is stored as ppo2.

As far as I know yes.

The current sensor interface doesn't expose the raw mV values. We could adjust the interface to expose those, so one can graph those and look at those even if we don't have valid calibration values.

The problem there is the storage format - we currently persist sensor1 (which is the 'voted' value) ... sensorX, and the value is always in bar. This is not great, but any change away from this will be breaking backwards compatibility, which sucks, in particular for the cloud storage.

Maybe they save the DiveCAN calibration values somewhere else? Or maybe they just get the ppo2 and mV values over DiveCAN and just saves the mV and the voted ppo2, in which case we can't without (some) guessing reproduce the ppo2 values used for each sensor.

Yes - with DiveCAN the calibration is saved on the O2 controller inside the unit. This way the unit can continue to function normally even when the handset is cable is severed or unplugged, or the handset battery dies.
I suspect that with DiveCAN the O2 controller uses the handset as a 'dumb display' and directly sends it the ppO2 values to be shown - so, in order to avoid breaking the specification of their log format, Shearwater then uses an assumed default calibration value to convert ppO2 values into the mV values that are logged.

I feel like it's a quite good feature to be able to analyze the individual sensor values, to gauge response time, and potentially find any early signs of current limitations in the sensors. Now when I've started diving rebreather myself I might get around to implement such a feature.

I definitely think the individual sensor graphs are important.
Also, welcome to the dark side. 😄

WARNING (abstract->context, "Disabled all O2 sensors due to a default calibration value.");
parser->calibrated = 0;
ppo2_source = "voted/averaged";
} else {
parser->calibrated = data[base];
ppo2_source = "cells";
}

// Get the dive mode from the header (if available).
if (logversion >= 8) {
Expand Down Expand Up @@ -830,8 +821,6 @@ shearwater_predator_parser_cache (shearwater_predator_parser_t *parser)
case M_CC:
case M_CC2:
DC_ASSIGN_FIELD(parser->cache, DIVEMODE, DC_DIVEMODE_CCR);
if (ppo2_source)
dc_field_add_string(&parser->cache, "PPO2 source", ppo2_source);
break;
case M_SC:
DC_ASSIGN_FIELD(parser->cache, DIVEMODE, DC_DIVEMODE_SCR);
Expand Down
Loading