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.

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

Rework SPL06 to do background updates compliant with the spec #28983

Merged
merged 5 commits into from
Feb 27, 2025

Conversation

andyp1per
Copy link
Collaborator

@andyp1per andyp1per commented Jan 1, 2025

Pulls in some bug fixes from betaflight to make the code more understandable.
Enabled background updates by default (which is what the DPS310 driver does)
Uses external temperature sensor as required by the spec.

This results in much more accurate baro updates from this sensor.

Tested on BetaFPV F405

@andyp1per andyp1per changed the title Rework SPL06 to do background updates compliant with the spec. Rework SPL06 to do background updates compliant with the spec Jan 1, 2025
@peterbarker
Copy link
Contributor

@radiolinkW this PR will directly affect your board RadiolinkPIX6

Wu was the committer on the commit which added to the SPL06 driver to add SPA06 support, and to add the ability to do "background" reading on this sensor.

@andyp1per has tested this on a board he is currently working on, and the background reading does improve the driver's performance significantly.

Would it be possible for you to test this PR on your RadiolinkPIX6 board and verify that the Baro still works as expected?

@andyp1per
Copy link
Collaborator Author

No response from radiolink

@radiolinkW
Copy link
Contributor

radiolinkW commented Jan 17, 2025

Regarding TMP_EXT(external temperature sensor), it is in the TMP_CFG register of SPL06, but in the MEAS_CFG register of SPA06.

Copy link
Contributor

@peterbarker peterbarker left a comment

Choose a reason for hiding this comment

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

Looks like this would have us write incorrect configuration into the SPA06.

@peterbarker
Copy link
Contributor

Regarding TMP_EXT(external temperature sensor), it is in the TMP_CFG register of SPL06, but in the MEAS_CFG register of SPA06.

Thanks for checking, we'll make sure we don't break that bit.

Can you confirm https://vi.aliexpress.com/item/1005007607117391.html?gatewayAdapt=glo2vnm always contain the SPA06, please? It'll be worth getting one of those into an ArduPilot DevTeam member's hands so we can check we're not breaking this sensor without bugging you :-)

@andyp1per
Copy link
Collaborator Author

Fixed

@tridge
Copy link
Contributor

tridge commented Jan 21, 2025

@andyp1per I think you should buy one of these boards so we can test the driver changes, can you apply for funding for that?

@tridge
Copy link
Contributor

tridge commented Jan 21, 2025

Tested on BetaFPV F405

BETAFPV-F405 doesn't list the SPL06 baro in hwdef? is this a variant?

@radiolinkW
Copy link
Contributor

Regarding TMP_EXT(external temperature sensor), it is in the TMP_CFG register of SPL06, but in the MEAS_CFG register of SPA06.

Thanks for checking, we'll make sure we don't break that bit.

Can you confirm https://vi.aliexpress.com/item/1005007607117391.html?gatewayAdapt=glo2vnm always contain the SPA06, please? It'll be worth getting one of those into an ArduPilot DevTeam member's hands so we can check we're not breaking this sensor without bugging you :-)

Yes, the RadiolinkPIX6 we ship includes the SPA06 baro, and we are waiting for you to release the 4.6 firmware because only the 4.6 firmware supports the SPA06 driver.
When will you release the 4.6 firmare?

@radiolinkW
Copy link
Contributor

radiolinkW commented Jan 21, 2025

@andyp1per I think you should buy one of these boards so we can test the driver changes, can you apply for funding for that?

We can provide a free RadiolinkPIX6 for you to test the SPA06 driver.
@andyp1per Can you provide me with your email address so that i can send you a free RadiolinkPIX6 purchase link?

@andyp1per
Copy link
Collaborator Author

andyp1per commented Jan 21, 2025

@andyp1per I think you should buy one of these boards so we can test the driver changes, can you apply for funding for that?

We can provide a free RadiolinkPIX6 for you to test the SPA06 driver. @andyp1per Can you provide me with your email address so that i can send you a free RadiolinkPIX6 purchase link?

E-mail sent

@andyp1per
Copy link
Collaborator Author

BETAFPV-F405 doesn't list the SPL06 baro in hwdef? is this a variant?

#28962

@andyp1per
Copy link
Collaborator Author

@tridge TBH the other option is to delete the driver and fold the SPA06 fixes into the DPS310 driver - BF only has one driver

@peterbarker
Copy link
Contributor

@tridge TBH the other option is to delete the driver and fold the SPA06 fixes into the DPS310 driver - BF only has one driver

So both SPL06 and SPA06 would move to the DPS310? Interesting...

@peterbarker
Copy link
Contributor

Regarding TMP_EXT(external temperature sensor), it is in the TMP_CFG register of SPL06, but in the MEAS_CFG register of SPA06.

Thanks for checking, we'll make sure we don't break that bit.
Can you confirm https://vi.aliexpress.com/item/1005007607117391.html?gatewayAdapt=glo2vnm always contain the SPA06, please? It'll be worth getting one of those into an ArduPilot DevTeam member's hands so we can check we're not breaking this sensor without bugging you :-)

Yes, the RadiolinkPIX6 we ship includes the SPA06 baro, and we are waiting for you to release the 4.6 firmware because only the 4.6 firmware supports the SPA06 driver. When will you release the 4.6 firmare?

We're just about to release -beta3 for 4.6 Can't be that much longer!

@andyp1per
Copy link
Collaborator Author

@radiolinkW are you able to test this change on your board? I sent you e-mail, but response so far.

@radiolinkW
Copy link
Contributor

@radiolinkW are you able to test this change on your board? I sent you e-mail, but response so far.

I haved replied to your email. We are on Spring Festival holiday and probably cannot test it.

@andyp1per
Copy link
Collaborator Author

Tested on SpeedyBeeF405AIO which has an SPA6 - checked out. I added the correct devid so that we can identify the different parts.

@andyp1per andyp1per requested a review from tridge February 24, 2025 15:23
@peterbarker
Copy link
Contributor

Tested on SpeedyBeeF405AIO which has an SPA6 - checked out. I added the correct devid so that we can identify the different parts.

The temperature register that @radiolinkW were interested in is functional?

@andyp1per
Copy link
Collaborator Author

Tested on SpeedyBeeF405AIO which has an SPA6 - checked out. I added the correct devid so that we can identify the different parts.

The temperature register that @radiolinkW were interested in is functional?

Right, that fix is part of this PR

@andyp1per
Copy link
Collaborator Author

andyp1per commented Feb 27, 2025

Fixes applied

#define READ_LENGTH 9

for (uint8_t i = 0; i < ARRAY_SIZE(buf); ) {
ssize_t chunk = MIN(READ_LENGTH, SPL06_CALIB_COEFFS_LEN - i);
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
ssize_t chunk = MIN(READ_LENGTH, SPL06_CALIB_COEFFS_LEN - i);
const ssize_t chunk = MIN(READ_LENGTH, ARRAY_SIZE(buf) - i);

@@ -200,7 +238,12 @@ bool AP_Baro_SPL06::_init()

_instance = _frontend.register_sensor();

_dev->set_device_type(DEVTYPE_BARO_SPL06);
if(type == Type::SPA06) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if(type == Type::SPA06) {
switch (type) {

@andyp1per andyp1per merged commit fba4031 into ArduPilot:master Feb 27, 2025
102 checks passed
@andyp1per andyp1per deleted the pr-spl06-fixes branch February 27, 2025 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants