Fixed I2C contention & timeout bugs #118
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
This pull request fixes two related bugs, which resulted in high CPU load from a high-priority task causing I2C timeouts, which led to tasks spinning endlessly in the (now removed) read/write spin-locks, which finally resulted in an assertFail on reenabling a FreeRTOS timer. This issue was exacerbated by the recent size increase in gyro bias calibration (more calculations from a high priority task), particularly if a radio connection is also established during the calibration routine. The bugs fixed by this pull request are: 1. Improper semaphore usage. * Previously the I2C semaphores were initialised as unlocked (the subsequent xSemaphoreTake in the initialization was forgotten), and as such that they could be taken, before first being given. * This caused the (intended-to-be-blocking) xSemaphoreTakes to always pass immediately, which is the reason for the spin-locks (since the code did not wait for the completion interrupts). * This commit changes from the deprecated vSemaphoreCreateBinary (which creates the semaphore in an unlocked state, and would require a subsequent xSemaphoreTake) to the more current xSemaphoreCreateBinary (which creates the semaphore in the locked state). 2. I2C Timeouts * The calls to CPAL_I2C_Read and CPAL_I2C_Write contain a spin-lock to detect timeout. * If a context switch occurs during this function (ie. if the task is interrupted by a higher priority task), then I2C will timeout. * This pull request disables context switching for the duration of these calls. Signed-off-by: Mike Hamer <[email protected]>
|
This should fix your problem, @ataffanel. Note that I only fixed this yesterday and thus it has only been running for a few hours, more testing is probably required. |
|
Thanks! I will merge it and so we have much more tester! :) This bug has be haunting us for a while so it is really exciting to see a potential fix. |
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.
This pull request fixes two related bugs, which resulted in high CPU load from a high-priority task causing I2C timeouts, which led to tasks spinning endlessly in the (now removed) read/write spin-locks, which finally resulted in an assertFail on reenabling a FreeRTOS timer. This issue was exacerbated by the recent size increase in gyro bias calibration (more calculations from a high priority task), particularly if a radio connection is also established during the calibration routine.
The bugs fixed by this pull request are: