Fix bug in analogWrite when accessing 16 bit register #94
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
|
Memory usage change @ c68af81
Click for full report table
Click for full report CSV |
|
LGTM! Can someone else (thinking about @SpenceKonde and @MCUdude 😉 ) test this patch and report if it works fine? Thanks a lot! |
facchinm
approved these changes
Dec 30, 2020
|
|
Just wanted to know if this patch was successfully tested by someone else. Thanks |
|
Haven't had a chance, too busty lately, It looks sound though. I'm just going to take it as is into DxCore (which does do PWM with type B timers, much as it pains me to see such capable3 utility timers reduced to 8-bit PWM) and see if anyone complains. |
MCUdude
added a commit
to MCUdude/MegaCoreX
that referenced
this pull request
May 23, 2021
@SpenceKonde did anyone complain since the integration into DxCore? |
Hi @SpenceKonde. Did you have feedback from DxCore users about this change? |
|
Nope. But... i also never remembered to do it. So that is less than helpful isn't it? The Dx-series, with 14 other, better PWM pins in the 48+ pin versions, and most of the type B timers not used by a pin at all (because there are better timers there) doesn't make for a place where this would be tested often.... |
Hi @MCUdude. Did you implement this patch in MegaCoreX and did you have feedback from users? Hi @MCUdude. Did you have complaints from users since you installed the patch in MegaCoreX ? Your feedback would be great so that we can move forward on this pull request. Thanks |
|
I just realized why nobody ever complained about this bug on DxCore.... What value will the TEMP register be expected to have in it? When the timer was set up, if you set it to 8-bit PWM mode, the period gets written to the low byte, but actually will be written to the TCBn.TEMP register, before an initial value gets written to the high byte as a default duty cycle. Each TCB has it's own TEMP register. So there are only four cases, that could cause an issue here, but most either don't, or are a wacky use case.
Does the official core use a TCB configured for PWM as the millis timer? If so, this should be rectified posthaste, as micros() followed by analogWrite on the wrong pin would be expected to cause breakage, and is kinda not a rare thing to do. The other things that can cause breakage are real wacky corner cases. DxCore does not permit users to use the millis timer for PWM, It also configures the timer very differently (I count to F_CPU/1000-1 - so I get exactly one millisecond per overflow, makes life easier and the mess of math to do the necessary division via bitshifts tractable. Anyway, on DxCore, analogWrite treats a pin only driven by the a TCB used as millis timer as not having PWM available while megaTinyCore doesn't support the TCB for PWM at all (nobody has asked for it and pushed back on "We don't support that because it rarely gives you an extra pin, we don't have enough TCBs, and it would waste flash for everyone even those who didn't use it" and they've asked for about every wacky feature you can think of and plenty of things that you can't. Putting TCA0 into split mode gives people enough PWM pins that they rarely run out of PWM pins. 6 PWM channels on a part with 11 usable I/O pins, or 8 on a part with 17 or 21 usable I/O pins seems to be enough to keep people happy! (most use 1-series with 2 more from TCD0.... And I don't think I've even cleaned up and checked in the example of how to beat a third independent channel (same frequency) out of TCD0 using the CCL.... I also use every opportunity to disparage capability of the TCBs as PWM sources, since they, uh, do kinda suck at it. The code suggested above is sound, sane and simple, fully consistent with datasheet guidelines. and I have no reservations about that implementation, which is going into DxCore now, though I was definitely tempted to just ignore it for the sake of saving... what, 5 clocks and 4 instruction words? when I realized the hoops one needs to jump through for it to be a problem and why nobody complained about it. |
|
Thanks @SpenceKonde for your detailed feedback. Do you think Arduino will use AVR DA and DB processors for some of their future boards? @facchinm this patch has been successfully tested in DxCore and is now part of it. Are there any additional tests or actions to do before implementing it in the megaavr lib? |
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 fix solves bug #91 which only affects timer B.
I also added a save/clear/restore interrupt flag sequence for timer A around the non-atomic 16-bit write operation, to avoid data corruption if an interrupt occurs when writing the 16-bit register.