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

Handle invalid baro pressure values #12815

Merged
merged 1 commit into from May 21, 2023

Conversation

haslinghuis
Copy link
Member

Fixes: #12744

@haslinghuis haslinghuis added this to the 4.5 milestone May 15, 2023
@haslinghuis haslinghuis self-assigned this May 15, 2023
@github-actions

This comment has been minimized.

@blckmn
Copy link
Member

blckmn commented May 16, 2023

AUTOMERGE: (FAIL)

  • github identifies PR as mergeable -> FAIL
  • assigned to a milestone -> PASS
  • cooling off period lapsed -> PASS
  • commit count less or equal to three -> PASS
  • Don't merge label NOT found -> PASS
  • at least one RN: label found -> PASS
  • Tested label found -> PASS
  • assigned to an approver -> PASS
  • approver count at least three -> FAIL

src/main/sensors/barometer.c Outdated Show resolved Hide resolved
src/main/sensors/barometer.c Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

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.

NaN source shall be fixed

src/main/sensors/barometer.c Show resolved Hide resolved
src/main/sensors/barometer.c Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

src/main/sensors/barometer.c Outdated Show resolved Hide resolved
src/main/sensors/barometer.c Outdated Show resolved Hide resolved
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@ledvinap
Copy link
Contributor

Hmm ... only change to get NaN is in pressureToAltitude, by passing negative pressure.
So either check for negative result in ms5611 (https://github.com/betaflight/betaflight/blob/master/src/main/drivers/barometer/barometer_ms5611.c#L195) or in/before pressureToAltitude

@haslinghuis
Copy link
Member Author

Should we drop the call to pressureToAltitude if pressure is negative or should we set it to 0 and let it calculate?
Also like to have root cause analysis as this was not an issue in 4.3.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@haslinghuis haslinghuis added Tested and removed Draft labels May 18, 2023
Copy link

@Neolight79 Neolight79 left a comment

Choose a reason for hiding this comment

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

Tested these changes on 4.4.1 - barometer MS5611 works well. (4.5.0 does not see any of the devices at all, but this is not relevant to these changes)
Thanks a lot @haslinghuis and @ledvinap

src/main/sensors/barometer.c Outdated Show resolved Hide resolved
src/main/sensors/barometer.c Show resolved Hide resolved
src/main/sensors/barometer.c Outdated Show resolved Hide resolved
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.

Looks good.

Now, someone should investigate why is ms5611 returning bad values shortly after initialization and make sure that negative/zero pressure is always returned in that case.

@ledvinap
Copy link
Contributor

Can someone open MS5611 issue?

One possible way to investigate MS5611 problem is to dump I2C traffic (Saleae). If dump is available, I can look at it.

@Neolight79
Copy link

Can someone open MS5611 issue?

One possible way to investigate MS5611 problem is to dump I2C traffic (Saleae). If dump is available, I can look at it.

Well, this is interesting task, but I'm not fast in such projects. Anyway, I'll order board for Saleae and try to dump and analyze data. It won't fast. I'll be back when I'll have useful info.

@github-actions

This comment has been minimized.

@github-actions
Copy link

Do you want to test this code? Here you have an automated build:
Assets
WARNING: It may be unstable. Use only for testing! See: https://www.youtube.com/watch?v=I1uN9CN30gw for instructions for unified targets!

@KarateBrot KarateBrot changed the title Fix MS5611 Handle invalid baro pressure values May 21, 2023
@haslinghuis haslinghuis merged commit f13a73d into betaflight:master May 21, 2023
19 checks passed
@haslinghuis haslinghuis deleted the fix-ms5611 branch May 21, 2023 09:17
@Neolight79
Copy link

Can someone open MS5611 issue?

One possible way to investigate MS5611 problem is to dump I2C traffic (Saleae). If dump is available, I can look at it.

@ledvinap I've found what wrong data exactly is coming from MS5611. The way I did it is simple: first of all I found the number of wrong values - it is always 1, I simply added the counter of wrong values and put this number to DEBUG_SET(DEBUG_BARO, 0, WrongNumbersCount);.
The second step was to split int32_t into two int_t and put them into other debug variables:

DEBUG_SET(DEBUG_BARO, 1, (int)(WrongValue>>16));
DEBUG_SET(DEBUG_BARO, 2, (int)(WrongValue&65535));

This number obviosly is not changing in time, but I'm receiving different least significant part after every reset. I made 6 resets and received the following wrong pressure values:

1111 1111 1111 1010 | 1111 1011 1011 1000
1111 1111 1111 1010 | 1111 1010 1000 1010
1111 1111 1111 1010 | 1111 1001 1111 0110
1111 1111 1111 1010 | 1111 1001 1101 1011
1111 1111 1111 1010 | 1111 1010 0101 1100
1111 1111 1111 1010 | 1111 1010 1010 0011

Five most significant octets are not changing. Only three least significant octets are changing.
Hope it will help.
Thanks a lot!

AkankshaJjw pushed a commit to AkankshaJjw/betaflight that referenced this pull request May 29, 2023
davidbitton pushed a commit to davidbitton/betaflight that referenced this pull request Feb 5, 2024
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.

GEPRC_F722_AIO: External barometer GEP-M8Q not working after update to BF 4.4.1
6 participants