Skip to content

Add support for SPL07_003 barometer - based on DPS310 #13427

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.

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

Merged
merged 10 commits into from
Mar 26, 2024

Conversation

haslinghuis
Copy link
Member

@haslinghuis haslinghuis commented Mar 6, 2024

Copy link

github-actions bot commented Mar 6, 2024

Do you want to test this code? You can flash it directly from Betaflight Configurator:

  • Simply put #13427 (this pull request number) in the Select commit field of the Configurator firmware flasher tab (you need to Enable expert mode, Show release candidates and Development).

WARNING: It may be unstable. Use only for testing!

@haslinghuis haslinghuis added this to the 4.5 milestone Mar 6, 2024
@haslinghuis haslinghuis force-pushed the add-spa06-003-support branch from 8a19f75 to 9f22036 Compare March 6, 2024 22:45
@kedeng
Copy link
Contributor

kedeng commented Mar 7, 2024

Defining USE_BARO_DPS310_GOERTEK will result in a compile error:

image

@ledvinap
Copy link
Contributor

ledvinap commented Mar 7, 2024

SPA006_003 uses longer polynom for calibration, c21 and c40 must be added. And used in calculation.
Also, after testing, this should be merged with autodetection enabled, so both SPA006_003 and DPS310 do work.

@haslinghuis haslinghuis changed the title Add support for GoerTek SPA006_003 barometer - based on DPS310 Add support for SPA006_003 / SPL07_003 barometer - based on DPS310 Mar 7, 2024
Copy link
Contributor

@ledvinap ledvinap left a comment

Choose a reason for hiding this comment

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

coefs are still missing in calculation

