[BMI270] Fix bug in data frame size calculation #366
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Previously, required_length was always 0 because we shifting by a huge number, rather than the bit position. So the check for incomplete frames would never trigger. The final data frame in a FIFO read is sometimes an incomplete frame, and therefore we would read garbage for the data frame. There are often incomplete frames when we call LEDManager to blink the LED, because the FIFO overruns. 1. During gyroscope calibration, we often read random gyroscope samples, which throws off the calibration results. 2. During 6-sided calibration, we often read random acceleration samples which causes rest detection to move on to collecting the next side, even if we don't move the tracker. 3. During normal operation, we will rarely read random gyroscope and accelerometer samples. These are usually one-offs and probably don't affect tracking. This change fixes the bounds check so that we correctly identify incomplete frames and discard them.
jabberrock
force-pushed
the
jabber-bmi270-fix-data-frames
branch
from
37d9e94
to
7004468
Compare
l0ud
reviewed
Nov 21, 2024
| + ((header & Fifo::AccelDataBit) >> Fifo::AccelDataBit)) | ||
| * 6; | ||
| uint8_t gyro_data_length | ||
| = (header & Fifo::GyrDataBit) == Fifo::GyrDataBit ? 6 : 0; |
There was a problem hiding this comment.
it's enough to (header & Fifo::GyrDataBit ? 6 : 0) afaik (should be even less operations)
similar for accel
|
@unlogisch04, @l0ud thanks for the review! Could you merge it? I don't have permissions yet |
unlogisch04
approved these changes
Nov 21, 2024
I also don't have permission to merge. Only @ButterscotchV or @Eirenliel have permission to merge. |
ButterscotchV
approved these changes
Nov 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, required_length was always 0 because we shifting by a huge number, rather than the bit position. So the check for incomplete frames would never trigger. The final data frame in a FIFO read is sometimes an incomplete frame, and therefore we would read garbage for the data frame. There are often incomplete frames when we call LEDManager to blink the LED, because the FIFO overruns.
This change fixes the bounds check so that we correctly identify incomplete frames and discard them.