-
Notifications
You must be signed in to change notification settings - Fork 907
Disabled interrupts for timing critical code section #2234
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
Open
Bobby2000
wants to merge
3
commits into
crankyoldgit:master
Choose a base branch
from
Bobby2000:workaround-for-rtos-tasks
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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 file contains hidden or 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 file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -54,6 +54,7 @@ void IRsend::sendCOOLIX(uint64_t data, uint16_t nbits, uint16_t repeat) { | |
| enableIROut(38); | ||
|
|
||
| for (uint16_t r = 0; r <= repeat; r++) { | ||
| beginCritical(); | ||
|
Owner
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See other comment. It would be great to remove this stuff if we can. i.e. Make it hidden and self-applying. |
||
| // Header | ||
| mark(kCoolixHdrMark); | ||
| space(kCoolixHdrSpace); | ||
|
|
@@ -75,6 +76,7 @@ void IRsend::sendCOOLIX(uint64_t data, uint16_t nbits, uint16_t repeat) { | |
| // Footer | ||
| mark(kCoolixBitMark); | ||
| space(kCoolixMinGap); // Pause before repeating | ||
| endCritical(); | ||
| } | ||
| space(kDefaultMessageGap); | ||
| } | ||
|
|
||
This file contains hidden or 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 file contains hidden or 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 file contains hidden or 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 file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What are the implications & practicalities of NOT calling
vTaskDelay(1)here?I'd much prefer to have the critical calls moved inside the
mark()method so it works seemlessly for all protocols & uses.For
space()s, we might be able to use the critical calls only when we are not making adelay()call. i.e. very small spaces. Thus it should yield for longer gaps when the timing doesn't matter as much.Has there been any analysis of where abouts in the message the interrupts are causing the message to be corrupted/not understood? My guess is in the bit-banged PWM
mark()messages and shortspace(), so if we effectively yield only for very long space, we could move the critical calls deeper inside, yield the CPU earlier with less impact & cleaner implementation.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The vTaskDelay call is there to allow the OS task scheduler to assign CPU time for other tasks like the idle task. The delay does not implement a busy wait loop, but rather tells the scheduler to suspend the task for x milliseconds allowing other tasks to run. This is important for the idle loop that, among other things, handles the watchdog timer "kick" preventing the CPU from doing a hard reset. Not having it there could cause the task to enter a new critical section right away and possibly exhausting the scheduler. Critical section disables interrupts and prevent the OS task scheduler from doing its job and therefor should be used with care. I did try to limit the critical section to the senddata functions but it did not work. I am guessing the entire command section including preamp, header, data and footer is timing critical? The reason I kept the critical section within the repeat loop is to allow other tasks to run between each command, since delays between commands are tolerated. I will try to do some more testing and see if I can get the critical section closer to the core transmit logic. Hang on and I will get back with the results.....