Comment on lines 187 to 188
if (!busReadBuf(dev, DPS310_REG_COEF, coef, READ_LENGTH)) {
return false;
}
if (!busReadBuf(dev, DPS310_REG_COEF + READ_LENGTH, coef + READ_LENGTH, COEFFICIENT_LENGTH - READ_LENGTH)) {
if (!busReadBuf(dev, DPS310_REG_COEF + READ_LENGTH, coef + READ_LENGTH, COEFFICIENT_LENGTH - READ_LENGTH)) {
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

for (unsigned i = 0; i < sizeof(coef); ) {
   int chunk = MIN(9, sizeof(coef) - i);
   if (!busReadBuf(dev, DPS310_REG_COEF + i,  coef + i, chunk) {
      return false;
   }
   i += chunk;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of 9 we need READ_LENGTH (COEFFICIENT_LENGTH / 2) ???

Copy link
Contributor

Choose a reason for hiding this comment

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

Why? It seems someone decided reading all 18 bytes is too much. So it was split into 2 chunks.
IMO it is pretty arbitrary and reading all bytes shall work, but it is safer to keep tested readout mechanism.

@haslinghuis
Copy link
Member Author

coefs are still missing in calculation

c31 and c40 only applies to the new chip ID.

@ledvinap
Copy link
Contributor

ledvinap commented Mar 7, 2024

c31 and c40 only applies to the new chip ID.

baroState.pressure = c00 + Praw_sc * (c10 + Praw_sc * (c20 + Praw_sc * c30)) + Traw_sc * c01 + Traw_sc * Praw_sc * (c11 + Praw_sc * c21);

SPA006_003 uses polynomials with higher order

@haslinghuis haslinghuis requested a review from ledvinap March 10, 2024 19:28
@haslinghuis haslinghuis changed the title Add support for SPA006_003 / SPL07_003 barometer - based on DPS310 Add support for SPA06_003 / SPL07_003 barometer - based on DPS310 Mar 10, 2024
Copy link
Contributor

@ledvinap ledvinap left a comment

Choose a reason for hiding this comment

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

Just minor style comments

@haslinghuis haslinghuis changed the title Add support for SPA06_003 / SPL07_003 barometer - based on DPS310 Add support for SPL07_003 barometer - based on DPS310 Mar 11, 2024
@ctzsnooze
Copy link
Member

These were some notes I made previously:
DPS310 0x77 = 119 by default, can be 0x76 = 118 if SDO is pulled to ground; betaflight uses 118

BMP280 0x76 = 118 (default, SDO connected to ground) (0x77 or 119 if set high, same as BMP180)
SPL06-007 0x77 = 119 (default, mimics DPS310) or 0x76 = 118 (if SDO pin is held to ground; mimics BMP280)

SPL07-003 0x77 = 119 (default, mimics DPS310) or 0x76 = 118 (if SDO pin is held to ground, mimics BMP280)

The SLP06-007 reports Product and Revision ID of 0x10, which exactly is the same as the DPS310, and has the same number of registers for the calibration coefficients. This is why it often is reported as the DPS310 in our code when we check status.

I wonder if the reason we have issues with the temperature values reported by SPL06's is because their coefficients are not being retrieved correctly, when considered as being DPS310's?

The SPL07 reports Product and Revision ID of 0x11, different from the DPS310. Compared to the SPL06 it has similar accuracy at a higher sample rate (200Hz). The SPL06 hasn't been a great performer. We specifically need to know if the SPL07 reports temperature more accurately, to improve pressure accuracy with temperature change. SPL06 is awful in that respect.

I'm a big fan of the newer Infineon DPS368.

@ledvinap
Copy link
Contributor

ad SPL06-007: You can try dumping coefficients, maybe there is some inconsistency ...
Also, calibration temp source may be wrong (The bit describing used source does not seem to be documented)

@haslinghuis
Copy link
Member Author

Tested with SPL07_003 barometer:

image

@ctzsnooze
Copy link
Member

ctzsnooze commented Mar 20, 2024

@haslinghuis was the temperature of the board likely to be 41.27 degrees when tested?
Maybe let the board start cold, at room temp, and watch temperature, as time passes, and as the board warms up?
The main problem with the clones was a failure to individualise the cal factors, particularly for temperature.

PS: if the altitude was just being lifted up and down in relation to when powered up, it's a VERY clean altitude trace - far better than the BMP280 in terms of noise!

@haslinghuis
Copy link
Member Author

haslinghuis commented Mar 20, 2024

PS: if the altitude was just being lifted up and down in relation to when powered up, it's a VERY clean altitude trace - far better than the BMP280 in terms of noise!

@ctzsnooze Yes had lifted the board up and down 3 times in a row :)

Did some bench testing. Room temp is 19 °C. Temp of the speedybee f405mini was around 65 °C. Using a fan it comes down.

press temp alt board
1015 1350 80 airbotg4aio (dps310) - cpu = 39 (master)
1015 1405 75 airbotg4aio (dps310) - cpu = (#13427)
1020 4010 64 iflight_blitz_f435 (dps310) - (master)
1020 4170 25 iflight_blitz_f435 (dps310) - (#13427)
1020 3853 22 matekh743 (dps310) - (master)
1020 3849 26 matekh743 (dps310) - (#13427)
1020 3800 80-138 kakuteh7mini (bmp280) - (master)
1019 4600 51 speedybeef405 (goertek) - (mfg)
1018 4900 22 speedybeef405 (goertek) - (#13427)

@haslinghuis haslinghuis requested a review from ctzsnooze March 21, 2024 22:46
@haslinghuis haslinghuis force-pushed the add-spa06-003-support branch from d6ee352 to 218b342 Compare March 22, 2024 22:08
@haslinghuis
Copy link
Member Author

haslinghuis commented Mar 22, 2024

Did some more intensive testing over time.

Rebased on master and added a commit to be more explicit on the new chip added. Original DPS310 is now not affected by this change in any way as altitude reading on DPS310 was affected (where altitude keeps rising with temperature) - so Goertek added coef calculation seems wrong for DPS310!!!

image

This is now solved.

@haslinghuis haslinghuis force-pushed the add-spa06-003-support branch from 218b342 to 06ac241 Compare March 23, 2024 00:20
@ledvinap
Copy link
Contributor

Original DPS310 is now not affected by this change in any way as altitude reading on DPS310 was affected (where altitude keeps rising with temperature) - so Goertek added coef calculation seems wrong for DPS310!!!

that's strange. the new coefs were set to 0, so the calculation should not change for DPS310..

@haslinghuis
Copy link
Member Author

haslinghuis commented Mar 23, 2024

Bench testing AIRBOTG4AIO lying still on the bench (using master build)

10:43:

image

10:45:

image

10:47:

image

10:50

image

10:52

image

Co-authored-by: Petr Ledvina <ledvinap@gmail.com>
Copy link
Member

@nerdCopter nerdCopter left a comment

Choose a reason for hiding this comment

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

approving -- i'll leave the testing and technicalities to ya'll, but the code looks fair to me.

@ctzsnooze
Copy link
Member

ctzsnooze commented Mar 25, 2024

I've explained the temperature compensation issue with these Goertek Baro's many times.

The 'real' Infineon-branded DPS310 chips have individualised calibration factors - for every individual chip they produce - for both pressure and temperature they flash individual cal values to the chip. As a result, the absolute temperature recorded by the chip is exceedingly accurate. Every single 'real' Infineon DPS310 will record room temperature very accurately. It is likely that the delta coefficient is also individualised, sine the real Infineon version does not drift with temperature.

Hence it is essential that if the chip really is an Infineon DPS310 then we need to use the coefficients provided by the chip.

The 'clone' Goertek chips do not individualise their temperature sensor coefficients during manufacturing. That's why we see bizarre absolute room temperature readings, eg -9C, or +40C, in your case, 11 deg C when at room temperature. It is likely that the delta coefficient is also not individualised because we often see significant drift with temperature with these devices.q

That's why I strongly advocate that manufacturers should use the real Infineon product. OK sure the DPS310 is out of production, but it has been superseded by an even better DPS368 with identical pinouts and coding - it's a drop-in replacement with about twice the resolution and half the noise.

I'm not certain we should approve this PR until it is checked against boards with real Infineon DPS310's, to ensure that their temperature measurements are accurate and that thermal stability is good. And we should ensure that a real Infineon DPS360 will work.

@haslinghuis haslinghuis merged commit 52af623 into betaflight:master Mar 26, 2024
@haslinghuis haslinghuis deleted the add-spa06-003-support branch March 26, 2024 23:11
@amaygoermicro
Copy link

The SPA06-003 driver is now supported in the master. Do you have a plan for when it will be released in Tag version?

@haslinghuis
Copy link
Member Author

We will have a final release candidate first.

@haslinghuis
Copy link
Member Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: COMPLETED
Development

Successfully merging this pull request may close these issues.

6 participants